Bug 211223 – Some HTML element critical paths include AtomString materialization

• Component: DOM
• Status: RESOLVED FIXED

[Yusuke Suzuki]

We should use `static NeverDestroyed<AtomString>`.

[Yusuke Suzuki]

Created attachment 398107 [details]
Patch

[Yusuke Suzuki]

Created attachment 398108 [details]
Patch

[Saam Barati]

Comment on attachment 398108 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        `static NeverDestroyed<const AtomString> ...("...", AtomString::ConstructFromLiteral)`. Since HTML is in the main thread, we can just use `static NeverDestroyed<>`.

can you add asserts everywhere you did this that we're on the main thread?

[Saam Barati]

I spoke to Yusuke and this is done everywhere.

My observation is we should have asserts in all those places.

But this patch doesn't need to be the thing adding them.

[Yusuke Suzuki]

(In reply to Saam Barati from comment #4)
> I spoke to Yusuke and this is done everywhere.
> 
> My observation is we should have asserts in all those places.
> 
> But this patch doesn't need to be the thing adding them.

I think we should introduce `MainThreadNeverDestroyed<const AtomString>` and insert `ASSERT(isMainThread());` in get accessor of that class. And replace existing `NeverDestroyed<const AtomString>` with that. But I think this should be done separately given that we already have so many things like that and this patch's goal is not fixing / finding an existing bug with a new abstraction.

[Yusuke Suzuki]

(In reply to Yusuke Suzuki from comment #5)
> (In reply to Saam Barati from comment #4)
> > I spoke to Yusuke and this is done everywhere.
> > 
> > My observation is we should have asserts in all those places.
> > 
> > But this patch doesn't need to be the thing adding them.
> 
> I think we should introduce `MainThreadNeverDestroyed<const AtomString>` and
> insert `ASSERT(isMainThread());` in get accessor of that class. And replace
> existing `NeverDestroyed<const AtomString>` with that. But I think this
> should be done separately given that we already have so many things like
> that and this patch's goal is not fixing / finding an existing bug with a
> new abstraction.

https://bugs.webkit.org/show_bug.cgi?id=211264

[Yusuke Suzuki]

Inserted `ASSERT(isMainThread())` for newly added NeverDestroyed<>. I postpone `isMainThread()` assertion check in the existing places in https://bugs.webkit.org/show_bug.cgi?id=211264.

[Yusuke Suzuki]

Created attachment 398119 [details]
Patch

Patch for landing

[Yusuke Suzuki]

Comment on attachment 398119 [details]
Patch

OK, EWS is green.

[EWS]

Committed r260977: <https://trac.webkit.org/changeset/260977>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 398119 [details].

[Radar WebKit Bug Importer]

<rdar://problem/62691779>

[Darin Adler]

Comment on attachment 398108 [details]
Patch

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

>> Source/WebCore/ChangeLog:10
>> +        `static NeverDestroyed<const AtomString> ...("...", AtomString::ConstructFromLiteral)`. Since HTML is in the main thread, we can just use `static NeverDestroyed<>`.
> 
> can you add asserts everywhere you did this that we're on the main thread?

If so, please don’t write each one out. Make a class that does it! Even one specific to AtomString would be OK with me.

[Darin Adler]

Comment on attachment 398119 [details]
Patch

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

> Source/WebCore/html/SearchInputType.cpp:70
> +    ASSERT(isMainThread());

Adding all these asserts like this doesn’t seem good. Tons of DOM code all assumes it’s on the main thread. Adding these assertions everywhere we use an AtomString like this is inelegant and inconsistent with WebKit coding style. The DOM code could be peppered with ASSERT(isMainThread()); that assertion is true/needed throughout.

If we really want these, I suggest we make a class MainThreadAtomStringLiteral:

    static MainThreadAtomStringLiteral webkitSearchResultsButtonName("-webkit-search-results-button");

The assertions would go in the "get" function as well as the construction code.

[Yusuke Suzuki]

Comment on attachment 398119 [details]
Patch

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

>> Source/WebCore/html/SearchInputType.cpp:70
>> +    ASSERT(isMainThread());
> 
> Adding all these asserts like this doesn’t seem good. Tons of DOM code all assumes it’s on the main thread. Adding these assertions everywhere we use an AtomString like this is inelegant and inconsistent with WebKit coding style. The DOM code could be peppered with ASSERT(isMainThread()); that assertion is true/needed throughout.
> 
> If we really want these, I suggest we make a class MainThreadAtomStringLiteral:
> 
>     static MainThreadAtomStringLiteral webkitSearchResultsButtonName("-webkit-search-results-button");
> 
> The assertions would go in the "get" function as well as the construction code.

I already landed MainThreadNeverDestroyed :) https://trac.webkit.org/changeset/261013/webkit

See https://bugs.webkit.org/show_bug.cgi?id=211223#c7 and https://bugs.webkit.org/show_bug.cgi?id=211264

[Yusuke Suzuki]

Comment on attachment 398119 [details]
Patch

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

>>> Source/WebCore/html/SearchInputType.cpp:70
>>> +    ASSERT(isMainThread());
>> 
>> Adding all these asserts like this doesn’t seem good. Tons of DOM code all assumes it’s on the main thread. Adding these assertions everywhere we use an AtomString like this is inelegant and inconsistent with WebKit coding style. The DOM code could be peppered with ASSERT(isMainThread()); that assertion is true/needed throughout.
>> 
>> If we really want these, I suggest we make a class MainThreadAtomStringLiteral:
>> 
>>     static MainThreadAtomStringLiteral webkitSearchResultsButtonName("-webkit-search-results-button");
>> 
>> The assertions would go in the "get" function as well as the construction code.
> 
> I already landed MainThreadNeverDestroyed :) https://trac.webkit.org/changeset/261013/webkit
> 
> See https://bugs.webkit.org/show_bug.cgi?id=211223#c7 and https://bugs.webkit.org/show_bug.cgi?id=211264

And that patch actually found one tricky usage in GTK/WPE ports https://trac.webkit.org/changeset/261075/webkit

[Darin Adler]

(In reply to Yusuke Suzuki from comment #14)
> I already landed MainThreadNeverDestroyed :)
> https://trac.webkit.org/changeset/261013/webkit

Could you use that here instead of adding all these assertions?

[Yusuke Suzuki]

(In reply to Darin Adler from comment #16)
> (In reply to Yusuke Suzuki from comment #14)
> > I already landed MainThreadNeverDestroyed :)
> > https://trac.webkit.org/changeset/261013/webkit
> 
> Could you use that here instead of adding all these assertions?

The patch here is already landed. And https://trac.webkit.org/changeset/261013/webkit is introduced after that, and it replaces inserted `ASSERT(isMainThread())` with `MainThreadNeverDestroyed<>`. So, it is already done :)

[Darin Adler]

(In reply to Yusuke Suzuki from comment #17)
> it replaces inserted `ASSERT(isMainThread())` with
> `MainThreadNeverDestroyed<>`

Great! Sorry I didn’t understand.

I still think that one specifically for AtomString would be nice to make the code less wordy.