Bug 182434 – [JSC] Clean up ArraySpeciesCreate

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Yusuke Suzuki]

[JSC] Clean up ArraySpeciesCreate

[Yusuke Suzuki]

Created attachment 332969 [details]
Patch

[Saam Barati]

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?

[Yusuke Suzuki]

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

[Yusuke Suzuki]

Committed r228012: <https://trac.webkit.org/changeset/228012>

[Radar WebKit Bug Importer]

<rdar://problem/37158348>

[Saam Barati]

This appears to be a 2-4% ARES6 regression across Mac and iOS. Can we roll it out to verify?

[Saam Barati]

(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.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 182493

[Saam Barati]

(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

[Alexey Shvayka]

Created attachment 373352 [details]
Patch

Fix native `speciesConstructArray` and expose it.

[Yusuke Suzuki]

(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?

[Alexey Shvayka]

(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).

[Yusuke Suzuki]

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.

[EWS Watchlist]

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

[jsc-armv7 EWS]

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

[Alexey Shvayka]

Created attachment 373523 [details]
Patch

Address review notes and adjust tests.

[Alexey Shvayka]

(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)

[WebKit Commit Bot]

Comment on attachment 373523 [details]
Patch

Clearing flags on attachment: 373523

Committed r247173: <https://trac.webkit.org/changeset/247173>

[Yusuke Suzuki]

Committed r247175: <https://trac.webkit.org/changeset/247175>