Do BytecodeEmitter::emitDestructuringObjRestExclusionSet without emitting GC Objects
Categories
(Core :: JavaScript Engine, task, P3)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox75 | --- | fixed |
People
(Reporter: mgaudet, Assigned: anba)
References
Details
Attachments
(13 files)
|
47 bytes,
text/x-phabricator-request
|
Details | Review | |
|
47 bytes,
text/x-phabricator-request
|
Details | Review | |
|
47 bytes,
text/x-phabricator-request
|
Details | Review | |
|
47 bytes,
text/x-phabricator-request
|
Details | Review | |
|
47 bytes,
text/x-phabricator-request
|
Details | Review | |
|
47 bytes,
text/x-phabricator-request
|
Details | Review | |
|
47 bytes,
text/x-phabricator-request
|
Details | Review | |
|
47 bytes,
text/x-phabricator-request
|
Details | Review | |
|
47 bytes,
text/x-phabricator-request
|
Details | Review | |
|
47 bytes,
text/x-phabricator-request
|
Details | Review | |
|
47 bytes,
text/x-phabricator-request
|
Details | Review | |
|
47 bytes,
text/x-phabricator-request
|
Details | Review | |
|
47 bytes,
text/x-phabricator-request
|
Details | Review |
BytecodeEmitter::emitDestructuringObjRestExclusionSet has an optimization which involves allocating a JSObject and doing some analysis and mutation of the object during BCE, with the goal of emitting better bytecode.
This approach as written is incompatible with stencil and needs to be rethought.
|
||
Updated•5 years ago
|
|
Reporter | |
Comment 1•5 years ago
|
||
One thing that would be worthwhile to do is to experiment with turning this optimization off (the problematic code is an optimization) and see what the impact is on performance.
|
Assignee | |
Comment 2•5 years ago
|
||
Performance is at minimum 50% slower when JSOp::NewObject isn't used and there's a higher impact when more properties are present in the exclusion set, for example at eight properties it was about twice as slow for me. I did a quick test to see if it's possible to use ObjLiteralCreationData here and it seems to work just fine. Should I prepare a proper patch to switch emitDestructuringObjRestExclusionSet to use ObjLiteralCreationData?
|
|
Assignee | |
Comment 4•5 years ago
|
||
The preceding JSOp::{Object,NewObject,NewObjectWithGroup,NewArrayCopyOnWrite}
opcode already performs the correct stack depth adjustment.
|
||
Updated•5 years ago
|
|
|
Assignee | |
Comment 5•5 years ago
|
||
The standard convention is to omit a is<T> test when it is directly followed
by as<T>, because as<T> is also testing for is<T>.
Depends on D65768
|
|
Assignee | |
Comment 7•5 years ago
|
||
We can directly return JS::Value from InterpretObjLiteralValue instead of passing
it indirectly through a MutableHandleValue.
Depends on D65771
|
|
Assignee | |
Comment 11•5 years ago
|
||
Use the ObjLiteral writer for the destructuring rest exclusion set when all
property keys are simple atoms, i.e. ObjectPropertyName or StringExpr. Also
supports MutateProto aka "proto", because "proto" doesn't have any
special meaning in object destructuring patterns. Number and BigInt property
keys stay unsupported for the fast-path and computed property names also need
to take the slow path, because their value isn't known at parse time.
Depends on D65775
|
|
Assignee | |
Comment 14•5 years ago
|
||
After part 10 we no longer perform an in-place update from JSOp::NewInit to
JSOp::NewObject, so the extra operand space for JSOp::NewInit isn't needed
anymore.
Depends on D65781
|
||
Comment 17•5 years ago
|
||
Pushed by cbrindusan@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/7fb3fd9172c5 Part 1: Remove unnecessary manual stack depth adjustment. r=cfallin https://hg.mozilla.org/integration/autoland/rev/de425cf4b2ac Part 2: Remove unnecessary is<T> tests when directly followed by as<T>. r=mgaudet https://hg.mozilla.org/integration/autoland/rev/c5b5f5a316f2 Part 3: Call NewPlainObjectWithProperties instead of reimplementing it. r=mgaudet https://hg.mozilla.org/integration/autoland/rev/bbccf2268360 Part 4: Return plain Value from InterpretObjLiteralValue. r=mgaudet https://hg.mozilla.org/integration/autoland/rev/b906552a9959 Part 5: Simplify while loop exit condition. r=mgaudet https://hg.mozilla.org/integration/autoland/rev/2b7d03a2ea2c Part 6: Prefer emplaceBack instead of append when adding elements to IdValueVector. r=mgaudet https://hg.mozilla.org/integration/autoland/rev/201ec4cf9a1e Part 7: Remove unused `type` parameter from emitPropertyListObjLiteral. r=mgaudet https://hg.mozilla.org/integration/autoland/rev/e8579166fcbf Part 8: Use ObjLiteral to create the destructuring rest exclusion set. r=mgaudet https://hg.mozilla.org/integration/autoland/rev/8b170b754a5b Part 9: Don't manually inline emitAtomOp with an atom index. r=mgaudet https://hg.mozilla.org/integration/autoland/rev/44fe840e94ff Part 10: Remove no longer used replaceNewInitWithNewObject method. r=mgaudet https://hg.mozilla.org/integration/autoland/rev/6c8fa3e00997 Part 11: Remove unused uint32_t operand space from JSOp::NewInit. r=mgaudet https://hg.mozilla.org/integration/autoland/rev/5e52daafbbed Part 12: Remove unused class-body support from isPropertyListObjLiteralCompatible. r=mgaudet https://hg.mozilla.org/integration/autoland/rev/9e1b60eda95f Part 13: Add missing BigInt property key handling. r=mgaudet
|
||
Comment 18•5 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/7fb3fd9172c5
https://hg.mozilla.org/mozilla-central/rev/de425cf4b2ac
https://hg.mozilla.org/mozilla-central/rev/c5b5f5a316f2
https://hg.mozilla.org/mozilla-central/rev/bbccf2268360
https://hg.mozilla.org/mozilla-central/rev/b906552a9959
https://hg.mozilla.org/mozilla-central/rev/2b7d03a2ea2c
https://hg.mozilla.org/mozilla-central/rev/201ec4cf9a1e
https://hg.mozilla.org/mozilla-central/rev/e8579166fcbf
https://hg.mozilla.org/mozilla-central/rev/8b170b754a5b
https://hg.mozilla.org/mozilla-central/rev/44fe840e94ff
https://hg.mozilla.org/mozilla-central/rev/6c8fa3e00997
https://hg.mozilla.org/mozilla-central/rev/5e52daafbbed
https://hg.mozilla.org/mozilla-central/rev/9e1b60eda95f
Description
•