1662559 - Change storage for private methods to save space

Status: RESOLVED FIXED

(Core :: JavaScript Engine, task, P3)

Description

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

Currently the storage of private methods means that every private method is treated like a field on an object.

This can be improved.

Comment 1

[Jason Orendorff [:jorendorff]]

Attachment: Bug 1662559 - Part 1: Rename NameLocation::fromBinding() -> BindingIter::nameLocation(). r=arai

Comment 2

[Jason Orendorff [:jorendorff]]

Attachment: Bug 1662559 - Part 2: Clone LexicalScope{Node} -> ClassBodyScope{Node}. r=arai

The new class is not actually used yet; see the next patch.

Depends on D108283

Comment 3

[Jason Orendorff [:jorendorff]]

Attachment: Bug 1662559 - Part 3: Clone LexicalScopeNode to ClassBodyScopeNode in the AST. r=mgaudet

Depends on D108284

Comment 4

[Jason Orendorff [:jorendorff]]

Attachment: Bug 1662559 - Part 3: Tighten up type of the ClassMemberList node kind. r=arai

Depends on D108285

Comment 5

[Jason Orendorff [:jorendorff]]

Attachment: Bug 1662559 - Part 4: Introduce ClassBodyLexicalEnvironmentObject. r=arai

It is a copy of BlockLexicalEnvironmentObject. Note that

• ClassBodyScope is not a LexicalScope, BUT

• ClassBodyLexicalEnvironmentObject is a LexicalEnvironmentObject

This is strictly a matter of how much implementation is shared. The word
"Lexical" in LexicalEnvironmentObject, and to some extent throughout
SpiderMonkey, is alas already pretty meaningless. In the language
specification, all environments are lexical environments.

Depends on D108286

Comment 6

[Jason Orendorff [:jorendorff]]

Attachment: Bug 1662559 - Part 6: Remove the unnecessary constStart field from ClassBodyScope data. r=mgaudet

Depends on D108287

Comment 7

[Jason Orendorff [:jorendorff]]

Attachment: Bug 1662559 - Part 5: Add .privateBrand to class body scopes. r=arai

Not every class gets this special binding. It will be needed for all classes
that have any nonstatic private methods, getters, or setters. With this patch,
we create a .privateBrand binding in the scopes for such classes, and populate
it with a symbol when initializing the class. However, it is not yet being used
anywhere; the constructor does not yet stamp instances with the private brand.

Depends on D108288

Comment 8

[Jason Orendorff [:jorendorff]]

Attachment: Bug 1662559 - Part 6: Stamp the private brand on instances. r=arai

This adds a hasPrivateBrand bit to the MemberInitializers struct in the
stencil. It is necessary to persist this bit across compilation units because
emitInitializeInstanceMembers can be called in direct eval code inside a
derived-class constructor.

Since .privateBrand is currently a let binding, this currently emits an
unnecessary CheckAliasedLexical instruction in the constructor for each class
that has nonstatic private methods. We'll eliminate these later on.

Depends on D108289

Comment 9

[Jason Orendorff [:jorendorff]]

Attachment: Bug 1662559 - Part 7: Add BindingKind::Synthetic, a kind of binding for pseudo-bindings. r=arai

This is currently used only by the bindings in a ClassBodyScope.

The behavior of these bindings is like Var bindings. There is no longer a TDZ
check, and we don't bother marking them as Uninitialized. So the special case
introduced in bug 1683746, peeking at the binding name to see if we should skip
the TDZ check, is no longer needed.

Depends on D108290

Comment 10

[Jason Orendorff [:jorendorff]]

Attachment: Bug 1662559 - Part 8: Add BindingKind::PrivateMethod. r=arai

This is how the frontend will later identify private methods in order to emit
special bytecode.

Depends on D108291

Comment 11

[Jason Orendorff [:jorendorff]]

Attachment: Bug 1662559 - Part 9: Migrate all ClassBodyScope bindings to be Synthetic. r=arai

