Bug 146191 – Make Array.join work directly on substrings without reifying them

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Darin Adler]

Make Array.join work directly on substrings without reifying them

[Darin Adler]

Created attachment 255318 [details]
Patch

[Darin Adler]

OK, Mr. Kling. You asked me to do this, so help advise me on what more I should do before landing it.

[WebKit Commit Bot]

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.

[Darin Adler]

Created attachment 255321 [details]
Patch

[WebKit Commit Bot]

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.

[Benjamin Poulain]

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.

[Darin Adler]

(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?

[Andreas Kling]

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.

[Darin Adler]

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).

[Darin Adler]

Created attachment 255396 [details]
Patch

[WebKit Commit Bot]

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.

[Darin Adler]

Ben, there’s a lot more that could be done to make Peacekeeper faster; we should go over that some time.

[Andreas Kling]

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.

[Andreas Kling]

Comment on attachment 255396 [details]
Patch

r=me

[Darin Adler]

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.

[Darin Adler]

Committed r185899: <http://trac.webkit.org/changeset/185899>

[Csaba Osztrogonác]

(In reply to comment #16)
> Committed r185899: <http://trac.webkit.org/changeset/185899>

It made runtime-array jsc test fail.

[Darin Adler]

(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.

[Darin Adler]

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.

[Darin Adler]

Fixed in http://trac.webkit.org/changeset/185904

[David Kilzer (:ddkilzer)]

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
[...]

[Mark Lam]

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>.

[Darin Adler]

Thanks for fixing the assertion, Mark!

[Darin Adler]

(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.