Bug 174348 - Improve use of NeverDestroyed
Summary: Improve use of NeverDestroyed
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Darin Adler
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-07-10 21:57 PDT by Darin Adler
Modified: 2017-07-18 10:05 PDT (History)
6 users (show)

See Also:


Attachments
Patch (338.27 KB, patch)
2017-07-10 21:58 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews112 for mac-elcapitan (1.89 MB, application/zip)
2017-07-10 23:33 PDT, Build Bot
no flags Details
Patch (344.41 KB, patch)
2017-07-11 19:16 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
Patch (344.83 KB, patch)
2017-07-11 20:45 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-elcapitan (1.02 MB, application/zip)
2017-07-11 22:03 PDT, Build Bot
no flags Details
Patch (345.18 KB, patch)
2017-07-11 22:07 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
Patch (345.42 KB, patch)
2017-07-14 09:32 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews116 for mac-elcapitan (1.73 MB, application/zip)
2017-07-14 11:17 PDT, Build Bot
no flags Details
Patch (345.36 KB, patch)
2017-07-15 09:22 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
Patch (345.46 KB, patch)
2017-07-15 09:57 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
Patch (345.38 KB, patch)
2017-07-15 10:20 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
Patch (348.55 KB, patch)
2017-07-15 11:12 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
Patch (348.46 KB, patch)
2017-07-15 19:52 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
Patch (369.70 KB, patch)
2017-07-15 23:20 PDT, Darin Adler
sam: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews114 for mac-elcapitan (1.80 MB, application/zip)
2017-07-16 01:00 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews126 for ios-simulator-wk2 (1.04 MB, application/zip)
2017-07-16 02:50 PDT, Build Bot
no flags Details
Patch for landing (and to re-run EWS) (370.82 KB, patch)
2017-07-17 09:25 PDT, Darin Adler
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Adler 2017-07-10 21:57:34 PDT Comment hidden (obsolete)
Comment 1 Darin Adler 2017-07-10 21:58:37 PDT Comment hidden (obsolete)
Comment 2 Build Bot 2017-07-10 22:00:34 PDT Comment hidden (obsolete)
Comment 3 Build Bot 2017-07-10 22:02:12 PDT Comment hidden (obsolete)
Comment 4 Build Bot 2017-07-10 23:33:20 PDT Comment hidden (obsolete)
Comment 5 Build Bot 2017-07-10 23:33:21 PDT Comment hidden (obsolete)
Comment 6 Darin Adler 2017-07-11 19:16:57 PDT Comment hidden (obsolete)
Comment 7 Darin Adler 2017-07-11 20:45:22 PDT Comment hidden (obsolete)
Comment 8 Build Bot 2017-07-11 22:03:43 PDT Comment hidden (obsolete)
Comment 9 Build Bot 2017-07-11 22:03:44 PDT Comment hidden (obsolete)
Comment 10 Darin Adler 2017-07-11 22:07:34 PDT Comment hidden (obsolete)
Comment 11 Darin Adler 2017-07-14 09:32:28 PDT Comment hidden (obsolete)
Comment 12 Build Bot 2017-07-14 11:17:33 PDT Comment hidden (obsolete)
Comment 13 Build Bot 2017-07-14 11:17:34 PDT Comment hidden (obsolete)
Comment 14 Darin Adler 2017-07-15 09:22:42 PDT Comment hidden (obsolete)
Comment 15 Darin Adler 2017-07-15 09:57:12 PDT Comment hidden (obsolete)
Comment 16 Darin Adler 2017-07-15 10:20:20 PDT Comment hidden (obsolete)
Comment 17 Darin Adler 2017-07-15 11:12:17 PDT Comment hidden (obsolete)
Comment 18 Darin Adler 2017-07-15 19:52:08 PDT Comment hidden (obsolete)
Comment 19 Darin Adler 2017-07-15 23:20:42 PDT Comment hidden (obsolete)
Comment 20 Darin Adler 2017-07-15 23:21:37 PDT Comment hidden (obsolete)
Comment 21 Build Bot 2017-07-16 01:00:05 PDT
Comment on attachment 315595 [details]
Patch

Attachment 315595 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4129383