This affects bytecode, as Synthetic bindings do not get TDZ checks.

This patch also tightens up some assertions when creating ScopeData. There are
several methods that create ScopeData, and they had gotten out of sync. The
assertions check that the ScopeData is not silently dropping bindings that were
present in ParseContext::Scope::declared_.

Depends on D108292

Comment 12

[Jason Orendorff [:jorendorff]]

Attachment: Bug 1662559 - Part 10: Propagate BindingKind::PrivateMethod to scope data. r=arai

This would cause private methods to be marked as such on scopes, except that
the parser still declares them as DeclarationKind::Synthetic for now.

Depends on D108293

Comment 13

[Jason Orendorff [:jorendorff]]

Attachment: Bug 1662559 - Part 11: Move .privateBrand to a fixed location and add BytecodeEmitter::lookupPrivate() to find it. r=arai

Depends on D108294

Comment 14

[Jason Orendorff [:jorendorff]]

Attachment: Bug 1662559 - Part 14: Minor refactor for BytecodeEmitter::emitSetOrInitializeDestructuring. r=arai

The diff is a mess, but this is very straightforward--we have a combination of
if-statements and a switch that could just be a single switch.

Depends on D108295

Comment 15

[Jason Orendorff [:jorendorff]]

Attachment: Bug 1662559 - Part 15: Add AST node types for private member access expressions (`obj.#member`). r=arai

The new parse nodes then need to be handled throughout the BytecodeEmitter. No
change in behavior.

Depends on D108296

Comment 16

[Jason Orendorff [:jorendorff]]

Attachment: Bug 1662559 - Part 16: Add PrivateOpEmitter, based on PropOpEmitter. r=arai

Depends on D108297

Comment 17

[Jason Orendorff [:jorendorff]]

Attachment: Bug 1662559 - Part 17: Change BytecodeEmitter to use the new PrivateOpEmitter class. r=arai

Depends on D108298

Comment 18

[Jason Orendorff [:jorendorff]]

Attachment: Bug 1662559 - Part 18: Remove all support for private members from ElemOpEmitter. r=arai

Depends on D108299

Comment 19

[Jason Orendorff [:jorendorff]]

Attachment: Bug 1662559 - Part 19: Emit DeclarationKind::PrivateName declarations and use all the optimized paths in PrivateOpEmitter. r=arai

At the same time, the initialization and storage of private methods is changed
to what the PrivateOpEmitter expects.

There are some barely-observable differences in behavior, resulting in two jit-test changes:

• fields/bug1683784.js - An error message changed. Not really better or worse.

• js/src/jit-test/tests/parser/script-source-extent.js - The debugger is able
  to observe initializers as individual scripts. We are no longer using
  initializers for private methods that can be optimized, and the debugger
  therefore observes the change. I adjusted the test to expect the new
  behavior.

Two jit-tests fail:

• fields/private-eval-in-frame.js - The very dynamic scopes in which
  Frame.prototype.eval runs do not pass through static information about
  private methods, so an assertion in lookupPrivate fails. One possible
  workaround would be to ban the use of private fields and methods in
  eval-in-frame code.

• debug/private-methods-eval-in-frame.js - Same.

All js/src/tests pass.

Depends on D108300

Comment 20

[Jason Orendorff [:jorendorff]]

Attachment: Bug 1662559 - Part 20: Tweak some comments in NameOpEmitter.h. r=arai

Depends on D108301

Comment 21

[Jason Orendorff [:jorendorff]]

And here's a Try run with failures in the two tests mentioned in comment 19.

https://treeherder.mozilla.org/jobs?repo=try&revision=d8b06479acc917552bd5bb1f3788868ba16f0c86&selectedTaskRun=IBLjprjoTDSkZaZ3_F-i8w.0

Comment 22

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

