Bug 237117

Summary: Small optimizations to TinyLRUCache
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: Web Template FrameworkAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, cmarcelo, darin, ews-watchlist, ggaren, sam, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Description Chris Dumez 2022-02-23 15:25:47 PST
Small optimizations to TinyLRUCache.
Comment 1 Chris Dumez 2022-02-23 15:31:24 PST
Created attachment 453035 [details]
Patch
Comment 2 Darin Adler 2022-02-23 17:48:15 PST
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 3 Chris Dumez 2022-02-23 17:58:23 PST
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.
Comment 4 Chris Dumez 2022-02-23 18:06:00 PST
Created attachment 453062 [details]
Patch
Comment 5 EWS 2022-02-24 00:56:50 PST
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].
Comment 6 Radar WebKit Bug Importer 2022-02-24 00:57:21 PST
<rdar://problem/89405387>
Comment 7 Sam Weinig 2022-03-04 17:09:34 PST
(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.
Comment 8 Geoffrey Garen 2022-03-04 17:33:26 PST
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?