Bug 234350 - Many call sites use makeNeverDestroyed, but could instead take advantage of the NeverDestroyed deduction guide
Summary: Many call sites use makeNeverDestroyed, but could instead take advantage of t...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Darin Adler
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-12-15 09:08 PST by Darin Adler
Modified: 2021-12-16 17:30 PST (History)
38 users (show)

See Also:


Attachments
Patch (158.18 KB, patch)
2021-12-15 12:28 PST, Darin Adler
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (158.18 KB, patch)
2021-12-15 12:44 PST, Darin Adler
no flags Details | Formatted Diff | Diff
Patch (158.25 KB, patch)
2021-12-15 14:41 PST, Darin Adler
no flags Details | Formatted Diff | Diff
Patch (158.42 KB, patch)
2021-12-15 16:43 PST, Darin Adler
sam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>