New failing tests:
security/contentSecurityPolicy/video-with-data-url-allowed-by-media-src-star.html
storage/indexeddb/modern/new-database-after-user-delete.html
Comment 22 Build Bot 2017-07-16 01:00:06 PDT Comment hidden (obsolete)
Comment 23 Build Bot 2017-07-16 02:50:53 PDT
Comment on attachment 315595 [details]
Patch

Attachment 315595 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4129732

New failing tests:
http/tests/canvas/canvas-tainted-after-draw-image.html
Comment 24 Build Bot 2017-07-16 02:50:54 PDT Comment hidden (obsolete)
Comment 25 Sam Weinig 2017-07-16 12:31:01 PDT
Comment on attachment 315595 [details]
Patch

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

Awesome! My biggest takeaway is that we have a lot of static maps / sets / vectors that never change and that perhaps we should have specialized data structures for them. And, it reaffirmed my curiosity on determining the right size of a list deserves what data structure (e.g. does a list of 4 items really need to be in a HashSet, or would lookups in Vector be just as fast).

> Source/WTF/wtf/NeverDestroyed.h:76
> +template<typename T> NeverDestroyed<T> makeNeverDestroyed(T&&);

I'm not sure I see the point of this forward declaration.

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2868
> +    for (auto& expectedName : names) {
> +        if (*expectedName == name)
> +            return true;
> +    }
> +    return false;

This could be written as:

 std::any_of(names.begin(), names.end(), [&name] (auto& expectedName){ return *expectedName == name; });

Not sure it's better.

If this idiom is common enough, maybe we should an overload of Vector::contains that takes a predicate. (It kind of already exists in the form of Vector::findMatching).

> Source/WebCore/editing/EditingStyle.cpp:901
> +static const Vector<const HTMLElementEquivalent*>& htmlElementEquivalents()

Maybe we need a std::array_ref (http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2012/n3334.html) for things like this. Or perhaps std::array could do it.

