1619007 - Do BytecodeEmitter::emitDestructuringObjRestExclusionSet without emitting GC Objects

Status: RESOLVED FIXED

(Core :: JavaScript Engine, task, P3)

Description

[Matthew Gaudet (he/him) [:mgaudet]]

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.

Comment 1

[Matthew Gaudet (he/him) [:mgaudet]]

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.

Comment 2

[André Bargull [:anba]]

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?

Comment 3

[Matthew Gaudet (he/him) [:mgaudet]]

If you have a prototype already, yes, absolutely.

You’re amazing!

Comment 4

[André Bargull [:anba]]

Attachment: Bug 1619007 - Part 1: Remove unnecessary manual stack depth adjustment. r=mgaudet!

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

Comment 5

[André Bargull [:anba]]

Attachment: Bug 1619007 - Part 2: Remove unnecessary is<T> tests when directly followed by as<T>. r=mgaudet!

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

Comment 6

[André Bargull [:anba]]

Attachment: Bug 1619007 - Part 3: Call NewPlainObjectWithProperties instead of reimplementing it. r=mgaudet!

Depends on D65769

Comment 7

[André Bargull [:anba]]

Attachment: Bug 1619007 - Part 4: Return plain Value from InterpretObjLiteralValue. r=mgaudet!

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

Depends on D65771

Comment 8

[André Bargull [:anba]]

Attachment: Bug 1619007 - Part 5: Simplify while loop exit condition. r=mgaudet!

Depends on D65772

Comment 9

[André Bargull [:anba]]

Attachment: Bug 1619007 - Part 6: Prefer emplaceBack instead of append when adding elements to IdValueVector. r=mgaudet!

Depends on D65773

Comment 10

[André Bargull [:anba]]

Attachment: Bug 1619007 - Part 7: Remove unused `type` parameter from emitPropertyListObjLiteral. r=mgaudet!

Depends on D65774

Comment 11

[André Bargull [:anba]]

Attachment: Bug 1619007 - Part 8: Use ObjLiteral to create the destructuring rest exclusion set. r=mgaudet!

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

Comment 12

[André Bargull [:anba]]

Attachment: Bug 1619007 - Part 9: Don't manually inline emitAtomOp with an atom index. r=mgaudet!

Depends on D65776

Comment 13

[André Bargull [:anba]]

Attachment: Bug 1619007 - Part 10: Remove no longer used replaceNewInitWithNewObject method. r=mgaudet!

Depends on D65777

Comment 14

[André Bargull [:anba]]

Attachment: Bug 1619007 - Part 11: Remove unused uint32_t operand space from JSOp::NewInit. r=mgaudet!

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 15

[André Bargull [:anba]]

Attachment: Bug 1619007 - Part 12: Remove unused class-body support from isPropertyListObjLiteralCompatible. r=mgaudet!

Depends on D65786

Comment 16

[André Bargull [:anba]]

Attachment: Bug 1619007 - Part 13: Add missing BigInt property key handling. r=mgaudet!

This case was missed in bug 1605835.

Depends on D65787

Comment 17

[Pulsebot]

Pushed by [e-mail]:
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

[Andrei Ciure[:aciure]]

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