1694044 - Cleanup js::LookupProperty and friends

Status: RESOLVED FIXED

(Core :: JavaScript Engine, task, P3)

Description

[Ted Campbell [:tcampbell]]

The current js::LookupPropertyPure code is a little hard to understand all the edges cases it takes into account and this can be simplified by adding specialized methods to PropertyResult and adding more comments.

Comment 1

[Ted Campbell [:tcampbell]]

Attachment: Bug 1694044 - Add additional modes to PropertyResult. r?jandem!

This is a specialized version of setOutOfRange that stores an extra flag to
indicate that the property id is a special TypedArrayIndex (a String that
converts to a Number and back exactly). This lets the caller know that the
prototype chain should not be searched without needing to repeat the
conversion operation.

Depends on D105884

Comment 2

[Ted Campbell [:tcampbell]]

Attachment: Bug 1694044 - Replace PropertyResult::setNonNativeProperty with specialized methods. r?jandem!

Depends on D105885

Comment 3

[Ted Campbell [:tcampbell]]

Attachment: Bug 1694044 - Stop using JS::Result for IsTypedArrayIndex. r?jandem!

The Result<Maybe<T>> construct isn't clearer than our usual return-bool
convention so simplify things by using an out-param. This should also
generate better machine code.

Depends on D105886

Comment 4

[Ted Campbell [:tcampbell]]

Attachment: Bug 1694044 - Cleanup js::LookoutPropertyPure. r?jandem!

Add documentation for some of the edge cases. Move the setNotFound case
within the per-type blocks since accidentally returning this would be a
serious issues and return false as the default is much safer.

Depends on D105887

Comment 5

[Ted Campbell [:tcampbell]]

In future, we should really better structure the TypedArrayObject and ArrayObject special handling throughout the engine. They are truly exotic object behaviours and even if we choose to inline the behaviour instead of using an ObjectOps/ClassOps hooks, we need a better way to organize this code.

Comment 6

[Ted Campbell [:tcampbell]]

Attachment: Bug 1694044 - Remove bool conversion from PropertyResult. r?jandem!

Instead use explicit isFound/isNotFound accessors for clarity.

Depends on D105885

Comment 7

[Ted Campbell [:tcampbell]]

I'll expand the scope of this bug to unify LookupPropertyInline with LookupPropertyPure and cleaning up other related methods.

Comment 8

[Ted Campbell [:tcampbell]]

Attachment: Bug 1694044 - Cleanup js::LookupProperty and friends

Use single implementation of NativeLookupOwnPropertyInline for:

• NativeLookupOwnPropertyNoResolve
• NativeLookupOwnProperty
• LookupOwnPropertyPure

Use single implementation of NativeLookupPropertyInline for:

• LookupPropertyPure

Use ClassMayResolveId instead of just checking for a resolve hook. This is
what the JITs already do.

Remove the TypedObject check from LookupOwnPropertyPure since none of the
callers would have handled it any different than the unknown case.

Replace a few checks for is<NativeObject> in lookup code with a check for the
lookupProperty hook instead. This more accurate and is still a simple lookup
on the JSClass.

Depends on D105885

Comment 9

[Ted Campbell [:tcampbell]]

Attachment: Bug 1694044 - NativeLookupOwnPropertyNoResolve does not need to root. r?jandem!

Depends on D105885

Comment 10

[Ted Campbell [:tcampbell]]

Attachment: Bug 1694044 - Replace NativeLookupOwnProperty<NoGC> with LookupOwnPropertyPure. r?jandem!

This now uses ClassMayResolveId instead of just checking for a resolve
hook. This changes a small number of cases that were fast paths in the VM to
now will invoke the mayResolve hook where they didn't before. This adds a
small penalty when the hook returns false, but in that case we were retrying
the full operation, running resolve hook and defining a property. This
reduces the number of similar implementations of lookup.

Depends on D106235

Comment 11

[Ted Campbell [:tcampbell]]

Attachment: Bug 1694044 - Remove template from NativeLookupOwnProperty. r?jandem!

Depends on D106236

Comment 12

[Ted Campbell [:tcampbell]]

Attachment: Bug 1694044 - Rename LookupPropertyInline / LookupOwnPropertyInline. r?jandem!

These only support NativeObjects, so rename for clarity.

Depends on D106237

Comment 13

[Ted Campbell [:tcampbell]]

Attachment: Bug 1694044 - Merge similar implementations into NativeLookupOwnPropertyInline. r?jandem

Merge the following implementations:

• NativeLookupOwnPropertyInline
• NativeLookupOwnPropertyNoResolve
• LookupOwnPropertyPure

This introduces a mayResolve check where there was not before, but the only
use that would have observed that is LookupNameNoGC which works on
environments that have no resolve hooks to being with. As a result, this is
simply removing duplicate code.

We no longer support anything with a lookupProperty hook. Previously the JITs
would tolerate them, but in practice do nothing useful. One case was for
environment objects which triggered this during SetProp but would later
abort. The other case was TypedObjects which are no longer meaningully
supported by the JITs and are treated no differently than unknown.

NOTE: An earlier patch moved some uses to LookupOwnPropertyPure already and
the justification for why the extra mayResolve hook is fine is listed
there.

Depends on D106238

Comment 14

[Ted Campbell [:tcampbell]]

Attachment: Bug 1694044 - Remove implementation of js::LookupPropertyPure. r?jandem!

Combine implementation of:

• NativeLookupPropertyInline
• LookupPropertyPure

Instead of checking for is<NativeObject>, check for a lookupProperty hook
which is more indicative of what we are worried about.

Depends on D106239

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b7cb1145d886
Stop using JS::Result for IsTypedArrayIndex. r=jandem
https://hg.mozilla.org/integration/autoland/rev/de42fd2add29
Remove bool conversion from PropertyResult. r=jandem
https://hg.mozilla.org/integration/autoland/rev/d577284e2444
Add additional modes to PropertyResult. r=jandem

Comment 16

[Sandor Molnar]

https://hg.mozilla.org/mozilla-central/rev/b7cb1145d886
https://hg.mozilla.org/mozilla-central/rev/de42fd2add29
https://hg.mozilla.org/mozilla-central/rev/d577284e2444

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6658fceaf7ce
NativeLookupOwnPropertyNoResolve does not need to root. r=jandem
https://hg.mozilla.org/integration/autoland/rev/05eda125b2c2
Rename LookupPropertyInline / LookupOwnPropertyInline. r=jandem
https://hg.mozilla.org/integration/autoland/rev/45f8a63e538b
Merge similar implementations into NativeLookupOwnPropertyInline. r=jandem
https://hg.mozilla.org/integration/autoland/rev/2180f01e167c
Remove implementation of js::LookupPropertyPure. r=jandem

Comment 18

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/6658fceaf7ce
https://hg.mozilla.org/mozilla-central/rev/05eda125b2c2
https://hg.mozilla.org/mozilla-central/rev/45f8a63e538b
https://hg.mozilla.org/mozilla-central/rev/2180f01e167c