> Source/WebCore/loader/CrossOriginAccessControl.cpp:70
>  bool isOnAccessControlResponseHeaderWhitelist(const String& name)
>  {
> -    static std::once_flag onceFlag;
> -    static LazyNeverDestroyed<HTTPHeaderSet> allowedCrossOriginResponseHeaders;
> -
> -    std::call_once(onceFlag, []{
> -        allowedCrossOriginResponseHeaders.construct<std::initializer_list<String>>({
> -            "cache-control",
> -            "content-language",
> -            "content-type",
> -            "expires",
> -            "last-modified",
> -            "pragma"
> -        });
> +    static const auto allowedCrossOriginResponseHeaders = makeNeverDestroyed(HTTPHeaderSet {
> +        "cache-control",
> +        "content-language",
> +        "content-type",
> +        "expires",
> +        "last-modified",
> +        "pragma"
>      });

This function seems to be unused. So you can just remove the whole thing.

> Source/WebCore/page/DebugPageOverlays.cpp:165
> +        HashMap<String, Color> map;
> +        for (auto& entry : entries)
> +            map.add(entry.name, Color { entry.r, entry.g, entry.b, 80 });
> +        return map;

Do initializer_lists for HashMaps like this not work? Something like:

map = { { "touchstart", Color(191, 191, 63, 80) }, ... }?

Would be nice if we could get that to work if it doesn't.

Also, elsewhere you do things like:

map.add(ASCIILiteral {entry.name }, ...). Might be good to be consistent.

> Source/WebCore/platform/MIMETypeRegistry.cpp:385
> +        return { 1, type };

While the terseness is nice, I find this a very confusing construction. That might be because I find the Vector constructor that it calls confusing.  Perhaps something like { { type } } (if that compiles)?

> Source/WebCore/platform/SchemeRegistry.cpp:137
> +    static const auto schemes = makeNeverDestroyedStringVector({

Why not just makeNeverDestroyed(Vector<String> { ? Why the extra function here?

> Source/WebCore/platform/SchemeRegistry.cpp:151
> +    static auto secureSchemes = makeNeverDestroyedSchemeSet(builtinSecureSchemes);

Seems like this could be build with an initialize_list as well.

> Source/WebCore/platform/graphics/FontCascade.cpp:497
> +    static const auto map = makeNeverDestroyed([] {
> +        static const char* const fontFamiliesWithInvalidCharWidth[] = {

Can this just us an initializer_list directly?

> Source/WebCore/rendering/RenderThemeMac.mm:115
> +@interface NSTextFieldCell ()
> +- (CFDictionaryRef)_coreUIDrawOptionsWithFrame:(NSRect)cellFrame inView:(NSView *)controlView includeFocus:(BOOL)includeFocus;
> +@end

This, a others in this file, should have a FIXME about moving it to one of the ..SPI.h files.

> Source/WebCore/rendering/svg/RenderSVGResource.cpp:178
> +            static NeverDestroyed<HashSet<SVGElement*>> invalidatingDependencies;

Ugh, this still bothers me that this is static.  Maybe it could live off renderer.document().accessSVGExtensions()? Clearly moving it down here is an improvement, as it shows much more clearly that the element is both added and removed.

> Source/WebCore/svg/SVGPatternElement.cpp:103
> +            SVGNames::patternUnitsAttr, SVGNames::patternContentUnitsAttr, SVGNames::patternTransformAttr,
> +            SVGNames::xAttr, SVGNames::yAttr, SVGNames::widthAttr, SVGNames::heightAttr,

would put these each on their own line.
Comment 26 Darin Adler 2017-07-16 17:18:01 PDT
(In reply to Sam Weinig from comment #25)
> My biggest takeaway is that we have a lot of static maps / sets /
> vectors that never change and that perhaps we should have specialized data
> structures for them. And, it reaffirmed my curiosity on determining the
> right size of a list deserves what data structure (e.g. does a list of 4
> items really need to be in a HashSet, or would lookups in Vector be just as
> fast).

Yes, eventually I think we might want to use linear search for some cases, binary search for others, and hash tables for others.
Comment 27 Darin Adler 2017-07-16 17:19:00 PDT
I don’t yet understand the test failure.
Comment 28 Darin Adler 2017-07-16 17:50:06 PDT
Comment on attachment 315595 [details]
Patch

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

Still unsure about why there are test failures on EWS!

>> Source/WTF/wtf/NeverDestroyed.h:76
>> +template<typename T> NeverDestroyed<T> makeNeverDestroyed(T&&);
> 
> I'm not sure I see the point of this forward declaration.

I like to define classes and functions, even templates, in a terse way without interleaving implementation. Then have implementations be separate. So I think of this as the interface to makeNeverDestroyed.

I’m not planning to remove this, but I don’t feel strongly. I will remove it if you want me to.

>> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2868
>> +    return false;
> 
> This could be written as:
> 
>  std::any_of(names.begin(), names.end(), [&name] (auto& expectedName){ return *expectedName == name; });
> 
> Not sure it's better.
> 
> If this idiom is common enough, maybe we should an overload of Vector::contains that takes a predicate. (It kind of already exists in the form of Vector::findMatching).

I will use any_of, I think.

>> Source/WebCore/editing/EditingStyle.cpp:901
>> +static const Vector<const HTMLElementEquivalent*>& htmlElementEquivalents()
> 
> Maybe we need a std::array_ref (http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2012/n3334.html) for things like this. Or perhaps std::array could do it.

Sure. Maybe it will be named std::span or std::array_view in the C++ standard some day. Or should we come up with our own? Quite similar to WTF::StringView, I think.

>> Source/WebCore/loader/CrossOriginAccessControl.cpp:70
>>      });
> 
> This function seems to be unused. So you can just remove the whole thing.

Good catch. Done.

>> Source/WebCore/page/DebugPageOverlays.cpp:165
>> +        return map;
> 
> Do initializer_lists for HashMaps like this not work? Something like:
> 
> map = { { "touchstart", Color(191, 191, 63, 80) }, ... }?
> 
> Would be nice if we could get that to work if it doesn't.
> 
> Also, elsewhere you do things like:
> 
> map.add(ASCIILiteral {entry.name }, ...). Might be good to be consistent.

I think an initializer_list for a HashMap will probably work. The reason I wrote it the way I did it was that I was tempted to make a static const array that I was sure the compiler would generate as data with no code, kind of "all POD", and the initializer_list with all the Color in it almost certainly will be code rather than just data. I am not saying I am making the tradeoff correctly.

And, yes, I know I am not consistent about ASCIILiteral. That’s because I don’t have a good understanding of what the tradeoff is. Is having the string inside the StringImpl better for performance than having a pointer that points all the way to the data segment? Does saving a few bytes from a memory allocation add up to enough to be worth it? Since it is simpler to not use ASCIILiteral, it’s always tempting to not use it. I really am unclear about what we should do! I am tempted to just add ASCIILiteral here. But I am not sure.

And then there’s the way these two issues intersect. It would be really ugly to have to write ASCIILiteral on every line, which I suppose is what I’d need if I was using the HashMap with an initializer_list. I suppose we’d want it even in that case. I really wish there was a different type for immortal string literals rather than const char* so I could do all of this with the type system.

>> Source/WebCore/platform/MIMETypeRegistry.cpp:385
>> +        return { 1, type };
> 
> While the terseness is nice, I find this a very confusing construction. That might be because I find the Vector constructor that it calls confusing.  Perhaps something like { { type } } (if that compiles)?

Yes, we should probably eliminate that Vector constructor. Without something to indicate what the  arguments mean, it is not very good.

I will try to see if the two sets of braces workw

>> Source/WebCore/platform/SchemeRegistry.cpp:137
>> +    static const auto schemes = makeNeverDestroyedStringVector({
> 
> Why not just makeNeverDestroyed(Vector<String> { ? Why the extra function here?

I wrote this before I realized I could do that. I will change it.

>> Source/WebCore/platform/SchemeRegistry.cpp:151
>> +    static auto secureSchemes = makeNeverDestroyedSchemeSet(builtinSecureSchemes);
> 
> Seems like this could be build with an initialize_list as well.

There is a Vector<String> here and I don’t know how to turn that into an initializer_list.

You did make a good point earlier about the fact that these say Vector but they should really be std::array_view or whatever. In fact, std::intiializer_list seems a lot like std::array_view.

>> Source/WebCore/platform/graphics/FontCascade.cpp:497
>> +        static const char* const fontFamiliesWithInvalidCharWidth[] = {
> 
> Can this just us an initializer_list directly?

Probably. I will try.

Honestly, I am a little worried that I should be using ConstructFromLiteral here (see all my comments about my ASCIILiteral uncertainty above). If I do decide I need that then I will be sorry if I use an initializer_list; or I suppose I can make a function that takes the initializer_list and does the good thing.

>> Source/WebCore/rendering/RenderThemeMac.mm:115
>> +@end
> 
> This, a others in this file, should have a FIXME about moving it to one of the ..SPI.h files.

Done. The wording I chose:

// FIXME: This should go into an SPI.h file in the spi directory.

>> Source/WebCore/rendering/svg/RenderSVGResource.cpp:178
>> +            static NeverDestroyed<HashSet<SVGElement*>> invalidatingDependencies;
> 
> Ugh, this still bothers me that this is static.  Maybe it could live off renderer.document().accessSVGExtensions()? Clearly moving it down here is an improvement, as it shows much more clearly that the element is both added and removed.

I’d be happy to move this. But maybe not in this patch?

>> Source/WebCore/svg/SVGPatternElement.cpp:103
>> +            SVGNames::xAttr, SVGNames::yAttr, SVGNames::widthAttr, SVGNames::heightAttr,
> 
> would put these each on their own line.

I thought I had grouped them into two themed sets: The pattern ones, then the geometry ones. When I put each on its own line, I usually also just sort instead of trying to use a logical order.

I want to leave this one as is. Not that it’s perfect, but I am not so motivated to change it. What do you think?
Comment 29 Darin Adler 2017-07-17 09:25:43 PDT
Created attachment 315668 [details]
Patch for landing (and to re-run EWS)
Comment 30 Darin Adler 2017-07-17 18:53:40 PDT
Committed r219595: <http://trac.webkit.org/changeset/219595>
Comment 31 Jonathan Bedard 2017-07-18 09:40:35 PDT
This broke some internal builds, fix incoming.
Comment 32 Jonathan Bedard 2017-07-18 10:05:04 PDT
Internal build fixed in <http://trac.webkit.org/changeset/219615>.  Note that this build fix is untested on iOS 10.3.