Closed Bug 1694044 Opened 1 year ago Closed 1 year ago

Cleanup js::LookupProperty and friends

Categories

(Core :: JavaScript Engine, task, P3)

task

Tracking

()

RESOLVED FIXED
88 Branch
Tracking Status
firefox-esr78 --- wontfix
firefox86 --- wontfix
firefox87 --- wontfix
firefox88 --- fixed

People

(Reporter: tcampbell, Assigned: tcampbell)

Details

Attachments

(7 files, 5 obsolete files)

48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review

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.

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

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

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

Attachment #9204442 - Attachment is obsolete: true
Attachment #9204441 - Attachment description: Bug 1694044 - Add PropertyResult::setTypedArrayOutOfRange. r?jandem! → Bug 1694044 - Add additional results to PropertyResult. r?jandem!
Attachment #9204444 - Attachment description: Bug 1694044 - Cleanup LookoutPropertyPure. r?jandem! → Bug 1694044 - Cleanup js::LookoutPropertyPure. r?jandem!
Severity: -- → S4

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.

Attachment #9204444 - Attachment is obsolete: true

Instead use explicit isFound/isNotFound accessors for clarity.

Depends on D105885

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

Summary: Cleanup js::LookupPropertyPure → Cleanup js::LookupProperty and friends
Attachment #9204441 - Attachment description: Bug 1694044 - Add additional results to PropertyResult. r?jandem! → Bug 1694044 - Add additional modes to PropertyResult. r?jandem!

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

Attachment #9204702 - Attachment is obsolete: true

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

These only support NativeObjects, so rename for clarity.

Depends on D106237

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

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

Attachment #9205032 - Attachment is obsolete: true
Attachment #9205033 - Attachment is obsolete: true
Keywords: leave-open
Pushed by tcampbell@mozilla.com:
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
Pushed by tcampbell@mozilla.com:
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
Status: NEW → RESOLVED
Closed: 1 year ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → 88 Branch
You need to log in before you can comment on or make changes to this bug.