Closed Bug 1619007 Opened 1 month ago Closed 25 days ago

Do BytecodeEmitter::emitDestructuringObjRestExclusionSet without emitting GC Objects

Categories

(Core :: JavaScript Engine, task, P3)

task

Tracking

()

RESOLVED FIXED
mozilla75
Tracking Status
firefox75 --- fixed

People

(Reporter: mgaudet, Assigned: anba)

References

(Blocks 1 open bug)

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.

See Also: → 1580246
See Also: → 1619010
Priority: -- → P3

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.

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?

If you have a prototype already, yes, absolutely.

You’re amazing!

The preceding JSOp::{Object,NewObject,NewObjectWithGroup,NewArrayCopyOnWrite}
opcode already performs the correct stack depth adjustment.

Assignee: nobody → andrebargull
Status: NEW → ASSIGNED

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

We can directly return JS::Value from InterpretObjLiteralValue instead of passing
it indirectly through a MutableHandleValue.

Depends on D65771

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

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

This case was missed in bug 1605835.

Depends on D65787

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
Regressions: 1621056
You need to log in before you can comment on or make changes to this bug.