Attachment: Bug 1662559 - Part 12: Modify effectiveScopePrivateFieldCache to store NameLocations as well. r=arai

By storing NameLocations we also get a BindingKind, which allows us to disambiguate between
PrivateMethods and Synthetics.

Depends on D108295

Comment 23

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

Attachment: Bug 1662559 - Part 13: Disallow invocation of private methods inside of evalInFrame r=arai

In order to correctly support this with the optimized storage for methods we
would need new bytecode like GetAliasedVar which is able to traverse
non-EnvironmentObject environments like DebugEnvironmentProxy.

Depends on D109401

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/88203b6f2e5a
Part 1: Rename NameLocation::fromBinding() -> BindingIter::nameLocation(). r=arai
https://hg.mozilla.org/integration/autoland/rev/8970e82f0312
Part 2: Clone LexicalScope{Node} -> ClassBodyScope{Node}. r=arai
https://hg.mozilla.org/integration/autoland/rev/547e0b38d66e
Part 3: Tighten up type of the ClassMemberList node kind. r=arai
https://hg.mozilla.org/integration/autoland/rev/ae45d407929f
Part 4: Introduce ClassBodyLexicalEnvironmentObject. r=arai
https://hg.mozilla.org/integration/autoland/rev/e200366e7e3a
Part 5: Add .privateBrand to class body scopes. r=arai
https://hg.mozilla.org/integration/autoland/rev/0b01a60bb702
Part 6: Stamp the private brand on instances. r=arai
https://hg.mozilla.org/integration/autoland/rev/3902d4cb84cc
Part 7: Add BindingKind::Synthetic, a kind of binding for pseudo-bindings. r=arai
https://hg.mozilla.org/integration/autoland/rev/70d767bf04ad
Part 8: Add BindingKind::PrivateMethod. r=arai
https://hg.mozilla.org/integration/autoland/rev/1420d94f0d59
Part 9: Migrate all ClassBodyScope bindings to be Synthetic. r=arai
https://hg.mozilla.org/integration/autoland/rev/0b14a97cd0fb
Part 10: Propagate BindingKind::PrivateMethod to scope data. r=arai
https://hg.mozilla.org/integration/autoland/rev/1bac4e440311
Part 11: Move .privateBrand to a fixed location and add BytecodeEmitter::lookupPrivate() to find it. r=arai
https://hg.mozilla.org/integration/autoland/rev/0f5bcb3ca09f
Part 12: Modify effectiveScopePrivateFieldCache to store NameLocations as well. r=arai
https://hg.mozilla.org/integration/autoland/rev/118b19a448fa
Part 13: Disallow invocation of private methods inside of evalInFrame r=arai
https://hg.mozilla.org/integration/autoland/rev/cc3a2fadee36
Part 14: Minor refactor for BytecodeEmitter::emitSetOrInitializeDestructuring. r=arai
https://hg.mozilla.org/integration/autoland/rev/405a80f38f5c
Part 15: Add AST node types for private member access expressions (`obj.#member`). r=arai
https://hg.mozilla.org/integration/autoland/rev/be3069057f5a
Part 16: Add PrivateOpEmitter, based on PropOpEmitter. r=arai
https://hg.mozilla.org/integration/autoland/rev/bd328ac14770
Part 17: Change BytecodeEmitter to use the new PrivateOpEmitter class. r=arai
https://hg.mozilla.org/integration/autoland/rev/5e4ca6c62d4b
Part 18: Remove all support for private members from ElemOpEmitter. r=arai
https://hg.mozilla.org/integration/autoland/rev/0fbfd31e0f78
Part 19: Emit DeclarationKind::PrivateName declarations and use all the optimized paths in PrivateOpEmitter. r=arai
https://hg.mozilla.org/integration/autoland/rev/4c69d3aabee4
Part 20: Tweak some comments in NameOpEmitter.h. r=arai

