Bug 237117

Created attachment 453035 [details]
Patch

Comment on attachment 453035 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=453035&action=review

> Source/WTF/ChangeLog:14
> +        - Use Vector::uncheckedAppend() instead of Vector::append() to bypass capacity
> +          checks given that the vector has an inline capacity that is the size of the
> +          cache (vector capacity never grows or shrinks).

Maybe we should us FixedVector instead?

> Source/WTF/wtf/TinyLRUCache.h:56
> +                Entry entry = WTFMove(m_cache[index]);

I’d use auto here.

Comment on attachment 453035 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=453035&action=review

>> Source/WTF/ChangeLog:14
>> +          cache (vector capacity never grows or shrinks).
> 
> Maybe we should us FixedVector instead?

I am not super familiar with FixedVector but doesn't it have a fixed size? Here the vector size is dynamic but the vector *capacity* is static.
I guess we could use a vector of fixed size (FixedVector) but then we'd need to fill the vector with "empty" values, distinguishable from actually cache values?

>> Source/WTF/wtf/TinyLRUCache.h:56
>> +                Entry entry = WTFMove(m_cache[index]);
> 
> I’d use auto here.

OK.

Created attachment 453062 [details]
Patch

Committed r290415 (247723@main): <https://commits.webkit.org/247723@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 453062 [details].

<rdar://problem/89405387>

(In reply to Chris Dumez from comment #3)
> Comment on attachment 453035 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=453035&action=review
> 
> >> Source/WTF/ChangeLog:14
> >> +          cache (vector capacity never grows or shrinks).
> > 
> > Maybe we should us FixedVector instead?
> 
> I am not super familiar with FixedVector but doesn't it have a fixed size?
> Here the vector size is dynamic but the vector *capacity* is static.
> I guess we could use a vector of fixed size (FixedVector) but then we'd need
> to fill the vector with "empty" values, distinguishable from actually cache
> values?
> 
> >> Source/WTF/wtf/TinyLRUCache.h:56
> >> +                Entry entry = WTFMove(m_cache[index]);
> > 
> > I’d use auto here.
> 
> OK.

Given the name (Tiny...) I think we can probably just a fixed size array for this. Im don't think we ever use this for something with more than 32 elements and we know the max capacity at compile time.

std::array or whatever would be fine. Just a bother to have to implement your own size(), remove(), and append() functions. Maybe not worth the bother?