1855705 - Add a simple cache for Object.assign's fast path

Status: RESOLVED FIXED

(Core :: JavaScript Engine, task)

Description

[Jan de Mooij [:jandem]]

I looked into this after bug 1853467 and a one-entry cache for the fast path we have for plain objects is highly effective: it has a hit rate > 90% on both Speedometer 2 and 3 and improves a shell version of the micro-benchmark in bug 1853467 as follows:

frozen-10 Took 702.12ms. => 291.76ms.
frozen-8 Took 573.66ms. => 243.06ms.
frozen-200 Took 1565.07ms. => 510.86ms.

regular-10 Took 480.85ms. => 275.58ms.
regular-8 Took 428.57ms. => 247.62ms.
regular-200 Took 1009.22ms. => 476.14ms.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Bug 1855705 part 1 - Simplify loop in TryAssignPlain a bit. r?jonco!

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Bug 1855705 part 2 - Add a cache for Object.assign's fast path. r?jonco!

The fast path we have for plain objects only works with data properties stored
in the shape, so a cache based on the shapes of the two objects works really
well for this code.

Even though it's a one-entry cache, the lookup method has a hit rate of
at least 90% on both Speedometer 2 and 3 so it's very effective.

Depends on D189503

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/36cb8b2bc8e2
part 1 - Simplify loop in TryAssignPlain a bit. r=jonco
https://hg.mozilla.org/integration/autoland/rev/26356a7c39e4
part 2 - Add a cache for Object.assign's fast path. r=jonco

Comment 4

[Natalia Csoregi [:nataliaCs]]

Backed out for causing assertion failures on Object.cpp

• backout: https://hg.mozilla.org/integration/autoland/rev/a216736126b15f69ebee5d940a5c3394f95759cb
• push: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&selectedTaskRun=Vzeu8Qa7SKeTxvsyeNoIBg.0&revision=26356a7c39e422cbadeb857db90bf1950622feef
• failure log: https://treeherder.mozilla.org/logviewer?job_id=430656680&repo=autoland&lineNumber=4024

[task 2023-09-28T16:30:53.221Z] 16:30:53     INFO - GECKO(1564) | must wait for focus in content
[task 2023-09-28T16:30:53.579Z] 16:30:53     INFO - GECKO(1564) | Assertion failure: fromPlain->slotSpan() == props.length(), at /builds/worker/checkouts/gecko/js/src/builtin/Object.cpp:965
[task 2023-09-28T16:30:53.581Z] 16:30:53     INFO -  Initializing stack-fixing for the first stack frame, this may take a while...
[task 2023-09-28T16:31:04.247Z] 16:31:04     INFO - GECKO(1564) | #01: JS_AssignObject(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSObject*>) [js/src/builtin/Object.cpp:1174]
[task 2023-09-28T16:31:04.247Z] 16:31:04     INFO - GECKO(1564) | #02: obj_assign(JSContext*, unsigned int, JS::Value*) [js/src/builtin/Object.cpp:1221]
[task 2023-09-28T16:31:04.247Z] 16:31:04     INFO - GECKO(1564) | #03: CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), js::CallReason, JS::CallArgs const&) [js/src/vm/Interpreter.cpp:486]
[task 2023-09-28T16:31:04.247Z] 16:31:04     INFO - GECKO(1564) | #04: js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) [js/src/vm/Interpreter.cpp:580]
[task 2023-09-28T16:31:04.247Z] 16:31:04     INFO - GECKO(1564) | #05: js::Interpret(JSContext*, js::RunState&) [js/src/vm/Interpreter.cpp:0]
[task 2023-09-28T16:31:04.247Z] 16:31:04     INFO - GECKO(1564) | #06: js::RunScript(JSContext*, js::RunState&) [js/src/vm/Interpreter.cpp:458]
[task 2023-09-28T16:31:04.247Z] 16:31:04     INFO - GECKO(1564) | #07: js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) [js/src/vm/Interpreter.cpp:612]
[task 2023-09-28T16:31:04.247Z] 16:31:04     INFO - GECKO(1564) | #08: js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>, js::CallReason) [js/src/vm/Interpreter.cpp:679]
[task 2023-09-28T16:31:04.247Z] 16:31:04     INFO - GECKO(1564) | #09: js::CallSelfHostedFunction(JSContext*, JS::Handle<js::PropertyName*>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) [js/src/vm/SelfHosting.cpp:1520]
[task 2023-09-28T16:31:04.247Z] 16:31:04     INFO - GECKO(1564) | #10: js::jit::InterpretResume(JSContext*, JS::Handle<JSObject*>, JS::Value*, JS::MutableHandle<JS::Value>) [js/src/jit/VMFunctions.cpp:1027]
[task 2023-09-28T16:31:04.247Z] 16:31:04     INFO - GECKO(1564) | #11: ??? (???:???)
[task 2023-09-28T16:31:04.247Z] 16:31:04     INFO - GECKO(1564) | ExceptionHandler::GenerateDump cloned child 2153
[task 2023-09-28T16:31:04.247Z] 16:31:04     INFO - GECKO(1564) | ExceptionHandler::SendContinueSignalToChild sent continue signal to child
[task 2023-09-28T16:31:04.247Z] 16:31:04     INFO - GECKO(1564) | ExceptionHandler::WaitForContinueSignal waiting for continue signal...

Comment 5

[Jan de Mooij [:jandem]]

Bleh, I think it's just a bogus assertion for dictionary objects. I'll add a test and confirm that's what's going on.

Comment 6

[Jan de Mooij [:jandem]]

I fixed the assertion and added some tests.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/684ff70f0f9c
part 1 - Simplify loop in TryAssignPlain a bit. r=jonco
https://hg.mozilla.org/integration/autoland/rev/7728a6fdad94
part 2 - Add a cache for Object.assign's fast path. r=jonco

Comment 8

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/684ff70f0f9c
https://hg.mozilla.org/mozilla-central/rev/7728a6fdad94