Comment 25

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/88203b6f2e5a
https://hg.mozilla.org/mozilla-central/rev/8970e82f0312
https://hg.mozilla.org/mozilla-central/rev/547e0b38d66e
https://hg.mozilla.org/mozilla-central/rev/ae45d407929f
https://hg.mozilla.org/mozilla-central/rev/e200366e7e3a
https://hg.mozilla.org/mozilla-central/rev/0b01a60bb702
https://hg.mozilla.org/mozilla-central/rev/3902d4cb84cc
https://hg.mozilla.org/mozilla-central/rev/70d767bf04ad
https://hg.mozilla.org/mozilla-central/rev/1420d94f0d59
https://hg.mozilla.org/mozilla-central/rev/0b14a97cd0fb
https://hg.mozilla.org/mozilla-central/rev/1bac4e440311
https://hg.mozilla.org/mozilla-central/rev/0f5bcb3ca09f
https://hg.mozilla.org/mozilla-central/rev/118b19a448fa
https://hg.mozilla.org/mozilla-central/rev/cc3a2fadee36
https://hg.mozilla.org/mozilla-central/rev/405a80f38f5c
https://hg.mozilla.org/mozilla-central/rev/be3069057f5a
https://hg.mozilla.org/mozilla-central/rev/bd328ac14770
https://hg.mozilla.org/mozilla-central/rev/5e4ca6c62d4b
https://hg.mozilla.org/mozilla-central/rev/0fbfd31e0f78
https://hg.mozilla.org/mozilla-central/rev/4c69d3aabee4

Comment 26

[Sandor Molnar[:smolnar]]

Backed out for fuzzing failures

Backout link: https://hg.mozilla.org/integration/autoland/rev/a0de4a88ac294b26a83f3776bb110d53405bde6a

Comment 27

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backout merged: https://hg.mozilla.org/mozilla-central/rev/a0de4a88ac29

Comment 28

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

Attachment: Bug 1662559 - Part 4 Fixup: Fix missed shared implementation of lexical scope r?arai

(This will be rolled into Part 4 for landing)

Comment 29

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

Attachment: Bug 1662559 - Part 9 Fixup: Correct initialization pathway for synthetic/private methods r?arai

The intention of the Patch stack, among other things, is to separate the
handling of regular lexical bindings from those that are synthetic and private
methods; things never exposed to or handled in script. As a result of this
divergence, a case in the NameOpEmitter was missed: here we emit SetLocal
instead of the expected InitializeLexical (well, as expected as can be without
thinking about a potentially more accurate bytecode called
InitializeSyntheticOrPrivate)

We also handle EnvironmentCoordinates in the same way

This will be rolled into Part 9 for landing.

Depends on D108293

Comment 30

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

Attachment: Bug 1662559 - Part 19 Fixup: Update stack depth calculations to handle more complex expression types, and error situations. r?arai

This will be rolled into Part 19 for landing.

Note: This is the fixup patch I am least comfortable with. I am open to the
suggestion that maybe the -correct- action here is to break apart
PrivateOpEmitter::emitGet(), which appears to have far too complex a semantics
to reason about.

