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

[Grazvydas Ignotas]

On Tue, Mar 14, 2017 at 12:20 AM, Timothy Arceri <tarceri at itsqueeze.com> wrote:
>
>
> On 13/03/17 22:53, Grazvydas Ignotas wrote:
>>
>> On Mon, Mar 13, 2017 at 3:01 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.
>>> ---
>>>  src/compiler/glsl/tests/cache_test.c | 49
>>> +++++++++++++++++++++++++++++++-----
>>>  src/util/disk_cache.c                | 41 ++++++++++++++++++++----------
>>>  2 files changed, 71 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/src/compiler/glsl/tests/cache_test.c
>>> b/src/compiler/glsl/tests/cache_test.c
>>> index 6451e65..f8259e0 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,27 @@ does_cache_contain(struct disk_cache *cache,
>>> cache_key key)
>>>
>>>     if (result) {
>>>        free(result);
>>>        return true;
>>>     }
>>>
>>>     return false;
>>>  }
>>>
>>>  static void
>>> +wait_one_second()
>>> +{
>>> +    unsigned wait_time = time(0) + 1;
>>> +    while (time(0) < wait_time);
>>> +}
>>
>>
>> If you enter this function at an unfortunate time (just as the second
>> is about to change) the function will not wait at all, so this will
>> cause spurious test failures. If you just sleep() it will waste
>> everyone's time on make check.
>>
>> It looks like flushing the queue in disk_cache_get() should solve
>> this, it might be needed anyway because the app may request something
>> while the thread is still writing it out?
>>
>
> I agree the wait is not ideal but it's more ideal that adding code just to
> pass tests. It's very unlikely an app will try to relink the same shader
> combination while we are still writing out and if it does I'm happy for this
> to trigger a relink, rather than adding more code which is unlikely to ever
> be used. If it's stuck waiting on I/O it's likely better to just recompile
> anyway.
>
> I guess the only options other than the wait are a flush helper that is only
> used by the test or expose the cache struct and access the queue directly in
> the test. Not really sure which of those is better.

I agree an interface just for the test might be too much. Could maybe
use a polling loop checking if the entry landed every 100ms or so and
giving up after some retries? Could also just stick a 300ms (or
similar) sleep but I fear it may cause spurious test failures on busy
systems. I think wine has that problem with their tests as they rely
on things happening after fixed sleep periods a lot.

Gražvydas