Make Array.join work directly on substrings without reifying them
Created attachment 255318 [details] Patch
OK, Mr. Kling. You asked me to do this, so help advise me on what more I should do before landing it.
Attachment 255318 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/ArrayPrototype.cpp:388: Missing space before { [whitespace/braces] [5] Total errors found: 1 in 9 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 255321 [details] Patch
Attachment 255321 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/ArrayPrototype.cpp:388: Missing space before { [whitespace/braces] [5] Total errors found: 1 in 9 files If any of these errors are false positives, please file a bug against check-webkit-style.
Can you please verify the performance on Peacekeeper's array tests? That code used to be ridiculously hot and we are not leading on that benchmark.
(In reply to comment #6) > Can you please verify the performance on Peacekeeper's array tests? Sure, I’ll do that. Is there an easy way to run just those tests?
Comment on attachment 255321 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=255321&action=review > Source/JavaScriptCore/ChangeLog:9 > + 1) Performance testing to see if this is truly a speedup. Even if there's only little or no speedup from this, there's good value in avoiding the StringImpl reification. For instance, on cnet.com, there is ~400 kB worth of StringImpl getting allocated below the JSString::value() call that you're sidestepping here. > Source/JavaScriptCore/ChangeLog:10 > + 2) Correctness testing for the various missing exception checks. AFAICT they are all OOM exception checks. I don't know how we'd test for those. > Source/JavaScriptCore/ChangeLog:28 > + (JSC::JSValue::toWTFStringSlowCase): MOved the contents of the MOved -> Moved > Source/JavaScriptCore/runtime/JSStringJoiner.h:96 > + m_strings.uncheckedAppend({ { }, { } }); I can't decide if this is cute or ugly. Let's go with cute.
Comment on attachment 255321 [details] Patch OK, I’ll put a new patch up for review with the change log fixed, and after I do some Peacekeeper performance testing (still might need some help on how to run only part of that test; the whole Peacekeeper seems like too much).
Created attachment 255396 [details] Patch
Attachment 255396 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/ArrayPrototype.cpp:382: Missing space before { [whitespace/braces] [5] Total errors found: 1 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.
Ben, there’s a lot more that could be done to make Peacekeeper faster; we should go over that some time.
Comment on attachment 255396 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=255396&action=review > Source/JavaScriptCore/ChangeLog:9 > + Bsides the Array.join change, this has other optimizations based on > + profiling the Peacekeeper array benchmark. Bsides -> Besides Other optimizations eh? I see you couldn't help yourself.. well, the StringView::m_length ones are great. So are the method table bypassing ones. I guess I can't really complain about this. Very nice. > Source/JavaScriptCore/ChangeLog:11 > + I measured a 14% speed improvement in the Peacekeeper array benchmark. Sweet! > Source/JavaScriptCore/runtime/ArrayPrototype.cpp:394 > + for (; i < length && array->canGetIndexQuickly(i); ++i) { > + stringJoiner.append(*exec, array->getIndexQuickly(i)); > + if (exec->hadException()) > + return JSValue::encode(jsUndefined()); > } It's a bit sad that we fall out of the JSArray fast path here if we encounter a hole (canGetIndexQuickly fails.) Maybe if the array's Structure says holesMustForwardToPrototype() is false, we could do even better here? Not sure if holes a common occurrence here in practice. > Source/JavaScriptCore/runtime/JSStringJoiner.h:52 > + Vector<StringViewWithUnderlyingString> m_strings; This seems like an ideal candidate for some gratuitous inline capacity.
Comment on attachment 255396 [details] Patch r=me
Comment on attachment 255396 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=255396&action=review >> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:394 >> } > > It's a bit sad that we fall out of the JSArray fast path here if we encounter a hole (canGetIndexQuickly fails.) > Maybe if the array's Structure says holesMustForwardToPrototype() is false, we could do even better here? > Not sure if holes a common occurrence here in practice. I agree we should come back to this. It’s strange that this function iterates the array so differently from the other array prototype functions. >> Source/JavaScriptCore/runtime/JSStringJoiner.h:52 >> + Vector<StringViewWithUnderlyingString> m_strings; > > This seems like an ideal candidate for some gratuitous inline capacity. I considered it. A value 10 or higher would have helped slightly with the Peacekeeper benchmark.
Committed r185899: <http://trac.webkit.org/changeset/185899>
(In reply to comment #16) > Committed r185899: <http://trac.webkit.org/changeset/185899> It made runtime-array jsc test fail.
(In reply to comment #17) > It made runtime-array jsc test fail. OK, I’ll investigate. We probably need to roll out until I deal with that. Sadly it seems that EWS does not run the jsc tests.
The problem is that RuntimeArray is derived from JSArray, but JSArray::length gives the wrong answer. My patch changed the Array.concat implementation to use JSArray::length when the pointer is to a JSArray.
Fixed in http://trac.webkit.org/changeset/185904
This change caused Debug builds to assert in the following test: sputnik/Conformance/15_Native_Objects/15.4_Array/15.4.4/15.4.4.3_Array_prototype_toLocaleString/S15.4.4.3_A1_T1.html Thread 0 Crashed:: Dispatch queue: com.apple.main-thread 0 com.apple.JavaScriptCore 0x0000000108a0fa5a WTFCrash + 42 1 com.apple.JavaScriptCore 0x000000010870fdf2 JSC::JSStringJoiner::join(JSC::ExecState&) + 114 2 com.apple.JavaScriptCore 0x0000000107fc7060 JSC::arrayProtoFuncToLocaleString(JSC::ExecState*) + 1040 3 ??? 0x0000339124e01028 0 + 56698481938472 4 com.apple.JavaScriptCore 0x00000001087a50c1 llint_entry + 26101 5 com.apple.JavaScriptCore 0x000000010879e889 vmEntryToJavaScript + 361 6 com.apple.JavaScriptCore 0x00000001085edf6a JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*) + 266 7 com.apple.JavaScriptCore 0x00000001085d173e JSC::Interpreter::execute(JSC::ProgramExecutable*, JSC::ExecState*, JSC::JSObject*) + 4830 8 com.apple.JavaScriptCore 0x00000001080c9411 JSC::evaluate(JSC::ExecState*, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&) + 465 9 com.apple.WebCore 0x000000010de83735 WebCore::JSMainThreadExecState::evaluate(JSC::ExecState*, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&) + 69 10 com.apple.WebCore 0x000000010e8d1ddd WebCore::ScriptController::evaluateInWorld(WebCore::ScriptSourceCode const&, WebCore::DOMWrapperWorld&) + 317 11 com.apple.WebCore 0x000000010e8d1f24 WebCore::ScriptController::evaluate(WebCore::ScriptSourceCode const&) + 68 12 com.apple.WebCore 0x000000010e8e1267 WebCore::ScriptElement::executeScript(WebCore::ScriptSourceCode const&) + 455 13 com.apple.WebCore 0x000000010e8e01f0 WebCore::ScriptElement::prepareScript(WTF::TextPosition const&, WebCore::ScriptElement::LegacyTypeSupport) + 1792 14 com.apple.WebCore 0x000000010d8d1c46 WebCore::HTMLScriptRunner::runScript(WebCore::Element*, WTF::TextPosition const&) + 374 15 com.apple.WebCore 0x000000010d8d1a49 WebCore::HTMLScriptRunner::execute(WTF::PassRefPtr<WebCore::Element>, WTF::TextPosition const&) + 137 16 com.apple.WebCore 0x000000010d803ac0 WebCore::HTMLDocumentParser::runScriptsForPausedTreeBuilder() + 288 [...]
The sputnik/Conformance/15_Native_Objects/15.4_Array/15.4.4/15.4.4.3_Array_prototype_toLocaleString/S15.4.4.3_A1_T1.html assertion failure was fixed in r185909: <http://trac.webkit.org/r185909>.
Thanks for fixing the assertion, Mark!
(In reply to comment #23) > Thanks for fixing the assertion, Mark! I noticed two things about the assertion fix: 1) All vectors have sizes that are <= capacity, so the assertion no longer has any value. We should remove it! 2) Only toLocaleString omits null and undefined elements. Both toString and Array.join convert them to the empty string (so put separators around them). I suspect it is actually a bug in toLocaleString that it is skipping those elements.