[opensource-dev] Review Request: VWR-24321: Validate textures starting with 00 too.

[Boroondas Gupte]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://codereview.secondlife.com/r/90/#review199
-----------------------------------------------------------

Ship it!


> 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
<http://codereview.secondlife.com/r/90/#comment133>

    This comment tells us the intention of everything in the
    	if (validate)
    conditional code.
    
    Let's see what happens ...



indra/newview/lltexturecache.cpp
<http://codereview.secondlife.com/r/90/#comment134>

    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
<http://codereview.secondlife.com/r/90/#comment135>

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



indra/newview/lltexturecache.cpp
<http://codereview.secondlife.com/r/90/#comment136>

    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).



indra/newview/lltexturecache.cpp
<http://codereview.secondlife.com/r/90/#comment137>

    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.)



indra/newview/lltexturecache.cpp
<http://codereview.secondlife.com/r/90/#comment138>

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



indra/newview/lltexturecache.cpp
<http://codereview.secondlife.com/r/90/#comment140>

    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
<http://codereview.secondlife.com/r/90/#comment141>

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



indra/newview/lltexturecache.cpp
<http://codereview.secondlife.com/r/90/#comment142>

    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
<http://codereview.secondlife.com/r/90/#comment143>

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



indra/newview/lltexturecache.cpp
<http://codereview.secondlife.com/r/90/#comment144>

    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
<http://codereview.secondlife.com/r/90/#comment145>

    ... actually purge marked down entries ...



indra/newview/lltexturecache.cpp
<http://codereview.secondlife.com/r/90/#comment146>

    ... 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 ...)

- Boroondas


On Jan. 14, 2011, 1:02 p.m., Aleric Inglewood wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/90/
> -----------------------------------------------------------
> 
> (Updated Jan. 14, 2011, 1:02 p.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> -------
> 
> Trivial patch, just removes stupidity.
> 
> 
> This addresses bug VWR-24321.
>     http://jira.secondlife.com/browse/VWR-24321
> 
> 
> Diffs
> -----
> 
>   doc/contributions.txt b0bd26c5638a 
>   indra/newview/lltexturecache.cpp b0bd26c5638a 
> 
> Diff: http://codereview.secondlife.com/r/90/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Aleric
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.secondlife.com/pipermail/opensource-dev/attachments/20110119/5ebc7be1/attachment-0001.htm