Depends on D108301

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/86392a2f9d18
Part 1: Rename NameLocation::fromBinding() -> BindingIter::nameLocation(). r=arai
https://hg.mozilla.org/integration/autoland/rev/a7ffffabe660
Part 2: Clone LexicalScope{Node} -> ClassBodyScope{Node}. r=arai
https://hg.mozilla.org/integration/autoland/rev/235c9126589a
Part 3: Tighten up type of the ClassMemberList node kind. r=arai
https://hg.mozilla.org/integration/autoland/rev/d3b226ecc677
Part 4: Introduce ClassBodyLexicalEnvironmentObject. r=arai
https://hg.mozilla.org/integration/autoland/rev/dd28bf5beaf8
Part 5: Add .privateBrand to class body scopes. r=arai
https://hg.mozilla.org/integration/autoland/rev/5ce850a6e268
Part 6: Stamp the private brand on instances. r=arai
https://hg.mozilla.org/integration/autoland/rev/0954e98760d0
Part 7: Add BindingKind::Synthetic, a kind of binding for pseudo-bindings. r=arai
https://hg.mozilla.org/integration/autoland/rev/58e84f08f369
Part 8: Add BindingKind::PrivateMethod. r=arai
https://hg.mozilla.org/integration/autoland/rev/7045f0aa9352
Part 9: Migrate all ClassBodyScope bindings to be Synthetic. r=arai
https://hg.mozilla.org/integration/autoland/rev/fafe4be45090
Part 10: Propagate BindingKind::PrivateMethod to scope data. r=arai
https://hg.mozilla.org/integration/autoland/rev/462358e96220
Part 11: Move .privateBrand to a fixed location and add BytecodeEmitter::lookupPrivate() to find it. r=arai
https://hg.mozilla.org/integration/autoland/rev/2ae9723b04dc
Part 12: Modify effectiveScopePrivateFieldCache to store NameLocations as well. r=arai
https://hg.mozilla.org/integration/autoland/rev/686e81e46bfb
Part 13: Disallow invocation of private methods inside of evalInFrame r=arai
https://hg.mozilla.org/integration/autoland/rev/4082f63c554a
Part 14: Minor refactor for BytecodeEmitter::emitSetOrInitializeDestructuring. r=arai
https://hg.mozilla.org/integration/autoland/rev/75ca82a2e26a
Part 15: Add AST node types for private member access expressions (`obj.#member`). r=arai
https://hg.mozilla.org/integration/autoland/rev/8dd759f91922
Part 16: Add PrivateOpEmitter, based on PropOpEmitter. r=arai
https://hg.mozilla.org/integration/autoland/rev/051adeafc417
Part 17: Change BytecodeEmitter to use the new PrivateOpEmitter class. r=arai
https://hg.mozilla.org/integration/autoland/rev/a48c2c09a753
Part 18: Remove all support for private members from ElemOpEmitter. r=arai
https://hg.mozilla.org/integration/autoland/rev/c4c9cea546f9
Part 19: Emit DeclarationKind::PrivateName declarations and use all the optimized paths in PrivateOpEmitter. r=arai
https://hg.mozilla.org/integration/autoland/rev/bc1d34f55b6b
Part 20: Tweak some comments in NameOpEmitter.h. r=arai

Comment 32

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/86392a2f9d18
https://hg.mozilla.org/mozilla-central/rev/a7ffffabe660
https://hg.mozilla.org/mozilla-central/rev/235c9126589a
https://hg.mozilla.org/mozilla-central/rev/d3b226ecc677
https://hg.mozilla.org/mozilla-central/rev/dd28bf5beaf8
https://hg.mozilla.org/mozilla-central/rev/5ce850a6e268
https://hg.mozilla.org/mozilla-central/rev/0954e98760d0
https://hg.mozilla.org/mozilla-central/rev/58e84f08f369
https://hg.mozilla.org/mozilla-central/rev/7045f0aa9352
https://hg.mozilla.org/mozilla-central/rev/fafe4be45090
https://hg.mozilla.org/mozilla-central/rev/462358e96220
https://hg.mozilla.org/mozilla-central/rev/2ae9723b04dc
https://hg.mozilla.org/mozilla-central/rev/686e81e46bfb
https://hg.mozilla.org/mozilla-central/rev/4082f63c554a
https://hg.mozilla.org/mozilla-central/rev/75ca82a2e26a
https://hg.mozilla.org/mozilla-central/rev/8dd759f91922
https://hg.mozilla.org/mozilla-central/rev/051adeafc417
https://hg.mozilla.org/mozilla-central/rev/a48c2c09a753
https://hg.mozilla.org/mozilla-central/rev/c4c9cea546f9
https://hg.mozilla.org/mozilla-central/rev/bc1d34f55b6b