Bug 174348 – Improve use of NeverDestroyed

• Component: Web Template Framework
• Status: RESOLVED FIXED

[Darin Adler]

Improve use of NeverDestroyed

[Darin Adler]

Created attachment 315081 [details]
Patch

[Build Bot]

Attachment 315081 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 143 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 315081 [details]
Patch

Attachment 315081 [details] did not pass bindings-ews (mac):
Output: http://webkit-queues.webkit.org/results/4098460

New failing tests:
(JS) JSTestCallbackInterface.cpp
(JS) JSTestObj.cpp
(JS) JSTestStandaloneDictionary.cpp
(JS) JSTestStandaloneEnumeration.cpp

[Build Bot]

Comment on attachment 315081 [details]
Patch

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

New failing tests:
storage/indexeddb/modern/new-database-after-user-delete.html

[Build Bot]

Created attachment 315084 [details]
Archive of layout-test-results from ews112 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Darin Adler]

Created attachment 315202 [details]
Patch

[Darin Adler]

Created attachment 315203 [details]
Patch

[Build Bot]

Comment on attachment 315203 [details]
Patch

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

New failing tests:
storage/websql/database-lock-after-reload.html

[Build Bot]

Created attachment 315204 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Darin Adler]

Created attachment 315206 [details]
Patch

[Darin Adler]

Created attachment 315432 [details]
Patch

[Build Bot]

Comment on attachment 315432 [details]
Patch

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

New failing tests:
storage/indexeddb/modern/new-database-after-user-delete.html

[Build Bot]

Created attachment 315460 [details]
Archive of layout-test-results from ews116 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Darin Adler]

Created attachment 315548 [details]
Patch

[Darin Adler]

Created attachment 315551 [details]
Patch

[Darin Adler]

Created attachment 315553 [details]
Patch

[Darin Adler]

Created attachment 315556 [details]
Patch

[Darin Adler]

Created attachment 315580 [details]
Patch

[Darin Adler]

Created attachment 315595 [details]
Patch

[Darin Adler]

Patch is a bit large. I am definitely willing to break it up and land in pieces; please let me know if you review what you think about that.

[Build Bot]

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

[Build Bot]

Created attachment 315597 [details]
Archive of layout-test-results from ews114 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

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

[Build Bot]

Created attachment 315599 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[Sam Weinig]

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.

[Darin Adler]

(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.

[Darin Adler]

I don’t yet understand the test failure.

[Darin Adler]

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?

[Darin Adler]

Created attachment 315668 [details]
Patch for landing (and to re-run EWS)

[Darin Adler]

Committed r219595: <http://trac.webkit.org/changeset/219595>

[Jonathan Bedard]

This broke some internal builds, fix incoming.

[Jonathan Bedard]

Internal build fixed in <http://trac.webkit.org/changeset/219615>.  Note that this build fix is untested on iOS 10.3.