| Summary: | [JSC] Clean up ArraySpeciesCreate | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Yusuke Suzuki <ysuzuki> | ||||||||
| Component: | JavaScriptCore | Assignee: | Yusuke Suzuki <ysuzuki> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | ashvayka, commit-queue, ews-watchlist, guijemont, guijemont+jsc-armv7-ews, joepeck, keith_miller, mark.lam, msaboff, saam, tzagallo, webkit-bug-importer | ||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||
| Version: | WebKit Nightly Build | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| Bug Depends on: | 182493 | ||||||||||
| Bug Blocks: | |||||||||||
| Attachments: |
|
||||||||||
|
Description
Yusuke Suzuki
2018-02-02 08:08:12 PST
Created attachment 332969 [details]
Patch
Comment on attachment 332969 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=332969&action=review r=me if perf is neutral > Source/JavaScriptCore/ChangeLog:10 > + We have duplicate code in filter, map, concatSlowPath. > + This patch creates a new global private function @arraySpeciesCreate, > + and use it. I thought we duplicated the code for perf reasons. Is this a slowdown anywhere? Comment on attachment 332969 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=332969&action=review >> Source/JavaScriptCore/ChangeLog:10 >> + and use it. > > I thought we duplicated the code for perf reasons. Is this a slowdown anywhere? It does not show performance regression in microbenchmarks and Octane at least. array-prototype-map 26.6705+-0.4800 26.1032+-0.2851 might be 1.0217x faster typescript x2 620.55935+-11.16048 619.50969+-10.22278 Committed r228012: <https://trac.webkit.org/changeset/228012> This appears to be a 2-4% ARES6 regression across Mac and iOS. Can we roll it out to verify? (In reply to Saam Barati from comment #6) > This appears to be a 2-4% ARES6 regression across Mac and iOS. Can we roll > it out to verify? Ima just roll it out. I'll update this bug when I get definitive data after the rollout. Re-opened since this is blocked by bug 182493 (In reply to Saam Barati from comment #7) > (In reply to Saam Barati from comment #6) > > This appears to be a 2-4% ARES6 regression across Mac and iOS. Can we roll > > it out to verify? > > Ima just roll it out. I'll update this bug when I get definitive data after > the rollout. I've confirmed that my rollout progressed ARES6 Created attachment 373352 [details]
Patch
Fix native `speciesConstructArray` and expose it.
(In reply to Alexey Shvayka from comment #10) > Created attachment 373352 [details] > Patch > > Fix native `speciesConstructArray` and expose it. Can you measure JetStream2, specifically ARES-6? (In reply to Yusuke Suzuki from comment #11) > > Can you measure JetStream2, specifically ARES-6? Already did: [trunk] average of 25.19ms (21 samples, lowest: 23.95ms) [patch] average of 24.31ms (11 samples, lowest: 23.33ms) ~3.6% ARES progression (overall score, release build). Comment on attachment 373352 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=373352&action=review r=me with fixes. > Source/JavaScriptCore/ChangeLog:13 > + Let's paste the measurement result here to tell that this is measured and it is not intended to cause the performance regression. > Source/JavaScriptCore/runtime/ArrayPrototype.cpp:242 > + && jsDynamicCast<ArrayConstructor*>(vm, constructorObject); Use `inherit<ArrayConstructor>` instead. > Source/JavaScriptCore/runtime/ArrayPrototype.cpp:272 > + JSObject* object = asObject(exec->argument(0)); Let's use uncheckedArgument(0) because we know all the caller of arrayProtoFuncSpeciesCreate passes the first argument. > Source/JavaScriptCore/runtime/ArrayPrototype.cpp:273 > + auto length = static_cast<uint64_t>(exec->argument(1).asNumber()); Use `uint64_t` instead of `auto` here. JSC codebase likes writing a type explicitly. And use `uncheckedArgument(1)` too. > Source/JavaScriptCore/runtime/ArrayPrototype.cpp:278 > + return encodedJSValue(); Use `{}` for the newer code. > Source/JavaScriptCore/runtime/ArrayPrototype.cpp:283 > + auto unsignedLength = static_cast<unsigned>(length); > + if (unsignedLength != length) { Comparing with `std::numeric_limits<unsigned>::max()` is better since it describes the intent explicitly. > Source/JavaScriptCore/runtime/ArrayPrototype.cpp:285 > + return encodedJSValue(); Ditto. > Source/JavaScriptCore/runtime/ArrayPrototype.cpp:290 > + JSObject* result = constructEmptyArray(exec, nullptr, unsignedLength); > + RETURN_IF_EXCEPTION(scope, encodedJSValue()); > + RELEASE_AND_RETURN(scope, JSValue::encode(result)); Do things like, RELEASE_AND_RETURN(scope, JSValue::encode(constructEmptyArray(exec, nullptr, unsignedLength))); The L289's exception check is not necessary if we write this code like the above one. Comment on attachment 373352 [details] Patch Attachment 373352 [details] did not pass jsc-ews (mac): Output: https://webkit-queues.webkit.org/results/12643623 New failing tests: stress/array-flatmap.js.ftl-eager-no-cjit-b3o1 stress/array-flatten.js.no-ftl stress/array-flatmap.js.bytecode-cache stress/array-flatten.js.mini-mode stress/array-flatten.js.ftl-no-cjit-b3o0 stress/array-flatten.js.ftl-no-cjit-small-pool stress/array-flatmap.js.ftl-no-cjit-validate-sampling-profiler stress/array-species-create-should-handle-masquerader.js.ftl-no-cjit-b3o0 stress/array-species-create-should-handle-masquerader.js.mini-mode stress/array-flatten.js.ftl-no-cjit-no-put-stack-validate stress/array-species-create-should-handle-masquerader.js.dfg-eager-no-cjit-validate stress/array-species-create-should-handle-masquerader.js.ftl-no-cjit-no-put-stack-validate stress/array-flatmap.js.dfg-eager-no-cjit-validate stress/array-species-create-should-handle-masquerader.js.ftl-eager-no-cjit-b3o1 stress/array-flatten.js.dfg-eager stress/array-species-create-should-handle-masquerader.js.default stress/array-species-create-should-handle-masquerader.js.bytecode-cache stress/array-flatten.js.no-cjit-validate-phases stress/array-flatten.js.ftl-eager-no-cjit-b3o1 stress/array-flatmap.js.no-ftl stress/array-species-create-should-handle-masquerader.js.ftl-no-cjit-validate-sampling-profiler stress/array-flatten.js.ftl-no-cjit-no-inline-validate stress/array-flatmap.js.ftl-no-cjit-no-inline-validate stress/array-flatten.js.no-cjit-collect-continuously stress/array-flatmap.js.default stress/array-flatmap.js.mini-mode stress/array-flatten.js.ftl-no-cjit-validate-sampling-profiler stress/array-flatmap.js.ftl-no-cjit-no-put-stack-validate stress/array-flatmap.js.dfg-eager stress/array-species-create-should-handle-masquerader.js.no-cjit-validate-phases stress/array-flatten.js.default stress/array-flatten.js.bytecode-cache stress/array-species-create-should-handle-masquerader.js.no-ftl stress/array-flatten.js.no-llint stress/array-flatmap.js.ftl-eager-no-cjit stress/array-flatmap.js.ftl-eager stress/array-flatmap.js.no-llint stress/array-species-create-should-handle-masquerader.js.dfg-maximal-flush-validate-no-cjit stress/array-flatmap.js.no-cjit-collect-continuously stress/array-flatmap.js.ftl-no-cjit-small-pool stress/array-flatmap.js.no-cjit-validate-phases stress/array-species-create-should-handle-masquerader.js.ftl-eager-no-cjit stress/array-flatten.js.dfg-eager-no-cjit-validate stress/array-flatten.js.ftl-eager-no-cjit stress/array-species-create-should-handle-masquerader.js.ftl-no-cjit-no-inline-validate stress/array-flatten.js.ftl-eager stress/array-species-create-should-handle-masquerader.js.ftl-eager stress/array-species-create-should-handle-masquerader.js.no-llint stress/array-flatten.js.dfg-maximal-flush-validate-no-cjit stress/array-flatmap.js.ftl-no-cjit-b3o0 stress/array-species-create-should-handle-masquerader.js.no-cjit-collect-continuously stress/array-flatmap.js.dfg-maximal-flush-validate-no-cjit stress/array-species-create-should-handle-masquerader.js.ftl-no-cjit-small-pool stress/array-species-create-should-handle-masquerader.js.dfg-eager apiTests Comment on attachment 373352 [details] Patch Attachment 373352 [details] did not pass jsc-armv7-ews (jsc-only): Output: https://webkit-queues.webkit.org/results/12643649 New failing tests: stress/array-flatten.js.mini-mode stress/array-species-create-should-handle-masquerader.js.mini-mode stress/array-species-create-should-handle-masquerader.js.dfg-eager-no-cjit-validate stress/array-flatmap.js.no-cjit-collect-continuously stress/array-species-create-should-handle-masquerader.js.default stress/array-flatten.js.no-cjit-validate-phases stress/array-flatten.js.no-cjit-collect-continuously stress/array-flatmap.js.default stress/array-flatmap.js.mini-mode stress/array-species-create-should-handle-masquerader.js.no-cjit-validate-phases stress/array-flatten.js.default stress/array-flatten.js.no-llint stress/array-species-create-should-handle-masquerader.js.dfg-eager stress/array-flatmap.js.no-llint stress/array-species-create-should-handle-masquerader.js.dfg-maximal-flush-validate-no-cjit stress/array-flatten.js.dfg-eager stress/array-flatmap.js.dfg-eager-no-cjit-validate stress/array-flatten.js.dfg-eager-no-cjit-validate stress/array-flatmap.js.no-cjit-validate-phases stress/array-flatmap.js.dfg-eager stress/array-species-create-should-handle-masquerader.js.no-llint stress/array-flatten.js.dfg-maximal-flush-validate-no-cjit stress/array-species-create-should-handle-masquerader.js.no-cjit-collect-continuously stress/array-flatmap.js.dfg-maximal-flush-validate-no-cjit apiTests Created attachment 373523 [details]
Patch
Address review notes and adjust tests.
(In reply to Yusuke Suzuki from comment #13) > Comment on attachment 373352 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=373352&action=review > > r=me with fixes. Thank you for thorough review, Yusuke. Code looks way better now. > > > Source/JavaScriptCore/ChangeLog:13 > > + > > Let's paste the measurement result here to tell that this is measured and it > is not intended to cause the performance regression. > I've repeated ARES-6 in more "sterile" environment (after reboot, no running apps, Wi-Fi off): same result (trunk: 23.24ms, patch: 23.26ms) for "cold" runs (10 runs, 3 samples, 90s interval between runs) 1% progression (trunk: 25.41ms, patch: 25.07ms) for "hot" runs (10 runs, 100 samples, no timeout) Comment on attachment 373523 [details] Patch Clearing flags on attachment: 373523 Committed r247173: <https://trac.webkit.org/changeset/247173> Committed r247175: <https://trac.webkit.org/changeset/247175> |