Correct.

indra/newview/lltexturecache.cpp (Diff revision 1)

validate_idx being used in a test later, it's not just for (validate_idx == 0) that the behavior will be different. I need to understand better what that idx is all about or you need to give a bit more explanation before I approve this diff.

1. Aleric Inglewood Jan. 18, 2011, 2:39 p.m. (Jan. 18, 2011, 2:39 p.m.)

   The debug setting CacheValidateCounter is set to 'next_id', which makes clear what it's meaning is: namely, the id that we will check next time. next_idx is a very local variable that is simply set to the value of CacheValidateCounter plus 1, and then that value is stored to CacheValidateCounter again for next time.
   
   validate_idx is the ID that is actually being tested this time. Hence, it should be the value of CacheValidateCounter before we increase that.
   
   Obviously, those ID's should be in the range 0...255, which is garanteed by doing a modulo 256 on next_id before writing it to CacheValidateCounter.
   
   The old code also increases validate_idx *before* it is used. That means that it will be in the range 1...256, and 0 is never tested (note that validate_idx is just increased, there is no modulo applied, so it's obvious that it shouldn't be increased). Even the wording ("next"_id) suggests that validate_idx should just be the value of CacheValidateCounter, which is the value of the previous next_id...
   
   So, after this patch, we get to the following code with validate_idx in the range 0...255, as it should be.
   

2. Cron Stardust Jan. 18, 2011, 8:51 p.m. (Jan. 18, 2011, 8:51 p.m.)

   Just double checking, as switching from pre-increment to addition can change behavior: In both cases next_idx will have the same value after this line is executed, however validate_idx will not.
   
   Prediff start state: validate_idx == 0;
   Prediff stop state: validate_idx == 1; next_idx == 1;
   
   Postdiff start state: validate_idx == 0;
   Postdiff stop state: validate_idx == 0; next_idx == 1;
   
   And another round over at the other end:
   
   Prediff start state: validate_idx == 255;
   Prediff stop state: validate_idx == 256; next_idx == 0;
   
   Postdiff start state: validate_idx == 255;
   Postdiff stop state: validate_idx == 255; next_idx == 0;
   
   So, yes, validate_idx will only have a 255 in it in this last case, however the over-all behavior has changed: validate_idx isn't being incremented at all in this line.
   
   WARNING: I have NOT looked at the rest of the diff, only at this one line.  Nor do I know if validate_idx should or shouldn't be incremented by this line of code.

