[Mesa-dev] [PATCH] glsl: fix disk cache eviction issue

[Timothy Arceri]

On Tue, 2017-01-10 at 22:34 +1100, Timothy Arceri wrote:
> On Tue, 2017-01-10 at 12:20 +0200, Tapani Pälli wrote:
> > 
> > On 01/09/2017 05:42 PM, Emil Velikov wrote:
> > > On 9 January 2017 at 15:36, Emil Velikov <emil.l.velikov at gmail.co
> > > m>
> > > wrote:
> > > > On 9 December 2016 at 05:58, Tapani Pälli <tapani.palli at intel.c
> > > > om
> > > > > wrote:
> > > > > When 'evict_random_item' attempts to remove cache content to
> > > > > make more
> > > > > space, it may try to remove from new cache directory it just
> > > > > created
> > > > > which only has '.tmp' content at this phase and not proper
> > > > > cache files,
> > > > > this will cause eviction to fail and 'test_put_and_get'
> > > > > subtest
> > > > > assumptions about cache size and item count won't hold.
> > > > > 
> > > > > Patch fixes this issue by adding 'exception' argument for
> > > > > random file
> > > > > matching so that algorithm won't pick the new directory.
> > > > > 
> > > > 
> > > > Hi Tapani,
> > > > 
> > > > I've mentioned this over at #android-ia, but adding it here for
> > > > posterity.
> > > > 
> > > > 
> > > > The issue itself is actually quite different - in my
> > > > experiments
> > > > with
> > > > importing SHA1 implementation to mesa, I've noticed that there
> > > > bug
> > > > lies with a buggy (and/or undefined behaviour) SHA1
> > > > implementation
> > > > given the input data.
> > > > 
> > > > Namely:
> > > > As one observes the output of _mesa_sha1_compute they will
> > > > notice
> > > > that
> > > > it differs across implementations. This in itself combined with
> > > > the
> > > > (afaict) correct assumption in test_put_and_get() that the
> > > > beginning
> > > > for the SHA1blob_key_byte_zero
> > > > 
> > > 
> > > [Pressed enter to quickly]
> > > 
> > > ...the SHA1 matches blob_key_byte_zero (as mentioned in the
> > > comment
> > > of
> > > said function) makes things work ... sometimes.
> > 
> > OK looks like this is going to be pretty tricky to debug.
> > Independent
> > of 
> > this test issue however I still believe my patch is
> > required/correct. 
> > There is nothing currently in cache eviction code that would
> > prevent
> > it 
> > from picking the directory created for new entry. If it picks it,
> > it 
> > does not delete anything away (since no cache files inside) and 
> > therefore does not respect the maximum size limit set for the
> > cache.
> 
> There are some interesting problems to be solved/improved with
> regards
> to eviction. The current version of shader-cache stores everything in
> a
>  single directory which I intend to change so that we instead store
> things in directory structures like so:
> 
>    ./Mesa-13.0.0/i965-BDW/
>    ./Mesa-13.0.1/i965-BDW/
>    ./Mesa-13.0.1/radeonsi/
> 
> This will help avoid costly fallbacks that are in the current
> patchset
> where we fallback to a recompile at drawtime the first time we use
> Mesa
> 13.0.1 on a shader that was cached in Mesa-13.0.0. This will also
> help
> 3rd parties wishing to push pre-compiled shaders to a users machine.
> 
> In this case we would always want to evict only from older Mesa
> versions one question is how much we should delete at a time.
> 
> Getting back to this patch one thing I don't understand is how we
> could
> hit this problem in a realistic scenario. Directories names are only
> two chars long and are hexidecimals which means a max of 256
> directories it should be impossible to reach the max size before all
> possible directories are created unless the cache size is extremely
> small in which case we are going to have other problems.
> 
> It may just make more sense to generate all possible directories when
> the parent directory is first created.

Oh right the problem is that its empty. Still unless the cache is very
small this seems unlikely. Did you actually come across this problem
somehow?

Another problem I've considered is that we always delete smaller files
than the ones we create and go over the cache size that way although
that seems unlikely also.

Hopefully we can land it soon and get some real world testing. 

> 
> 
> 
> 
> 
> 
> > 
> > > So in a Tl;Dr; we want to drop the ~5 different codepaths, in
> > > favour
> > > of a single one, used by everyone - devs, testers, and users.
> > > 
> > 
> > Right, I agree this would be very nice. I will still check what 
> > supporting boringssl would mean, at least from quick glance it has
> > quite 
> > different API from others (which is also unstable ..) which did
> > not 
> > encourage me very much last time.
> > 
> > // Tapani
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev