Bug 199636 – Optimize join of large empty arrays

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

[Tadeu Zagallo]

...

[Tadeu Zagallo]

Created attachment 373745 [details]
Patch

[EWS Watchlist]

Comment on attachment 373745 [details]
Patch

Attachment 373745 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/12700541

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 373748 [details]
Archive of layout-test-results from ews102 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[Yusuke Suzuki]

Comment on attachment 373745 [details]
Patch

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

> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:522
> +            unsigned count = length - 1;

Do we need `length == 0` handling?

[Mark Lam]

Comment on attachment 373745 [details]
Patch

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

I think one the changes in your patch is that you're creating a JSRopeString instead of a regular JSString.  In this microbenchmark, the result string is never consumed as a string.  As a result, we never incur the cost of resolving the rope.  In contrast, the old default path that falls thru to the generalCase would eagerly resolve the rope.  What this means is that in practice, you may potentially be shifting the workload to a later time.  Just in case, please run benchmarks to make sure there is no regression and include the benchmark results in the ChangeLog.

r=me if EWS is green and there's no perf regression.

> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:521
> +            JSString* operand = jsString(&vm, separator.toString());

jsString() allocates and can throw an exception.  So, you'll need a RETURN_IF_EXCEPTION(scope, { }) here.

>> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:522
>> +            unsigned count = length - 1;
> 
> Do we need `length == 0` handling?

This is already handled in "case 0:" above.

[Yusuke Suzuki]

Comment on attachment 373745 [details]
Patch

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

>>> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:522
>>> +            unsigned count = length - 1;
>> 
>> Do we need `length == 0` handling?
> 
> This is already handled in "case 0:" above.

I think `case 0:` is for separator.length (separator string's length). And this "length" is length of array.

[EWS Watchlist]

Comment on attachment 373745 [details]
Patch

Attachment 373745 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/12700739

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 373754 [details]
Archive of layout-test-results from ews106 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 373745 [details]
Patch

Attachment 373745 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/12700621

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 373755 [details]
Archive of layout-test-results from ews115 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 373745 [details]
Patch

Attachment 373745 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/12700633

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 373758 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.14.5

[Mark Lam]

Comment on attachment 373745 [details]
Patch

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

>>>> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:522
>>>> +            unsigned count = length - 1;
>>> 
>>> Do we need `length == 0` handling?
>> 
>> This is already handled in "case 0:" above.
> 
> I think `case 0:` is for separator.length (separator string's length). And this "length" is length of array.

Oops.  Sorry I missed that.

[Tadeu Zagallo]

Created attachment 373772 [details]
Patch

[Mark Lam]

Comment on attachment 373772 [details]
Patch

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

r=me with fix.

> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:521
> +            if (!length)

This should be length <= 1.  If length == 1, it should also produce an empty string (nothing to separator).

[Mark Lam]

Comment on attachment 373772 [details]
Patch

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

>> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:521
>> +            if (!length)
> 
> This should be length <= 1.  If length == 1, it should also produce an empty string (nothing to separator).

Please add a test to catch this case if the EWS doesn't already have a test that catches this.

[Tadeu Zagallo]

Comment on attachment 373772 [details]
Patch

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

>>> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:521
>>> +            if (!length)
>> 
>> This should be length <= 1.  If length == 1, it should also produce an empty string (nothing to separator).
> 
> Please add a test to catch this case if the EWS doesn't already have a test that catches this.

That makes sense, I'll change it. We don't need to allocate operand when length is 1, but it will also return the empty string as is (it won't append to result in 528, since count is 0, and then it will return result in 534, which is still the empty string).

[Mark Lam]

Comment on attachment 373772 [details]
Patch

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

>>>> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:521
>>>> +            if (!length)
>>> 
>>> This should be length <= 1.  If length == 1, it should also produce an empty string (nothing to separator).
>> 
>> Please add a test to catch this case if the EWS doesn't already have a test that catches this.
> 
> That makes sense, I'll change it. We don't need to allocate operand when length is 1, but it will also return the empty string as is (it won't append to result in 528, since count is 0, and then it will return result in 534, which is still the empty string).

Ah, you are right.  That's the second time today I'm looking specifically for something in the code, and completely missed what I was looking for.

[EWS Watchlist]

Comment on attachment 373772 [details]
Patch

Attachment 373772 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/12702977

New failing tests:
mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla-ftl-eager-no-cjit-validate-phases
apiTests

[Tadeu Zagallo]

Created attachment 373824 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 373824 [details]
Patch for landing

Clearing flags on attachment: 373824

Committed r247296: <https://trac.webkit.org/changeset/247296>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/52878284>