Bug 199636 - Optimize join of large empty arrays
Summary: Optimize join of large empty arrays
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tadeu Zagallo
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-07-09 12:46 PDT by Tadeu Zagallo
Modified: 2019-07-10 01:25 PDT (History)
11 users (show)

See Also:


Attachments
Patch (3.56 KB, patch)
2019-07-09 12:49 PDT, Tadeu Zagallo
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-highsierra (1.89 MB, application/zip)
2019-07-09 13:17 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-highsierra-wk2 (1.13 MB, application/zip)
2019-07-09 13:47 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews115 for mac-highsierra (2.94 MB, application/zip)
2019-07-09 13:48 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews122 for ios-simulator-wk2 (1.25 MB, application/zip)
2019-07-09 13:55 PDT, Build Bot
no flags Details
Patch (4.61 KB, patch)
2019-07-09 15:06 PDT, Tadeu Zagallo
no flags Details | Formatted Diff | Diff
Patch for landing (4.61 KB, patch)
2019-07-09 23:42 PDT, Tadeu Zagallo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tadeu Zagallo 2019-07-09 12:46:09 PDT
...
Comment 1 Tadeu Zagallo 2019-07-09 12:49:49 PDT
Created attachment 373745 [details]
Patch
Comment 2 Build Bot 2019-07-09 13:17:11 PDT
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.
Comment 3 Build Bot 2019-07-09 13:17:13 PDT
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
Comment 4 Yusuke Suzuki 2019-07-09 13:28:07 PDT
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?
Comment 5 Mark Lam 2019-07-09 13:29:00 PDT
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.
Comment 6 Yusuke Suzuki 2019-07-09 13:30:56 PDT
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.
Comment 7 Build Bot 2019-07-09 13:47:54 PDT
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.
Comment 8 Build Bot 2019-07-09 13:47:56 PDT
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
Comment 9 Build Bot 2019-07-09 13:48:39 PDT
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.
Comment 10 Build Bot 2019-07-09 13:48:41 PDT
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
Comment 11 Build Bot 2019-07-09 13:55:03 PDT
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.
Comment 12 Build Bot 2019-07-09 13:55:05 PDT
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
Comment 13 Mark Lam 2019-07-09 13:55:32 PDT
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.
Comment 14 Tadeu Zagallo 2019-07-09 15:06:25 PDT
Created attachment 373772 [details]
Patch
Comment 15 Mark Lam 2019-07-09 15:10:53 PDT
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).
Comment 16 Mark Lam 2019-07-09 15:12:02 PDT
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.
Comment 17 Tadeu Zagallo 2019-07-09 15:17:18 PDT
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).
Comment 18 Mark Lam 2019-07-09 15:22:32 PDT
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.
Comment 19 Build Bot 2019-07-09 18:09:30 PDT
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
Comment 20 Tadeu Zagallo 2019-07-09 23:42:37 PDT
Created attachment 373824 [details]
Patch for landing
Comment 21 WebKit Commit Bot 2019-07-10 01:24:58 PDT
Comment on attachment 373824 [details]
Patch for landing

Clearing flags on attachment: 373824

Committed r247296: <https://trac.webkit.org/changeset/247296>
Comment 22 WebKit Commit Bot 2019-07-10 01:25:00 PDT
All reviewed patches have been landed.  Closing bug.
Comment 23 Radar WebKit Bug Importer 2019-07-10 01:25:26 PDT
<rdar://problem/52878284>