[Mesa-dev] [PATCH 5/7] util/disk_cache: hash pointer size and gpu name into cache keys

[Timothy Arceri]

On 17/03/17 00:00, Grazvydas Ignotas wrote:
> On Thu, Mar 16, 2017 at 2:49 AM, Timothy Arceri <tarceri at itsqueeze.com> wrote:
>>
>>
>> On 16/03/17 10:09, Grazvydas Ignotas wrote:
>>>
>>> This allows to get rid of the arch and gpu name directories.
>>> Also, gpu_name is now optional.
>>>
>>> Signed-off-by: Grazvydas Ignotas <notasas at gmail.com>
>>> ---
>>>  src/compiler/glsl/tests/cache_test.c | 13 ++--------
>>>  src/util/disk_cache.c                | 47
>>> +++++++++++++++---------------------
>>>  src/util/disk_cache.h                | 17 -------------
>>>  3 files changed, 21 insertions(+), 56 deletions(-)
>>>
>>> diff --git a/src/compiler/glsl/tests/cache_test.c
>>> b/src/compiler/glsl/tests/cache_test.c
>>> index f923e8d..f6d3621 100644
>>> --- a/src/compiler/glsl/tests/cache_test.c
>>> +++ b/src/compiler/glsl/tests/cache_test.c
>>> @@ -182,16 +182,11 @@ test_disk_cache_create(void)
>>>     expect_null(cache, "disk_cache_create with XDG_CACHE_HOME set with"
>>>                 "a non-existing parent directory");
>>>
>>> -   /* Create string with expected directory hierarchy */
>>> -   char expected_dir_h[255];
>>> -   sprintf(expected_dir_h, "%s%s%s", CACHE_TEST_TMP
>>> "/xdg-cache-home/mesa/",
>>> -           get_arch_bitness_str(), "/test");
>>> -
>>>     mkdir(CACHE_TEST_TMP, 0755);
>>>     cache = disk_cache_create("test", version_blob, sizeof(version_blob));
>>>     expect_non_null(cache, "disk_cache_create with XDG_CACHE_HOME set");
>>>
>>> -   check_directories_created(expected_dir_h);
>>> +   check_directories_created(CACHE_TEST_TMP "/xdg-cache-home/mesa");
>>>
>>>     disk_cache_destroy(cache);
>>>
>>> @@ -204,15 +199,11 @@ test_disk_cache_create(void)
>>>     expect_null(cache, "disk_cache_create with MESA_GLSL_CACHE_DIR set
>>> with"
>>>                 "a non-existing parent directory");
>>>
>>> -   sprintf(expected_dir_h, "%s%s%s", CACHE_TEST_TMP
>>> -           "/mesa-glsl-cache-dir/mesa/", get_arch_bitness_str(),
>>> -           "/test");
>>> -
>>>     mkdir(CACHE_TEST_TMP, 0755);
>>>     cache = disk_cache_create("test", version_blob, sizeof(version_blob));
>>>     expect_non_null(cache, "disk_cache_create with MESA_GLSL_CACHE_DIR
>>> set");
>>>
>>> -   check_directories_created(expected_dir_h);
>>> +   check_directories_created(CACHE_TEST_TMP "/mesa-glsl-cache-dir/mesa");
>>>
>>>     disk_cache_destroy(cache);
>>>  }
>>> diff --git a/src/util/disk_cache.c b/src/util/disk_cache.c
>>> index 5b249d9..904aa66 100644
>>> --- a/src/util/disk_cache.c
>>> +++ b/src/util/disk_cache.c
>>> @@ -158,29 +158,6 @@ concatenate_and_mkdir(void *ctx, const char *path,
>>> const char *name)
>>>        return NULL;
>>>  }
>>>
>>> -static char *
>>> -create_mesa_cache_dir(void *mem_ctx, const char *path, const char
>>> *gpu_name)
>>> -{
>>> -   char *new_path = concatenate_and_mkdir(mem_ctx, path, "mesa");
>>> -   if (new_path == NULL)
>>> -      return NULL;
>>> -
>>> -   /* Create a parent architecture directory so that we don't remove
>>> cache
>>> -    * files for other architectures. In theory we could share the cache
>>> -    * between architectures but we have no way of knowing if they were
>>> created
>>> -    * by a compatible Mesa version.
>>> -    */
>>> -   new_path = concatenate_and_mkdir(mem_ctx, new_path,
>>> get_arch_bitness_str());
>>> -   if (new_path == NULL)
>>> -      return NULL;
>>> -
>>> -   new_path = concatenate_and_mkdir(mem_ctx, new_path, gpu_name);
>>> -   if (new_path == NULL)
>>> -      return NULL;
>>> -
>>> -   return new_path;
>>> -}
>>> -
>>>  struct disk_cache *
>>>  disk_cache_create(const char *gpu_name, const void *version_blob,
>>>                    size_t version_blob_size)
>>> @@ -218,7 +195,7 @@ disk_cache_create(const char *gpu_name, const void
>>> *version_blob,
>>>        if (mkdir_if_needed(path) == -1)
>>>           goto fail;
>>>
>>> -      path = create_mesa_cache_dir(local, path, gpu_name);
>>> +      path = concatenate_and_mkdir(local, path, "mesa");
>>>        if (path == NULL)
>>>           goto fail;
>>>     }
>>> @@ -230,7 +207,7 @@ disk_cache_create(const char *gpu_name, const void
>>> *version_blob,
>>>           if (mkdir_if_needed(xdg_cache_home) == -1)
>>>              goto fail;
>>>
>>> -         path = create_mesa_cache_dir(local, xdg_cache_home, gpu_name);
>>> +         path = concatenate_and_mkdir(local, xdg_cache_home, "mesa");
>>>           if (path == NULL)
>>>              goto fail;
>>>        }
>>> @@ -266,7 +243,7 @@ disk_cache_create(const char *gpu_name, const void
>>> *version_blob,
>>>        if (path == NULL)
>>>           goto fail;
>>>
>>> -      path = create_mesa_cache_dir(local, path, gpu_name);
>>> +      path = concatenate_and_mkdir(local, path, "mesa");
>>>        if (path == NULL)
>>>           goto fail;
>>>     }
>>> @@ -275,11 +252,25 @@ disk_cache_create(const char *gpu_name, const void
>>> *version_blob,
>>>     if (cache == NULL)
>>>        goto fail;
>>>
>>> -   cache->key_blob_size = version_blob_size;
>>> +   /* Prepare the blob to hash into the cache keys */
>>> +   uint8_t pointer_size = sizeof(void *);
>>> +   cache->key_blob_size = version_blob_size + sizeof(pointer_size);
>>> +   if (gpu_name != NULL)
>>> +      cache->key_blob_size += strlen(gpu_name);
>>>     cache->key_blob = ralloc_size(cache, cache->key_blob_size);
>>>     if (cache->key_blob == NULL)
>>>        goto fail;
>>> -   memcpy(cache->key_blob, version_blob, version_blob_size);
>>> +
>>> +   char *pos = cache->key_blob;
>>> +   memcpy(pos, version_blob, version_blob_size);
>>> +   pos += version_blob_size;
>>> +   memcpy(pos, &pointer_size, sizeof(pointer_size));
>>> +   pos += sizeof(pointer_size);
>>> +   if (gpu_name != NULL) {
>>
>>
>> Is there any valid reason for this to be NULL? Can we just add an assert()
>> and assume this is never NULL?
>
> I saw you passing "radv" in the radv cache patch so thought to make it
> optional instead of not-so-useful "radv" as gpu string.

Far enough. I think that is a mistake on my part as we will still want 
the gpu_id in the cache entries header for identification. I need to fix 
it but I'm holding off until we get your series in.