HTMLNames should construct strings at compile time
Created attachment 194104 [details] Patch
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.
CCing dbates for perl ninjatude.
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.
(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.
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?
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.
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.
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?
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.
> (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.
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.
Created attachment 194121 [details] Patch for landing
Do we know what affect on binary size we should expect from this? The perf sheriffs may notice a jump.
This patch increases the binary size of DumpRenderTree by 0.02%.
Created attachment 194125 [details] Patch for landing
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
Created attachment 194134 [details] Patch
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.
(You should feel free to ignore my nits above. I believe you should mostly just commit this thing and iterate at this point.)
> (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.
Comment on attachment 194134 [details] Patch Clearing flags on attachment: 194134 Committed r146419: <http://trac.webkit.org/changeset/146419>
All reviewed patches have been landed. Closing bug.
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
How would one get crash-logs/backtraces from that bot?
(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.
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)
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)
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)
Re-opened since this is blocked by bug 112870
This ASSERT is trying to check that the StringImpl was allocated by malloc, which is of course not true after this patch.
Created attachment 194202 [details] Patch
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.
Created attachment 194209 [details] Forgot AtomicStringImpl
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.
Comment on attachment 194209 [details] Forgot AtomicStringImpl Clearing flags on attachment: 194209 Committed r146464: <http://trac.webkit.org/changeset/146464>
Why remove the ValueCheck instead of checking StrimgImpl::isStatic()? That would have fixed the assertion failure without removing the check.
> 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.