[Mesa-dev] [PATCH V3] util/disk_cache: use a thread queue to write to shader cache

[Timothy Arceri]

On 14/03/17 21:27, Grazvydas Ignotas wrote:
> On Tue, Mar 14, 2017 at 4:06 AM, Timothy Arceri <tarceri at itsqueeze.com> wrote:
>> This should help reduce any overhead added by the shader cache
>> when programs are not found in the cache.
>>
>> To avoid creating any special function just for the sake of the
>> tests we add a one second delay whenever we call dick_cache_put()
>> to give it time to finish.
>>
>> V2: poll for file when waiting for thread in test
>> V3: fix poll delay to really be 100ms, and simplify the wait function
>> ---
>>   src/compiler/glsl/tests/cache_test.c | 63 ++++++++++++++++++++++++++++++++----
>>   src/util/disk_cache.c                | 41 +++++++++++++++--------
>>   2 files changed, 85 insertions(+), 19 deletions(-)
>>
>> diff --git a/src/compiler/glsl/tests/cache_test.c b/src/compiler/glsl/tests/cache_test.c
>> index 6451e65..10505d4 100644
>> --- a/src/compiler/glsl/tests/cache_test.c
>> +++ b/src/compiler/glsl/tests/cache_test.c
>> @@ -25,20 +25,21 @@
>>
>>   #include <stdio.h>
>>   #include <stdlib.h>
>>   #include <stdbool.h>
>>   #include <string.h>
>>   #include <ftw.h>
>>   #include <errno.h>
>>   #include <stdarg.h>
>>   #include <inttypes.h>
>>   #include <limits.h>
>> +#include <time.h>
>>   #include <unistd.h>
>>
>>   #include "util/mesa-sha1.h"
>>   #include "util/disk_cache.h"
>>
>>   bool error = false;
>>
>>   #ifdef ENABLE_SHADER_CACHE
>>
>>   static void
>> @@ -224,20 +225,40 @@ does_cache_contain(struct disk_cache *cache, cache_key key)
>>
>>      if (result) {
>>         free(result);
>>         return true;
>>      }
>>
>>      return false;
>>   }
>>
>>   static void
>> +wait_until_file_written(struct disk_cache *cache, cache_key key)
>> +{
>> +   struct timespec req;
>> +   struct timespec rem;
>> +
>> +   /* Set 100ms delay */
>> +   req.tv_sec = 0;
>> +   req.tv_nsec = 100000000;
>> +
>> +   unsigned retries = 0;
>> +   while (retries++ < 20) {
>> +      if (does_cache_contain(cache, key)) {
>> +         break;
>> +      }
>> +
>> +      nanosleep(&req, &rem);
>> +   }
>> +}
>> +
>> +static void
>>   test_put_and_get(void)
>>   {
>>      struct disk_cache *cache;
>>      /* If the text of this blob is changed, then blob_key_byte_zero
>>       * also needs to be updated.
>>       */
>>      char blob[] = "This is a blob of thirty-seven bytes";
>>      uint8_t blob_key[20];
>>      uint8_t blob_key_byte_zero = 0xca;
>>      char string[] = "While this string has thirty-four";
>> @@ -253,35 +274,49 @@ test_put_and_get(void)
>>      _mesa_sha1_compute(blob, sizeof(blob), blob_key);
>>
>>      /* Ensure that disk_cache_get returns nothing before anything is added. */
>>      result = disk_cache_get(cache, blob_key, &size);
>>      expect_null(result, "disk_cache_get with non-existent item (pointer)");
>>      expect_equal(size, 0, "disk_cache_get with non-existent item (size)");
>>
>>      /* Simple test of put and get. */
>>      disk_cache_put(cache, blob_key, blob, sizeof(blob), NULL);
>>
>> +   /* disk_cache_put() hands things off to a thread give it some time to
>> +    * finsih.
> finish
>
>> +    */
>> +   wait_until_file_written(cache, blob_key);
>> +
>>      result = disk_cache_get(cache, blob_key, &size);
>> -   expect_equal_str(blob, result, "disk_cache_get of existing item (pointer)");
>> -   expect_equal(size, sizeof(blob), "disk_cache_get of existing item (size)");
>> +   if (result) {
>> +      expect_equal_str(blob, result, "disk_cache_get of existing item (pointer)");
>> +      expect_equal(size, sizeof(blob), "disk_cache_get of existing item (size)");
>>
>> -   free(result);
>> +      free(result);
>> +   }
>>
>>      /* Test put and get of a second item. */
>>      _mesa_sha1_compute(string, sizeof(string), string_key);
>>      disk_cache_put(cache, string_key, string, sizeof(string), NULL);
>>
>> +   /* disk_cache_put() hands things off to a thread give it some time to
>> +    * finsih.
>> +    */
> finish (do we need to repeat the comment every time?)
>
>> +   wait_until_file_written(cache, string_key);
>> +
>>      result = disk_cache_get(cache, string_key, &size);
>> -   expect_equal_str(result, string, "2nd disk_cache_get of existing item (pointer)");
>> -   expect_equal(size, sizeof(string), "2nd disk_cache_get of existing item (size)");
>> +   if (result) {
>> +      expect_equal_str(result, string, "2nd disk_cache_get of existing item (pointer)");
>> +      expect_equal(size, sizeof(string), "2nd disk_cache_get of existing item (size)");
>>
>> -   free(result);
>> +      free(result);
>> +   }
> Here (and before) you check disk_cache_get() result for NULL ,,,
>
>>      /* Set the cache size to 1KB and add a 1KB item to force an eviction. */
>>      disk_cache_destroy(cache);
>>
>>      setenv("MESA_GLSL_CACHE_MAX_SIZE", "1K", 1);
>>      cache = disk_cache_create("test", "make_check");
>>
>>      one_KB = calloc(1, 1024);
>>
>>      /* Obviously the SHA-1 hash of 1024 zero bytes isn't particularly
>> @@ -299,20 +334,25 @@ test_put_and_get(void)
>>       * interestingly full than that).
>>       *
>>       * For this test, we force this signature to land in the same
>>       * directory as the original blob first written to the cache.
>>       */
>>      _mesa_sha1_compute(one_KB, 1024, one_KB_key);
>>      one_KB_key[0] = blob_key_byte_zero;
>>
>>      disk_cache_put(cache, one_KB_key, one_KB, 1024, one_KB);
>>
>> +   /* disk_cache_put() hands things off to a thread give it some time to
>> +    * finish.
>> +    */
>> +   wait_until_file_written(cache, one_KB_key);
>> +
>>      result = disk_cache_get(cache, one_KB_key, &size);
>>      expect_non_null(result, "3rd disk_cache_get of existing item (pointer)");
>>      expect_equal(size, 1024, "3rd disk_cache_get of existing item (size)");
> ... and here you don't. This should probably be made consistent. I
> don't think the NULL checking is needed though, the whole point of the
> test is to check if things work as expected.
Right that was dumb. I think I was just trying to get the test to stop 
crashing while I fixed the timing issues and forgot to go back and fix it.

>
> Other than that:
> Reviewed-by: Grazvydas Ignotas <notasas at gmail.com>
Thanks.