3. Twisted Laws Jan. 19, 2011, 10:31 a.m. (Jan. 19, 2011, 10:31 a.m.)

   Given the way this seems to work to me without changing the sequence things are checked...  
   I'd change line 1619 to  if (uuididx == (validate_idx % 256))
   Otherwise it was checking for 1 thru 256 and never 0...  this does not change what appears
   to be incorrect coding where Aleric pointed out and thus won't change the current
   logic that it starts by checking 1 thru 255 before checking 0.  To retain the current sequence
   that things are checked, you would have to compare uuididx to next_idx along with the change
   Aleric provided.
   
   However it seems to me that all is ok with using Aleric's correction and leaving the remaining
   code untouched.  (I can't see where changing the sequence of checking makes a difference.)

4. Boroondas Gupte Jan. 19, 2011, 2:47 p.m. (Jan. 19, 2011, 2:47 p.m.)

   > Just double checking, as switching from pre-increment to addition can change behavior:
   > In both cases next_idx will have the same value after this line is executed, however validate_idx will not.
   
   That's the intention. Without the change suggested here, validate_idx ends up in the wrong range. See my more detailed review below.

5. Aleric Inglewood Jan. 19, 2011, 3:05 p.m. (Jan. 19, 2011, 3:05 p.m.)

   @Cron Stardust : That is correct. The bug was that validate_idx was 1 too large. Nor incrementing it is the correct thing.
   Re the 'WARNING': this line is the only line in the diff :p
   

> validate_idx being used in a test later, it's not just for (validate_idx == 0)
> that the behavior will be different. I need to understand better what that idx
> is all about or you need to give a bit more explanation before I approve this diff.

The different intention is needed. Without the change Aleric suggests, files that have a specific byte of their UUID being 0 will never be validated. On the other hand, sometimes no files will be tested at all (when validate_idx is 256, as no byte can assume that value).

indra/newview/lltexturecache.cpp (Diff revision 1)

This comment tells us the intention of everything in the
	if (validate)
conditional code.

Let's see what happens ...

indra/newview/lltexturecache.cpp (Diff revision 1)

This value will be overwritten below. Has to be declared already here, so it will still be available in the second
	if (validate)
scope.

Here comes the first one:

indra/newview/lltexturecache.cpp (Diff revision 1)

Overwrite validate_idx with value from settings. Will be something within [0,255], as we will see below.

indra/newview/lltexturecache.cpp (Diff revision 1)

Old code:
validate_idx will now be in [1,256]. (one higher than before)

New code:
validate_idx will stay in [0,256]. (untouched)

Old and new code:
next_idx will be in [0,256], but different than the new-code validate_idx (one higher modulo 256).

1. Boroondas Gupte Jan. 19, 2011, 2:52 p.m. (Jan. 19, 2011, 2:52 p.m.)

   ok, tripple checked the ranges, but still typed them wrong ... this should be
   
   New code:
   validate_idx will stay in [0,255]. (untouched)
   
   Old and new code:
   next_idx will be in [0,255], but different than the new-code validate_idx (one higher modulo 256).

indra/newview/lltexturecache.cpp (Diff revision 1)

next_idx (still in the range [0,256]) gets saved to setting. (Thus why the value we get from the setting above must be in that interval.)

1. Boroondas Gupte Jan. 19, 2011, 2:52 p.m. (Jan. 19, 2011, 2:52 p.m.)

   make this [0,255], too ^_^

indra/newview/lltexturecache.cpp (Diff revision 1)

We iterate over a set of pairs. The pairs' second entries are our 'indices'. Current index is available in idx.

indra/newview/lltexturecache.cpp (Diff revision 1)

If the entry has to be purged anyway, because of size constraints, we don't have to validate it.

Otherwise, here's the second
	if (validate)
scope.

indra/newview/lltexturecache.cpp (Diff revision 1)

this actually happens in the following if scope (if the condition is met)

indra/newview/lltexturecache.cpp (Diff revision 1)

Testing each entry each time probably takes too much time. Thus, as the comment in the beginning told us, only a fraction is tested. This is done by just skipping the following validation code for most entries during the for-loop we are currently in.

Should the entry of the current iteration be tested? We decide that by comparing a specific byte of its UUID with validate_idx. Thus, during the loop, all cache entries that have this byte take on the same value as validate_idx will be validated.

For this to actually  be (on average) 1/256th of the files (as said comment claims), this byte must be uniformly distributed (i.e. all values in [0,255] must be equally likely for it). The server probably makes sure of that when assigning UUIDs.

Because a byte takes values in [0,255], validate_idx must take on each of these values over time, so that all files will be validated sooner or later, if the stay around long enough. Thus the original code was wrong and the new one is correct.

That mData[0] is indeed a byte can be seen where its declared. (See https://bitbucket.org/lindenlab/viewer-development/src/40d0806e9800/indra/llcommon/lluuid.h#cl-127 -- It's a U8.)

indra/newview/lltexturecache.cpp (Diff revision 1)

The actual validation. Entries for files that fail it are marked down for purging.

indra/newview/lltexturecache.cpp (Diff revision 1)

If we neither are short of space nor have validated any cache entries, no entries can possibly be marked down for purging, so we can leave early here.

indra/newview/lltexturecache.cpp (Diff revision 1)

... actually purge marked down entries ...

indra/newview/lltexturecache.cpp (Diff revision 1)

... end of the long for loop.

I hope this makes things clear (and that I'm reading the code correctly and not telling any humbug ...)

Thanks for the clarification!  Looks good to me.

Ok, I read the code in more details and I'm indeed convinced now that this is correct. Ship it!