Bug 182434

Summary: [JSC] Clean up ArraySpeciesCreate
Product: WebKit Reporter: Yusuke Suzuki <ysuzuki>
Component: JavaScriptCoreAssignee: 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 Flags
Patch
saam: review+
Patch
ysuzuki: review+, ysuzuki: commit-queue-
Patch none

Description Yusuke Suzuki 2018-02-02 08:08:12 PST
[JSC] Clean up ArraySpeciesCreate
Comment 1 Yusuke Suzuki 2018-02-02 08:23:14 PST
Created attachment 332969 [details]
Patch
Comment 2 Saam Barati 2018-02-02 08:46:16 PST
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 3 Yusuke Suzuki 2018-02-02 09:11:25 PST
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
Comment 4 Yusuke Suzuki 2018-02-02 09:12:29 PST
Committed r228012: <https://trac.webkit.org/changeset/228012>
Comment 5 Radar WebKit Bug Importer 2018-02-02 09:13:18 PST
<rdar://problem/37158348>
Comment 6 Saam Barati 2018-02-05 10:08:52 PST
This appears to be a 2-4% ARES6 regression across Mac and iOS. Can we roll it out to verify?
Comment 7 Saam Barati 2018-02-05 10:09:14 PST
(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.
Comment 8 WebKit Commit Bot 2018-02-05 10:10:03 PST
Re-opened since this is blocked by bug 182493
Comment 9 Saam Barati 2018-02-05 15:18:37 PST
(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
Comment 10 Alexey Shvayka 2019-07-02 12:39:48 PDT
Created attachment 373352 [details]
Patch

Fix native `speciesConstructArray` and expose it.
Comment 11 Yusuke Suzuki 2019-07-02 13:25:11 PDT
(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?
Comment 12 Alexey Shvayka 2019-07-02 13:49:09 PDT
(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 13 Yusuke Suzuki 2019-07-02 14:16:23 PDT
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 14 EWS Watchlist 2019-07-02 14:41:15 PDT
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 15 jsc-armv7 EWS 2019-07-02 14:48:32 PDT
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
Comment 16 Alexey Shvayka 2019-07-05 12:24:49 PDT
Created attachment 373523 [details]
Patch

Address review notes and adjust tests.
Comment 17 Alexey Shvayka 2019-07-05 12:37:25 PDT
(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 18 WebKit Commit Bot 2019-07-05 13:26:08 PDT
Comment on attachment 373523 [details]
Patch

Clearing flags on attachment: 373523

Committed r247173: <https://trac.webkit.org/changeset/247173>
Comment 19 Yusuke Suzuki 2019-07-05 14:30:36 PDT
Committed r247175: <https://trac.webkit.org/changeset/247175>