On Tue, 2017-01-10 at 22:41 +1100, Timothy Arceri wrote: > 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@gmail. > > > > co > > > > m> > > > > wrote: > > > > > On 9 December 2016 at 05:58, Tapani Pälli <tapani.palli@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?
Oh there is a bug report. I should have looked at the original email, those max sizes seem a bit ridiculous. > > 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@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev > > > > _______________________________________________ > > mesa-dev mailing list > > mesa-dev@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev > > _______________________________________________ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/mesa-dev _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
