Bug 112769 – HTMLNames local names should be thread-safe

• Status: RESOLVED
• Resolution: WONTFIX
• Component: New Bugs

[Adam Barth]

HTMLNames local names should be thread-safe

[Adam Barth]

Created attachment 193965 [details]
work in progress

[Adam Barth]

There's an interesting issue with using these StringImpls on background threads because the'll have their isAtomic bit set but won't be in the background thread's atomic string table.

[Benjamin Poulain]

(In reply to comment #2)
> There's an interesting issue with using these StringImpls on background threads because the'll have their isAtomic bit set but won't be in the background thread's atomic string table.

Isn't that the case already today when you use the StringImpl in the Threaded Parser?

[Benjamin Poulain]

Thinking about it. Wouldn't it be better if isAtomic() is always false on the static strings?

The purpose of isAtomic is for destruction of the StringImpl (and a little shortcut on AtomicString creation). The first is irrelevant for static string, the second would be misleading.

[Eric Seidel (no email)]

Yes.  It does seem that "isAtomic" is really "shouldRemoveFromAtomicStringTableOnDestruction", and thus doesn't apply to static strings.

We could make this explicit with some renamings of the functions.  It's kinda ugly that this is all in StringImpl, but I completely understand why we do it this way. :)

[Adam Barth]

> Isn't that the case already today when you use the StringImpl in the Threaded Parser?

Yes, but that's something I'm considering whether we can improve with this patch.

> Thinking about it. Wouldn't it be better if isAtomic() is always false on the static strings?

That would mean that this operation would be slow:

String foo = HTMLName::divTag;
AtomicString bar = foo;

because we'd need to do a hash lookup.

> The purpose of isAtomic is for destruction of the StringImpl (and a little shortcut on AtomicString creation). The first is irrelevant for static string, the second would be misleading.

That's true as this patch stands currently, but we should consider improving that situation.

> Yes.  It does seem that "isAtomic" is really "shouldRemoveFromAtomicStringTableOnDestruction", and thus doesn't apply to static strings.

That's one thing the isAtomic bit is used for, but as Benjamin points out, there is another use for the bit.

It's not clear to me whether we want static StringImpl objects to be able to be part of multiple AtomicStringTables.  We don't need to resolve this question in this patch, so perhaps we should worry about making that decision in the future.

[Benjamin Poulain]

> It's not clear to me whether we want static StringImpl objects to be able to be part of multiple AtomicStringTables.  We don't need to resolve this question in this patch, so perhaps we should worry about making that decision in the future.

I think we should still figure out a plan. I understand it is not strictly needed for your patch, but it would be good to have clear semantic from the beginning (and enforce it with assertions).

> > Thinking about it. Wouldn't it be better if isAtomic() is always false on the static strings?
> 
> That would mean that this operation would be slow:
> 
> String foo = HTMLName::divTag;
> AtomicString bar = foo;
> 
> because we'd need to do a hash lookup.

To a certain definition of "slow" yes :)

It would be really easy to measure. Once your patch is done, could you check how often is this branch taken for the HTMLNames?

[Benjamin Poulain]

Maybe it is simpler to split this in two?
1) Have the names setup at compile times.
2) Make them static and enforce one of the two options discussed.

[Adam Barth]

Sure.  Breaking this patch into smaller steps it a good idea.

[Adam Barth]

The first step of this patch is in bug 112831.

[Adam Barth]

Created attachment 194148 [details]
work in progress

[Adam Barth]

Created attachment 194153 [details]
works in the Chromium port

[Adam Barth]

There's some more port-specific work to do for this patch.  We need to call WebCore::init early in the initialization process, before we've used any AtomicStrings, to ensure that the static versions of the strings are the ones that end up in the atomic string table.

The good news is that we have an ASSERT that will catch when we screw this up.  The bad news is that I'm not sure where to put the call to WebCore::init in all the ports.  For WebKit2, WebProcess::initializeProcess looks like a tempting location.  For non-WebKit2 ports, I have no clue.

Any help in this regard would be appreciated.

[Adam Barth]

Comment on attachment 194153 [details]
works in the Chromium port

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

> Source/WTF/wtf/text/StringStatics.cpp:87
> +void StringStatics::init()

These probably should move to WebCore since they're specific to the web.

> Source/WebKit/chromium/src/WebKit.cpp:149
>      WTF::initializeThreading();
>      WTF::initializeMainThread();
> -    WTF::AtomicString::init();
> +    WebCore::init();

Maybe I should look for where other ports call initializeThreading?

[Anne van Kesteren]

Threaded HTML parser was removed.