Bug 234350 – Many call sites use makeNeverDestroyed, but could instead take advantage of the NeverDestroyed deduction guide

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Darin Adler]

Many call sites use makeNeverDestroyed, but could instead take advantage of the NeverDestroyed deduction guide

[Darin Adler]

Created attachment 447271 [details]
Patch

[Darin Adler]

Created attachment 447277 [details]
Patch

[Darin Adler]

Created attachment 447287 [details]
Patch

[Darin Adler]

Created attachment 447298 [details]
Patch

[Sam Weinig]

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.

[Darin Adler]

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.

[Darin Adler]

Committed r287138 (?): <https://commits.webkit.org/r287138>

[Radar WebKit Bug Importer]

<rdar://problem/86578814>