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

[Grazvydas Ignotas]

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.

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