Bug 112831 – HTMLNames should construct strings at compile time

• Component: New Bugs
• Status: RESOLVED FIXED

[Adam Barth]

HTMLNames should construct strings at compile time

[Adam Barth]

Created attachment 194104 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 194104 [details]
Patch

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

Looks good to me.  Could you comment on how much of a startup win this is?

> Source/WebCore/dom/make_names.pl:131
> +    print F "    // Note: These AtomicStrings might end up with different StringImpls,\n";

... if an earlier identical static string was already inserted into the AtomicStringTable...

> Source/WebCore/dom/make_names.pl:716
> +    print F StaticString::GenerateValidateStrings(\%allStrings);

GenerateValidateStrings?  Seems like an odd name.

[Eric Seidel (no email)]

CCing dbates for perl ninjatude.

[Eric Seidel (no email)]

Comment on attachment 194104 [details]
Patch

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

> Source/WebCore/bindings/scripts/StaticString.pm:29
> +sub GenerateStrings

I think the perl ninjas would tell you these should have ($) to let perl know they take one argument.

[Adam Barth]

(In reply to comment #2)
> (From update of attachment 194104 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=194104&action=review
> 
> Looks good to me.  Could you comment on how much of a startup win this is?

Investigating that now.  At this point, however, I'm more excited about this patch as a stepping stone to making these thread-safe.

> > Source/WebCore/dom/make_names.pl:131
> > +    print F "    // Note: These AtomicStrings might end up with different StringImpls,\n";
> 
> ... if an earlier identical static string was already inserted into the AtomicStringTable...

I should probably remove this comment.  It's more relevant for when we make these strings static.

> > Source/WebCore/dom/make_names.pl:716
> > +    print F StaticString::GenerateValidateStrings(\%allStrings);
> 
> GenerateValidateStrings?  Seems like an odd name.

GenerateAsserts?  GenerateStringAsserts?  I'm open to naming suggestions.

> > Source/WebCore/bindings/scripts/StaticString.pm:29
> > +sub GenerateStrings
> 
> I think the perl ninjas would tell you these should have ($) to let perl know they take one argument.

Will do.

[Darin Adler]

Comment on attachment 194104 [details]
Patch

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

Seems good. Anders and Ben were talking about making a version of the hash function that runs at C++ compile time, but given we already have a script in play here, seems good to do it this way.

> Source/WTF/wtf/text/StringImpl.h:777
> +    struct StaticData {

Looks good. I’d probably name this something more like StaticASCIILiteral, to make it clearer why it’s OK to just have m_data and why it’s OK to have initial values that mimic ConstructFromLiteral.

> Source/WTF/wtf/text/StringImpl.h:782
> +        unsigned m_refCount;
> +        unsigned m_length;
> +        const LChar* m_data8;
> +        void* m_buffer;
> +        unsigned m_hashAndFlags;

Normally for a struct we’d leave off the m_ prefixes, but I can see an argument for keeping them here. Might be clearer with a comment stating that these match the data members of the StringImpl class itself.

> Source/WTF/wtf/text/StringImpl.h:791
> +    void assertValidHash()

How about the name assertHasValidHash?

Since these are assertions they don’t need to be inlined, why not put it the code the .cpp file instead of the .h file?

> Source/WTF/wtf/text/StringImpl.h:798
> +private:

I suggest putting a comment here about how this data needs to match the struct above.

> Source/WebCore/dom/QualifiedName.cpp:177
> +    new (reinterpret_cast<void*>(targetAddress)) QualifiedName(nullAtom, AtomicString(name), nameNamespace);

Not new to your patch, but why do we reinterpret_cast a void* to void*?

> Source/WebCore/dom/QualifiedName.h:148
> -void createQualifiedName(void* targetAddress, const char* name, unsigned nameLength);
> -void createQualifiedName(void* targetAddress, const char* name, unsigned nameLength, const AtomicString& nameNamespace);
> +void createQualifiedName(void* targetAddress, StringImpl* name);
> +void createQualifiedName(void* targetAddress, StringImpl* name, const AtomicString& nameNamespace);

Would be nice if it was clearer that these are for some kind of exotic use. Might even want to use the word construct instead of create to emphasize that.

> Source/WebCore/dom/make_names.pl:132
> +    print F "    // Note: These AtomicStrings might end up with different StringImpls,\n";
> +    print F "    // but they will always end up with static StringImpls.\n";

This comment seems a bit enigmatic. I think I’m an expert on AtomicString and I’m not exactly sure what it means or why it’s true. What does it mean to say they always “end up with static impls”? And is the reason they do because of the timing of when this function is called? Or some other reason?

[Adam Barth]

Finished: 10.706664 s
(In reply to comment #6)
> (From update of attachment 194104 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=194104&action=review
> 
> > Source/WebCore/dom/QualifiedName.cpp:177
> > +    new (reinterpret_cast<void*>(targetAddress)) QualifiedName(nullAtom, AtomicString(name), nameNamespace);
> 
> Not new to your patch, but why do we reinterpret_cast a void* to void*?

That's been bugging me too.  I'll fix it in a separate patch.

> > Source/WebCore/dom/make_names.pl:132
> > +    print F "    // Note: These AtomicStrings might end up with different StringImpls,\n";
> > +    print F "    // but they will always end up with static StringImpls.\n";
> 
> This comment seems a bit enigmatic. I think I’m an expert on AtomicString and I’m not exactly sure what it means or why it’s true. What does it mean to say they always “end up with static impls”? And is the reason they do because of the timing of when this function is called? Or some other reason?

I should have removed this comment.  It's relevant for the next patch when we mark these StringImpls as static and make them safe to use them from multiple threads.

[Darin Adler]

Comment on attachment 194104 [details]
Patch

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

>> Source/WTF/wtf/text/StringImpl.h:791
>> +    void assertValidHash()
> 
> How about the name assertHasValidHash?
> 
> Since these are assertions they don’t need to be inlined, why not put it the code the .cpp file instead of the .h file?

Or maybe assertHashIsValid or assertHashIsCorrect.

[Darin Adler]

Comment on attachment 194104 [details]
Patch

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

> Source/WebCore/WebCore.gyp/WebCore.gyp:750
> +            '../bindings/scripts/Hasher.pm',
> +            '../bindings/scripts/StaticString.pm',

Do other build systems need changes like this, or is this something only relevant for gyp?

[Darin Adler]

Comment on attachment 194104 [details]
Patch

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

>> Source/WebCore/WebCore.gyp/WebCore.gyp:750
>> +            '../bindings/scripts/Hasher.pm',
>> +            '../bindings/scripts/StaticString.pm',
> 
> Do other build systems need changes like this, or is this something only relevant for gyp?

Maybe DerivedSources.make needs a similar change.

[Adam Barth]

> (From update of attachment 194104 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=194104&action=review
> 
> > Source/WebCore/WebCore.gyp/WebCore.gyp:750
> > +            '../bindings/scripts/Hasher.pm',
> > +            '../bindings/scripts/StaticString.pm',
>
> Do other build systems need changes like this, or is this something only relevant for gyp?
>
> Maybe DerivedSources.make needs a similar change.

I'll investigate before landing.

[Adam Barth]

This patch is a 15% improvement to my hacky start-up benchmark.  tc_malloc itself drops from 1.95% of startup time to 1.74%.  There are 43% fewer calls to WTF::fastMalloc after the patch.

A large fraction of the remaining calls to WTF::fastMalloc are coming from EventNames, which could probably benefit from a similar optimization.

[Adam Barth]

Created attachment 194121 [details]
Patch for landing

[Eric Seidel (no email)]

Do we know what affect on binary size we should expect from this?  The perf sheriffs may notice a jump.

[Adam Barth]

This patch increases the binary size of DumpRenderTree by 0.02%.

[Adam Barth]

Created attachment 194125 [details]
Patch for landing

[Build Bot]

Comment on attachment 194125 [details]
Patch for landing

Attachment 194125 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17187662

[Adam Barth]

Created attachment 194134 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 194134 [details]
Patch

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

> Source/WebCore/DerivedSources.make:829
> +WebKitFontFamilyNames.cpp WebKitFontFamilyNames.h : dom/make_names.pl bindings/scripts/Hasher.pm bindings/scripts/StaticString.pm css/WebKitFontFamilyNames.in

If we were really fancy we would use a variable in Make for these 3 files.  Or maybe a rule?  basically this depends on make_names, but make_names depends on these other two files.  I do not speak Make well enough to do such craziness however (and don't expect you to).

> Source/WebCore/dom/make_names.pl:224
> +sub valueForName

Same ($) nit as before.

[Eric Seidel (no email)]

(You should feel free to ignore my nits above.  I believe you should mostly just commit this thing and iterate at this point.)

[Adam Barth]

> (You should feel free to ignore my nits above.  I believe you should mostly just commit this thing and iterate at this point.)

If it's ok with you, I'd like to address these issues in a followup patch.  /me is slightly discouraged fighting with N build systems.

[WebKit Review Bot]

Comment on attachment 194134 [details]
Patch

Clearing flags on attachment: 194134

Committed r146419: <http://trac.webkit.org/changeset/146419>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

It seems like this patch broke Windows Debug tests:
http://build.webkit.org/builders/Apple%20Win%207%20Debug%20%28Tests%29/builds/50581 (146417)
http://build.webkit.org/builders/Apple%20Win%207%20Debug%20%28Tests%29/builds/50582 (146419)

All tests are crashing. Just to verify this, I've tried a clean rebuild of WebKit:
http://build.webkit.org/builders/Apple%20Win%20Debug%20%28Build%29/builds/64494

but this also resulted in crashes:
http://build.webkit.org/builders/Apple%20Win%207%20Debug%20%28Tests%29/builds/50585

[Eric Seidel (no email)]

How would one get crash-logs/backtraces from that bot?

[Ryosuke Niwa]

(In reply to comment #25)
> How would one get crash-logs/backtraces from that bot?

Apparently we can't :( I think this used to work but got broken at some point in the past. Anyhow, I've asked Roger to generate stack trace.

[Ryosuke Niwa]

This also broke Mac debug testers:
http://build.webkit.org/builders/Apple%20MountainLion%20Debug%20WK1%20%28Tests%29/builds/6554 (r146415)
http://build.webkit.org/builders/Apple%20MountainLion%20Debug%20WK1%20%28Tests%29/builds/6555 (r146419)

[Ryosuke Niwa]

e.g.
http://build.webkit.org/results/Apple%20MountainLion%20Debug%20WK1%20(Tests)/r146419%20(6555)/results.html

Application Specific Information:
CRASHING TEST: canvas/philip/tests/2d.canvas.readonly.html

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.WebCore             	0x000000010bbf211a WTF::ValueCheck<WTF::AtomicStringImpl*>::checkConsistency(WTF::AtomicStringImpl const*) + 106 (ValueCheck.h:45)
1   com.apple.WebCore             	0x000000010bf1ef75 WTF::HashTable<WTF::AtomicStringImpl*, WTF::KeyValuePair<WTF::AtomicStringImpl*, WebCore::Element*>, WTF::KeyValuePairKeyExtractor<WTF::KeyValuePair<WTF::AtomicStringImpl*, WebCore::Element*> >, WTF::PtrHash<WTF::AtomicStringImpl*>, WTF::HashMapValueTraits<WTF::HashTraits<WTF::AtomicStringImpl*>, WTF::HashTraits<WebCore::Element*> >, WTF::HashTraits<WTF::AtomicStringImpl*> >::checkTableConsistencyExceptSize() const + 293 (HashTable.h:1249)
2   com.apple.WebCore             	0x000000010bf1ed89 WTF::HashTable<WTF::AtomicStringImpl*, WTF::KeyValuePair<WTF::AtomicStringImpl*, WebCore::Element*>, WTF::KeyValuePairKeyExtractor<WTF::KeyValuePair<WTF::AtomicStringImpl*, WebCore::Element*> >, WTF::PtrHash<WTF::AtomicStringImpl*>, WTF::HashMapValueTraits<WTF::HashTraits<WTF::AtomicStringImpl*>, WTF::HashTraits<WebCore::Element*> >, WTF::HashTraits<WTF::AtomicStringImpl*> >::checkTableConsistency() const + 25 (HashTable.h:1223)
3   com.apple.WebCore             	0x000000010bf18525 WTF::HashMap<WTF::AtomicStringImpl*, WebCore::Element*, WTF::PtrHash<WTF::AtomicStringImpl*>, WTF::HashTraits<WTF::AtomicStringImpl*>, WTF::HashTraits<WebCore::Element*> >::checkConsistency() const + 21 (HashMap.h:413)
4   com.apple.WebCore             	0x000000010bf18661 WebCore::Element* WebCore::DocumentOrderedMap::get<&(WebCore::keyMatchesId(WTF::AtomicStringImpl*, WebCore::Element*))>(WTF::AtomicStringImpl*, WebCore::TreeScope const*) const + 193 (DocumentOrderedMap.cpp:122)
5   com.apple.WebCore             	0x000000010bf18095 WebCore::DocumentOrderedMap::getElementById(WTF::AtomicStringImpl*, WebCore::TreeScope const*) const + 37 (DocumentOrderedMap.cpp:145)
6   com.apple.WebCore             	0x000000010d518176 WebCore::TreeScope::getElementById(WTF::AtomicString const&) const + 150 (TreeScope.cpp:149)
7   com.apple.WebCore             	0x000000010be9153f WebCore::Document::getElementById(WTF::AtomicString const&) const + 47 (Document.cpp:709)
8   com.apple.WebCore             	0x000000010c6fde5a WebCore::jsDocumentPrototypeFunctionGetElementById(JSC::ExecState*) + 666 (JSDocument.cpp:2431)
9   ???                           	0x0000204460c01045 0 + 35478053064773
10  com.apple.JavaScriptCore      	0x000000010a381254 JSC::JITCode::execute(JSC::JSStack*, JSC::ExecState*, JSC::JSGlobalData*) + 84 (JITCode.h:135)
11  com.apple.JavaScriptCore      	0x000000010a37e3ff JSC::Interpreter::executeCall(JSC::ExecState*, JSC::JSObject*, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) + 1519 (Interpreter.cpp:1059)
12  com.apple.JavaScriptCore      	0x000000010a185922 JSC::call(JSC::ExecState*, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) + 306 (CallData.cpp:40)
13  com.apple.WebCore             	0x000000010c66e0d2 WebCore::JSMainThreadExecState::call(JSC::ExecState*, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) + 146 (JSMainThreadExecState.h:56)
14  com.apple.WebCore             	0x000000010c7bef46 WebCore::JSEventListener::handleEvent(WebCore::ScriptExecutionContext*, WebCore::Event*) + 1238 (JSEventListener.cpp:129)
15  com.apple.WebCore             	0x000000010c10c712 WebCore::EventTarget::fireEventListeners(WebCore::Event*, WebCore::EventTargetData*, WTF::Vector<WebCore::RegisteredEventListener, 1ul>&) + 498 (EventTarget.cpp:258)
16  com.apple.WebCore             	0x000000010c10c32c WebCore::EventTarget::fireEventListeners(WebCore::Event*) + 380 (EventTarget.cpp:203)
17  com.apple.WebCore             	0x000000010c04b880 WebCore::DOMWindow::dispatchEvent(WTF::PassRefPtr<WebCore::Event>, WTF::PassRefPtr<WebCore::EventTarget>) + 272 (DOMWindow.cpp:1714)
18  com.apple.WebCore             	0x000000010c052958 WebCore::DOMWindow::dispatchLoadEvent() + 296 (DOMWindow.cpp:1688)
19  com.apple.WebCore             	0x000000010be99cdf WebCore::Document::dispatchWindowLoadEvent() + 143 (Document.cpp:3703)
20  com.apple.WebCore             	0x000000010be97732 WebCore::Document::implicitClose() + 466 (Document.cpp:2459)
21  com.apple.WebCore             	0x000000010c1dbf9b WebCore::FrameLoader::checkCallImplicitClose() + 155 (FrameLoader.cpp:838)
22  com.apple.WebCore             	0x000000010c1dbc46 WebCore::FrameLoader::checkCompleted() + 358 (FrameLoader.cpp:782)
23  com.apple.WebCore             	0x000000010c1dbe05 WebCore::FrameLoader::loadDone() + 21 (FrameLoader.cpp:727)
24  com.apple.WebCore             	0x000000010bbb9532 WebCore::CachedResourceLoader::loadDone(WebCore::CachedResource*) + 114 (CachedResourceLoader.cpp:759)
25  com.apple.WebCore             	0x000000010d355a2f WebCore::SubresourceLoader::releaseResources() + 191 (SubresourceLoader.cpp:335)
26  com.apple.WebCore             	0x000000010d11c1a9 WebCore::ResourceLoader::didFinishLoading(double) + 73 (ResourceLoader.cpp:347)
27  com.apple.WebCore             	0x000000010d355629 WebCore::SubresourceLoader::didFinishLoading(double) + 569 (SubresourceLoader.cpp:292)
28  com.apple.WebCore             	0x000000010d11cad5 WebCore::ResourceLoader::didFinishLoading(WebCore::ResourceHandle*, double) + 53 (ResourceLoader.cpp:501)
29  com.apple.WebCore             	0x000000010d1191aa -[WebCoreResourceHandleAsDelegate connectionDidFinishLoading:] + 186 (ResourceHandleMac.mm:798)
30  com.apple.Foundation          	0x00007fff9005d528 __65-[NSURLConnectionInternal _withConnectionAndDelegate:onlyActive:]_block_invoke_0 + 28
31  com.apple.Foundation          	0x00007fff9005d46c -[NSURLConnectionInternal _withConnectionAndDelegate:onlyActive:] + 227
32  com.apple.Foundation          	0x00007fff9005d368 -[NSURLConnectionInternal _withActiveConnectionAndDelegate:] + 63
33  com.apple.CFNetwork           	0x00007fff8e6f45c1 ___delegate_didFinishLoading_block_invoke_0 + 40
34  com.apple.CFNetwork           	0x00007fff8e6e6a7a ___withDelegateAsync_block_invoke_0 + 90
35  com.apple.CFNetwork           	0x00007fff8e7772ea __block_global_1 + 28
36  com.apple.CoreFoundation      	0x00007fff95ba9154 CFArrayApplyFunction + 68
37  com.apple.CFNetwork           	0x00007fff8e6d77e4 RunloopBlockContext::perform() + 124
38  com.apple.CFNetwork           	0x00007fff8e6d76bb MultiplexerSource::perform() + 221
39  com.apple.CoreFoundation      	0x00007fff95b8ab31 __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 17
40  com.apple.CoreFoundation      	0x00007fff95b8a455 __CFRunLoopDoSources0 + 245
41  com.apple.CoreFoundation      	0x00007fff95bad7f5 __CFRunLoopRun + 789
42  com.apple.CoreFoundation      	0x00007fff95bad0e2 CFRunLoopRunSpecific + 290
43  com.apple.Foundation          	0x00007fff900daf5e -[NSRunLoop(NSRunLoop) runMode:beforeDate:] + 268
44  DumpRenderTree                	0x0000000109f8f9f9 runTest(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) + 5017 (DumpRenderTree.mm:1372)
45  DumpRenderTree                	0x0000000109f8e5ea runTestingServerLoop() + 282 (DumpRenderTree.mm:832)
46  DumpRenderTree                	0x0000000109f8dfe5 dumpRenderTree(int, char const**) + 405 (DumpRenderTree.mm:887)

[Ryosuke Niwa]

This also broke Chromium debug testers. e.g.
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac10.6%20%28dbg%29/builds/3309 (r146413)
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac10.6%20%28dbg%29/builds/3310 (r146426)

http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20%28dbg%29/builds/7326 (r146410)
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20%28dbg%29/builds/7327 (r146419)

[WebKit Review Bot]

Re-opened since this is blocked by bug 112870

[Adam Barth]

This ASSERT is trying to check that the StringImpl was allocated by malloc, which is of course not true after this patch.

[Eric Seidel (no email)]

Created attachment 194202 [details]
Patch

[Eric Seidel (no email)]

With the discussion in bug 112873 still going.  I've posted a patch which just disables this ValueCheck assert for StringImpl, since it's no longer valid.

[Eric Seidel (no email)]

Created attachment 194209 [details]
Forgot AtomicStringImpl

[Eric Seidel (no email)]

I expect this patch to resolve the Debug crashes.

% run-webkit-tests --debug canvas/philip/tests/2d.canvas.readonly.html
Test configuration: <mountainlion, x86_64, debug>
Using Debug build
Found 1 test; running 1, skipping 0.
The test ran as expected.

[WebKit Review Bot]

Comment on attachment 194209 [details]
Forgot AtomicStringImpl

Clearing flags on attachment: 194209

Committed r146464: <http://trac.webkit.org/changeset/146464>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Maciej Stachowiak]

Why remove the ValueCheck instead of checking StrimgImpl::isStatic()? That would have fixed the assertion failure without removing the check.

[Adam Barth]

> Why remove the ValueCheck instead of checking StrimgImpl::isStatic()? That would have fixed the assertion failure without removing the check.

These strings won't be static (in the sense of isStatic) until bug 112769 lands.