Bug 234350

Summary: Many call sites use makeNeverDestroyed, but could instead take advantage of the NeverDestroyed deduction guide
Product: WebKit Reporter: Darin Adler <darin>
Component: WebKit Misc.Assignee: Darin Adler <darin>
Status: RESOLVED FIXED    
Severity: Normal CC: aboxhall, achristensen, andersca, andresg_22, apinheiro, benjamin, calvaris, cdumez, cfleizach, changseok, cmarcelo, dino, dmazzoni, eric.carlson, esprehn+autocc, ews-watchlist, fmalita, glenn, gyuyoung.kim, hta, japhet, jcraig, jdiggs, jer.noble, kangil.han, kondapallykalyan, mifenton, mmaxfield, pdr, philipj, sabouhallawa, samuel_white, sam, schenney, sergio, tommyw, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
See Also: https://bugs.webkit.org/show_bug.cgi?id=234412
Attachments:
Description Flags
Patch
ews-feeder: commit-queue-
Patch
none
Patch
none
Patch sam: review+

Description Darin Adler 2021-12-15 09:08:55 PST
Many call sites use makeNeverDestroyed, but could instead take advantage of the NeverDestroyed deduction guide
Comment 1 Darin Adler 2021-12-15 12:28:00 PST Comment hidden (obsolete)
Comment 2 Darin Adler 2021-12-15 12:44:27 PST Comment hidden (obsolete)
Comment 3 Darin Adler 2021-12-15 14:41:01 PST
Created attachment 447287 [details]
Patch
Comment 4 Darin Adler 2021-12-15 16:43:15 PST
Created attachment 447298 [details]
Patch
Comment 5 Sam Weinig 2021-12-15 17:03:47 PST
Comment on attachment 447287 [details]
Patch

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

r=me, but it should probably build on all the bots first.

> Source/WebCore/Modules/mediasession/MediaSession.cpp:80
> +    static constexpr SortedArrayMap map { mappings };

Nice.

> Source/WebCore/page/DebugPageOverlays.cpp:155
> +    static NeverDestroyed regionColors { MemoryCompactLookupOnlyRobinHoodHashMap<String, SRGBA<uint8_t>> { {

This should really be converted to a SortedArrayMap at some point.

> Source/WebCore/page/PerformanceUserTiming.cpp:46
> +    static NeverDestroyed map = MemoryCompactLookupOnlyRobinHoodHashMap<String, NavigationTimingFunction> {

Can this use a SortedArrayMap as well? (not sure what would happen with the member function pointers).

> Source/WebCore/platform/graphics/FontCascade.cpp:376
> +    static NeverDestroyed map = MemoryCompactLookupOnlyRobinHoodHashSet<AtomString> {

I wonder if the AtomicString hashing optimization is actually faster than a SortedArraySet here. (it would be good to have a rough feeling of where the cutoff is).

> Source/WebCore/platform/graphics/avfoundation/cf/MediaPlayerPrivateAVFoundationCF.cpp:1767
> +    static NeverDestroyed propertyKeyName = [] {

If this is only used on apple platforms (e.g. not on Windows), it might make sense to convert this objective-c and use @[] which is even more constant.

> Source/WebCore/platform/graphics/cocoa/SourceBufferParserWebM.cpp:559
>          HashSet<String, ASCIICaseInsensitiveHash> types;

I think this could be simplified to:

static NeverDestroyed types = HashSet<String, ASCIICaseInsensitiveHash> {
#if ENABLE(VP9)
        "video/webm";
#endif
#if ENABLE(VORBIS) || ENABLE(OPUS)
        "audio/webm";
#endif
    };


or something thereabouts.

That said, the only user of this just iterates the whole set, so it could just be a Span of a constexpr array.
Comment 6 Darin Adler 2021-12-15 17:09:34 PST
Comment on attachment 447287 [details]
Patch

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

>> Source/WebCore/page/DebugPageOverlays.cpp:155
>> +    static NeverDestroyed regionColors { MemoryCompactLookupOnlyRobinHoodHashMap<String, SRGBA<uint8_t>> { {
> 
> This should really be converted to a SortedArrayMap at some point.

I thought to myself: Need to look at every use of MemoryCompactLookupOnlyRobinHoodHashMap, at least, to see if SortedArrayMap is better suited. I worry that the hashing may still be a little faster, so it’s not always an automatic win, but the sorted array will definitely use less memory. Probably will change many of them.

>> Source/WebCore/page/PerformanceUserTiming.cpp:46
>> +    static NeverDestroyed map = MemoryCompactLookupOnlyRobinHoodHashMap<String, NavigationTimingFunction> {
> 
> Can this use a SortedArrayMap as well? (not sure what would happen with the member function pointers).

It absolutely can. Should work fine with those pointers. The reason I didn’t do the conversion was my uncertainty mentioned above.

>> Source/WebCore/platform/graphics/FontCascade.cpp:376
>> +    static NeverDestroyed map = MemoryCompactLookupOnlyRobinHoodHashSet<AtomString> {
> 
> I wonder if the AtomicString hashing optimization is actually faster than a SortedArraySet here. (it would be good to have a rough feeling of where the cutoff is).

Great insight. I would indeed love to use SortedArraySet instead if it’s a win.

>> Source/WebCore/platform/graphics/avfoundation/cf/MediaPlayerPrivateAVFoundationCF.cpp:1767
>> +    static NeverDestroyed propertyKeyName = [] {
> 
> If this is only used on apple platforms (e.g. not on Windows), it might make sense to convert this objective-c and use @[] which is even more constant.

I think it’s used only on Windows.

>> Source/WebCore/platform/graphics/cocoa/SourceBufferParserWebM.cpp:559
>>          HashSet<String, ASCIICaseInsensitiveHash> types;
> 
> I think this could be simplified to:
> 
> static NeverDestroyed types = HashSet<String, ASCIICaseInsensitiveHash> {
> #if ENABLE(VP9)
>         "video/webm";
> #endif
> #if ENABLE(VORBIS) || ENABLE(OPUS)
>         "audio/webm";
> #endif
>     };
> 
> 
> or something thereabouts.
> 
> That said, the only user of this just iterates the whole set, so it could just be a Span of a constexpr array.

Both great points. I will come back and improve this after landing the patch.
Comment 7 Darin Adler 2021-12-16 08:54:27 PST
Committed r287138 (?): <https://commits.webkit.org/r287138>
Comment 8 Radar WebKit Bug Importer 2021-12-16 08:55:20 PST
<rdar://problem/86578814>