[Mesa-dev] [PATCH 1/3] util/disk_cache: use LRU eviction rather than random eviction (v2)

[Alan Swanson]

On Sat, 2017-03-11 at 17:08 +0000, Emil Velikov wrote:
> On 10 March 2017 at 10:51, Grazvydas Ignotas <notasas at gmail.com>
> wrote:
> > 
> > On Fri, Mar 10, 2017 at 6:23 AM, Timothy Arceri <tarceri at itsqueeze.
> > com> wrote:
> > > 
> > > On 07/03/17 12:25, Alan Swanson wrote:
> > > > 
> > > > 
> > > > Still using random selection of two-character subdirectory in
> > > > which
> > > > to check cache files rather than scanning entire cache.
> > > > 
> > > > v2: Factor out double strlen call
> > > > ---
> > > >  src/util/disk_cache.c | 78
> > > > +++++++++++++++++++++++----------------------------
> > > >  1 file changed, 35 insertions(+), 43 deletions(-)
> > > > 
> > > > diff --git a/src/util/disk_cache.c b/src/util/disk_cache.c
> > > > index 31a9336582..2a923be3dc 100644
> > > > --- a/src/util/disk_cache.c
> > > > +++ b/src/util/disk_cache.c

[SNIP]

> > > >     char *filename;
> > > > 
> > > >     dir = opendir(dir_path);
> > > >     if (dir == NULL)
> > > >        return NULL;
> > > > 
> > > > -   count = 0;
> > > > -
> > > > -   while (1) {
> > > > -      entry = readdir(dir);
> > > > -      if (entry == NULL)
> > > > -         break;
> > > > -      if (!predicate(entry, dir_path))
> > > > -         continue;
> > > > -
> > > > -      count++;
> > > > -   }
> > > > -
> > > > -   if (count == 0) {
> > > > -      closedir(dir);
> > > > -      return NULL;
> > > > -   }
> > > > -
> > > > -   victim = rand() % count;
> > > > -
> > > > -   rewinddir(dir);
> > > > -   count = 0;
> > > > -
> > > >     while (1) {
> > > >        entry = readdir(dir);
> > > >        if (entry == NULL)
> > > >           break;
> > > >        if (!predicate(entry, dir_path))
> > > >           continue;
> > > > -      if (count == victim)
> > > > -         break;
> > > > 
> > > > -      count++;
> > > > +      if (fstatat(dirfd(dir), entry->d_name, &sb, 0) == 0) {
> > > > +         if (!lru_atime || (sb.st_atime < lru_atime)) {
> > > > +            len = strlen(entry->d_name) + 1;
> > > > +            tmp = realloc(lru_name, len);
> > > > +            if (tmp) {
> > > > +               lru_name = tmp;
> > > 
> > > 
> > > I don't think we need tmp here. Why not use lru_name directly?
> > 
> > It *is* needed, otherwise if realloc fails, you'll leak old memory.
> > 
> Indeed. Alternatively one can avoid all the strlen/realloc/free
> dances
> by using NAME_MAX.
> 
> As per the POSIX manual
> "The array d_name is of unspecified size, but shall contain a
> filename
> of at most {NAME_MAX} bytes followed by a terminating null byte."
> 
> Not 100% sure if it'll play well with Hurd and similar platforms :-\

But here it's a very simple dance. The realloc should mostly become a
noop, at least with glibc, after the first entry in cache and just
activating for any .tmp files. Using MAX always seems like a lazy over
allocation to me.

A follow on patch will pass the sb and len to predicate functions
instead saving them from calling fstat and strlen on same entry again.
Will post that this weekend when I get back home.

-- 
Alan.