| Summary: | Optimize join of large empty arrays | ||
|---|---|---|---|
| Product: | WebKit | Reporter: | Tadeu Zagallo <tzagallo> |
| Component: | JavaScriptCore | Assignee: | Tadeu Zagallo <tzagallo> |
| Status: | RESOLVED FIXED | ||
| Severity: | Normal | CC: | commit-queue, ews-watchlist, fpizlo, keith_miller, mark.lam, msaboff, rmorisset, rniwa, saam, webkit-bug-importer, ysuzuki |
| Priority: | P2 | Keywords: | InRadar |
| Version: | WebKit Nightly Build | ||
| Hardware: | Unspecified | ||
| OS: | Unspecified | ||
| Attachments: | |||
|
Description
Tadeu Zagallo
2019-07-09 12:46:09 PDT
Created attachment 373745 [details]
Patch
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. 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 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 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 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 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. 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 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. 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 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. 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 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. Created attachment 373772 [details]
Patch
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 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 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 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 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 Created attachment 373824 [details]
Patch for landing
Comment on attachment 373824 [details] Patch for landing Clearing flags on attachment: 373824 Committed r247296: <https://trac.webkit.org/changeset/247296> All reviewed patches have been landed. Closing bug. |