Re: [Mesa-dev] [PATCH 3/7] glsl: Add initial functions to implement an on-disk cache
On Wed, Feb 04 2015, Tapani Pälli wrote: > What would you think about changing this to use some defined maximum > size (in MB)? I think for the user size is what matters and it could be > a configurable option, number of items seems a bit vague and hard to > predict (?) Yes, changing over to a maximum size would provide a nice, configurable knob for the user. I don't see an easy way to get the current "index file" approach to allow for size-based management. But the index file is also the only piece of code that's still got some FIXME comments in it that I'm not sure how to best address. I think I'll call the index file a premature optimization and just pull it out. That will make the initial patch provide a cache of unlimited size (no replacement at all). Then, I'll follow up with a patch that provides some sort of size-based replacement policy. > Will there be some further verification here if the file contents are > what is expected or is this done in higher level where cache is > called? I would expect any verification to take place at a higher level. -Carl pgph1MWfIKFpZ.pgp Description: PGP signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 3/7] glsl: Add initial functions to implement an on-disk cache
Hi, On 02/05/2015 01:31 AM, Kenneth Graunke wrote: On Wednesday, February 04, 2015 01:52:57 PM Carl Worth wrote: From: Kristian Høgsberg This code provides for an on-disk cache of objects. Objects are stored and retrieved (in ~/.cache/mesa) via names that are arbitrary 20-byte sequences, (intended to be SHA-1 hashes of something identifying for the content). The cache is limited to a maximum number of entries (1024 in this patch), and uses random replacement. These attributes are managed via The cache will need to be much larger than 1024 entries - perhaps by an order of magnitude. Order of magnitude isn't enough, even for single application. DOTA2 alone can create >1 shaders [1]. Games that do not use separate shader objects extension can have combinatorial explosion of concatenated shader code pieces... For example, "Shadowrun Returns" uses 1299 shaders, "Left 4 Dead 2" uses 1849 shaders, and "Natural Selection 2" uses 2719 shaders. A single application could overflow the cache :) For what it's worth, the total number of shaders in my game collection is around 17,000. - Eero [1] At least 2013 version of DOTA2 did, depending on what options were selected. Carl, my Apitrace trace "library" has such trace if you're interested. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 3/7] glsl: Add initial functions to implement an on-disk cache
On Thu, Feb 5, 2015 at 1:31 AM, Kenneth Graunke wrote: > On Wednesday, February 04, 2015 01:52:57 PM Carl Worth wrote: > > From: Kristian Høgsberg > > > > This code provides for an on-disk cache of objects. Objects are stored > > and retrieved (in ~/.cache/mesa) via names that are arbitrary 20-byte > > sequences, (intended to be SHA-1 hashes of something identifying for > > the content). > > > > The cache is limited to a maximum number of entries (1024 in this > > patch), and uses random replacement. These attributes are managed via > > Hi Carl, > > The cache will need to be much larger than 1024 entries - perhaps by an > order of magnitude. For example, "Shadowrun Returns" uses 1299 shaders, > "Left 4 Dead 2" uses 1849 shaders, and "Natural Selection 2" uses 2719 > shaders. A single application could overflow the cache :) > Seconded. Many Unity games end up loading between several hundred to several thousand shaders. -- Aras Pranckevičius work: http://unity3d.com home: http://aras-p.info ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 3/7] glsl: Add initial functions to implement an on-disk cache
Hi;
On 02/04/2015 11:52 PM, Carl Worth wrote:
From: Kristian Høgsberg
This code provides for an on-disk cache of objects. Objects are stored
and retrieved (in ~/.cache/mesa) via names that are arbitrary 20-byte
sequences, (intended to be SHA-1 hashes of something identifying for
the content).
The cache is limited to a maximum number of entries (1024 in this
patch), and uses random replacement. These attributes are managed via
What would you think about changing this to use some defined maximum
size (in MB)? I think for the user size is what matters and it could be
a configurable option, number of items seems a bit vague and hard to
predict (?)
8<
+uint8_t *
+cache_get(struct program_cache *cache, cache_key key, size_t *size)
+{
+ int fd, ret, len;
+ struct stat sb;
+ char filename[256], *data;
+
+ if (size)
+ *size = 0;
+
+ if (!cache_has(cache, key))
+ return NULL;
+
+ get_cache_file(cache, filename, sizeof filename, key);
+
+ fd = open(filename, O_RDONLY | O_CLOEXEC);
+ if (fd == -1)
+ return NULL;
+
+ if (fstat(fd, &sb) == -1) {
+ close(fd);
+ return NULL;
+ }
+
+ data = (char *) malloc(sb.st_size);
+ if (data == NULL) {
+ close(fd);
+ return NULL;
+ }
Will there be some further verification here if the file contents are
what is expected or is this done in higher level where cache is called?
// Tapani
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 3/7] glsl: Add initial functions to implement an on-disk cache
On Wed, Feb 4, 2015 at 6:04 PM, Carl Worth wrote: > On Wed, Feb 04 2015, Matt Turner wrote: >> Rebase needed. I removed GLSL_SRCDIR a week and a half ago. > > Was this the removal of all those subdir-objects warnings? Thank you! Yes! Just one warning remaining :). >> I don't think it makes a difference to autotools, but I think I'd list >> cache.h here instead of in LIBGLSL_FILES. > > Actually, that was sort of intentional. The cache.c file is only conditionally > compiled, but cache.h has the inline stub implementations that we want > to compile unconditionally. So it felt right to put the .h in the > unconditional list. > > Of course, maybe that doesn't matter at all. It's the .c files that > include the .h file that are going to result in the inline stubs being > provided. Now that we're on the subject, what does automake even > do with .h files that are listed in these lists? Do they need to be > there for non-srcdir builds to work or something? It's just used to tell the `dist` rule to include it in the tarball. > If it truly doesn't matter, then I would prefer to see the cache.h next > to the cache.c, yes. Yeah, I don't think it matters. I think make itself figures out dependencies on headers. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 3/7] glsl: Add initial functions to implement an on-disk cache
On Wed, Feb 04 2015, Matt Turner wrote: > Rebase needed. I removed GLSL_SRCDIR a week and a half ago. Was this the removal of all those subdir-objects warnings? Thank you! > I don't think it makes a difference to autotools, but I think I'd list > cache.h here instead of in LIBGLSL_FILES. Actually, that was sort of intentional. The cache.c file is only conditionally compiled, but cache.h has the inline stub implementations that we want to compile unconditionally. So it felt right to put the .h in the unconditional list. Of course, maybe that doesn't matter at all. It's the .c files that include the .h file that are going to result in the inline stubs being provided. Now that we're on the subject, what does automake even do with .h files that are listed in these lists? Do they need to be there for non-srcdir builds to work or something? If it truly doesn't matter, then I would prefer to see the cache.h next to the cache.c, yes. >> + cache = (struct program_cache *) malloc(sizeof *cache); > > Don't cast malloc. I'll blame krh on this one, (and he can in turn blame it on bad habits From programming in C++ too much). :-) >> + return (void *) data; > > I don't think you need this cast either? Just removing that cast doesn't quite do the trick because there's a potential signedness difference between char and uint8_t. But the cast is not the way I want to fix this, so thanks for pointing that out. (I have gone back and forth on whether this function should return a uint8_t* or a "void *". There's a later patch in the next series that still expects a direct assignment from cache_get() to some data-structure pointer to work without a cast. So I may change cache_get back to returning a "void *". But even then, the cast above won't be needed.). Thanks for your attention to detail. -Carl pgpaAlNnBcoKs.pgp Description: PGP signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 3/7] glsl: Add initial functions to implement an on-disk cache
On Wed, Feb 04 2015, Kenneth Graunke wrote: > The cache will need to be much larger than 1024 entries - perhaps by an > order of magnitude. Thanks for the feedback. I had meant to add a comment next to that 1024 in the code along the lines of "This value was chosen arbitrarily. An appropriate value will need to be found with testing." > We should respect $XDG_CACHE_HOME from the XDG base directory spec: > http://standards.freedesktop.org/basedir-spec/basedir-spec-latest.html > > Also, the snprintf makes me a bit nervous See patch 5/7. It fixes both of these problems. (My apologies for setting you up to review code that was replaced later in the series---but I'm always a little hesitant to squash things to much when only some of the code is actually mine). > Typo: unnecessary compile (missing 'n'). Also, */ goes on its own > line. Thanks. I've fixed both of these now. -Carl pgpRw1mcWIDW5.pgp Description: PGP signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 3/7] glsl: Add initial functions to implement an on-disk cache
On Wed, Feb 4, 2015 at 1:52 PM, Carl Worth wrote:
> From: Kristian Høgsberg
>
> This code provides for an on-disk cache of objects. Objects are stored
> and retrieved (in ~/.cache/mesa) via names that are arbitrary 20-byte
> sequences, (intended to be SHA-1 hashes of something identifying for
> the content).
>
> The cache is limited to a maximum number of entries (1024 in this
> patch), and uses random replacement. These attributes are managed via
> an index file that is stored in the cache directory and mmapped. This
> file is indexed by the low-order bytes of the cached object's names
> and each entry stores the complete name. So a quick comparison of the
> index entry verifies whether the cache has an item, or whether an
> existing item should be replaced.
>
> Note: Some FIXME comments are still present in this commit. These will
> be addressed in subsequent commits, (and before any of this code gets
> any active use).
> ---
> src/glsl/Makefile.am | 4 +
> src/glsl/Makefile.sources | 3 +
> src/glsl/cache.c | 230
> ++
> 3 files changed, 237 insertions(+)
> create mode 100644 src/glsl/cache.c
>
> diff --git a/src/glsl/Makefile.am b/src/glsl/Makefile.am
> index 01123bc..604af51 100644
> --- a/src/glsl/Makefile.am
> +++ b/src/glsl/Makefile.am
> @@ -137,6 +137,10 @@ libglsl_la_SOURCES =
> \
> $(LIBGLSL_FILES)\
> $(NIR_FILES)
>
> +if ENABLE_SHADER_CACHE
> +libglsl_la_SOURCES += $(LIBGLSL_SHADER_CACHE_FILES)
> +endif
> +
> glsl_compiler_SOURCES = \
> $(top_srcdir)/src/mesa/main/imports.c \
> $(top_srcdir)/src/mesa/program/prog_hash_table.c \
> diff --git a/src/glsl/Makefile.sources b/src/glsl/Makefile.sources
> index 8375f6e..c5b742c 100644
> --- a/src/glsl/Makefile.sources
> +++ b/src/glsl/Makefile.sources
> @@ -179,6 +179,9 @@ LIBGLSL_FILES = \
> $(GLSL_SRCDIR)/s_expression.cpp \
> $(GLSL_SRCDIR)/s_expression.h
>
> +LIBGLSL_SHADER_CACHE_FILES = \
> + $(GLSL_SRCDIR)/cache.c
Rebase needed. I removed GLSL_SRCDIR a week and a half ago.
I don't think it makes a difference to autotools, but I think I'd list
cache.h here instead of in LIBGLSL_FILES.
> +
> # glsl_compiler
>
> GLSL_COMPILER_CXX_FILES = \
> diff --git a/src/glsl/cache.c b/src/glsl/cache.c
> new file mode 100644
> index 000..fd087db
> --- /dev/null
> +++ b/src/glsl/cache.c
> @@ -0,0 +1,230 @@
> +/*
> + * Copyright © 2014 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> DEALINGS
> + * IN THE SOFTWARE.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include "util/mesa-sha1.h"
> +
> +#include "cache.h"
> +
> +#define INDEX_SIZE 1024
> +struct program_cache {
> + unsigned char *index;
> + char path[256];
> +};
> +
> +struct program_cache *
> +cache_create(void)
> +{
> + struct program_cache *cache;
> + char index_file[256], buffer[512];
> + struct stat sb;
> + size_t size;
> + int fd;
> + struct passwd pwd, *result;
> +
> + getpwuid_r(getuid(), &pwd, buffer, sizeof buffer, &result);
> + if (result == NULL)
> + return NULL;
> + snprintf(index_file, sizeof index_file,
> +"%s/.cache/mesa/index", pwd.pw_dir);
> +
> + fd = open(index_file, O_RDWR | O_CREAT | O_CLOEXEC, 0644);
> + if (fd == -1) {
> + /* FIXME: Check for ENOENT and mkdir on demand */
> + return NULL;
> + }
> +
> + if (fstat(fd, &sb) == -1) {
> + close(fd);
> + return NULL;
> + }
> +
> + size = INDEX_SIZE * CACHE_KEY_SIZE;
> + if (sb.st_size == 0) {
> + if (ftruncate(fd, size) == -1) {
> + close(fd);
> + return NULL;
> + }
> + fsync(fd);
> + }
> +
> + cache = (struct program_cache *) malloc(sizeof *cache);
Don't cast m
Re: [Mesa-dev] [PATCH 3/7] glsl: Add initial functions to implement an on-disk cache
On Wednesday, February 04, 2015 01:52:57 PM Carl Worth wrote:
> From: Kristian Høgsberg
>
> This code provides for an on-disk cache of objects. Objects are stored
> and retrieved (in ~/.cache/mesa) via names that are arbitrary 20-byte
> sequences, (intended to be SHA-1 hashes of something identifying for
> the content).
>
> The cache is limited to a maximum number of entries (1024 in this
> patch), and uses random replacement. These attributes are managed via
Hi Carl,
The cache will need to be much larger than 1024 entries - perhaps by an
order of magnitude. For example, "Shadowrun Returns" uses 1299 shaders,
"Left 4 Dead 2" uses 1849 shaders, and "Natural Selection 2" uses 2719
shaders. A single application could overflow the cache :)
For what it's worth, the total number of shaders in my game collection
is around 17,000.
A couple of other comments inline...
> an index file that is stored in the cache directory and mmapped. This
> file is indexed by the low-order bytes of the cached object's names
> and each entry stores the complete name. So a quick comparison of the
> index entry verifies whether the cache has an item, or whether an
> existing item should be replaced.
>
> Note: Some FIXME comments are still present in this commit. These will
> be addressed in subsequent commits, (and before any of this code gets
> any active use).
> ---
> src/glsl/Makefile.am | 4 +
> src/glsl/Makefile.sources | 3 +
> src/glsl/cache.c | 230
> ++
> 3 files changed, 237 insertions(+)
> create mode 100644 src/glsl/cache.c
>
> diff --git a/src/glsl/Makefile.am b/src/glsl/Makefile.am
> index 01123bc..604af51 100644
> --- a/src/glsl/Makefile.am
> +++ b/src/glsl/Makefile.am
> @@ -137,6 +137,10 @@ libglsl_la_SOURCES =
> \
> $(LIBGLSL_FILES)\
> $(NIR_FILES)
>
> +if ENABLE_SHADER_CACHE
> +libglsl_la_SOURCES += $(LIBGLSL_SHADER_CACHE_FILES)
> +endif
> +
> glsl_compiler_SOURCES = \
> $(top_srcdir)/src/mesa/main/imports.c \
> $(top_srcdir)/src/mesa/program/prog_hash_table.c \
> diff --git a/src/glsl/Makefile.sources b/src/glsl/Makefile.sources
> index 8375f6e..c5b742c 100644
> --- a/src/glsl/Makefile.sources
> +++ b/src/glsl/Makefile.sources
> @@ -179,6 +179,9 @@ LIBGLSL_FILES = \
> $(GLSL_SRCDIR)/s_expression.cpp \
> $(GLSL_SRCDIR)/s_expression.h
>
> +LIBGLSL_SHADER_CACHE_FILES = \
> + $(GLSL_SRCDIR)/cache.c
> +
> # glsl_compiler
>
> GLSL_COMPILER_CXX_FILES = \
> diff --git a/src/glsl/cache.c b/src/glsl/cache.c
> new file mode 100644
> index 000..fd087db
> --- /dev/null
> +++ b/src/glsl/cache.c
> @@ -0,0 +1,230 @@
> +/*
> + * Copyright © 2014 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> DEALINGS
> + * IN THE SOFTWARE.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include "util/mesa-sha1.h"
> +
> +#include "cache.h"
> +
> +#define INDEX_SIZE 1024
> +struct program_cache {
> + unsigned char *index;
> + char path[256];
> +};
> +
> +struct program_cache *
> +cache_create(void)
> +{
> + struct program_cache *cache;
> + char index_file[256], buffer[512];
> + struct stat sb;
> + size_t size;
> + int fd;
> + struct passwd pwd, *result;
> +
> + getpwuid_r(getuid(), &pwd, buffer, sizeof buffer, &result);
> + if (result == NULL)
> + return NULL;
> + snprintf(index_file, sizeof index_file,
> +"%s/.cache/mesa/index", pwd.pw_dir);
We should respect $XDG_CACHE_HOME from the XDG base directory spec:
http://standards.freedesktop.org/basedir-spec/basedir-spec-latest.html
Also, the snprintf makes me a bit nervous - we don't know how long the
user's home directory path will be, much less $XDG_CACHE_HOME. snprintf
saves us from overflow
