Need to follow up on getting rid of LoadDOMPrivate

RESOLVED FIXED in Firefox 59

Status

()

RESOLVED FIXED
4 years ago
a year ago

People

(Reporter: bzbarsky, Assigned: jandem)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla59
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox59 fixed)

Details

Attachments

(1 attachment)

Before the changes in bug 1073842, getting the C++ object pointer out of a JS DOM object was a simple private value read from slot 0.

Now it's a branchy thing which first reads the shape, then checks for number of fixed slots, then eithe reads the first fixed slot or reads the slots pointer and reads the first slot from there.  Microbenchmarks don't seem to show much performance impact, though those wouldn't be affected by i-cache misses much anyway.

In any case, bug 1073842 comment 29 says this is temporary until bug 966518 is fixed, plus some work on JSObject::swap.  Filing this to actually track this happening...
Blocks: 1128681
Blocks: 1218972
(Assignee)

Comment 1

2 years ago
I fixed this as part of bug 1237504.
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Depends on: 1237504
No longer depends on: 966518
Resolution: --- → DUPLICATE
Duplicate of bug: 1237504
This isn't fixed.  LoadDOMPrivate is still in js/src/jit/CodeGenerator.cpp, is still used from the visit*DOM* bits, and still does various branching and jazz.
Status: RESOLVED → REOPENED
Flags: needinfo?(jdemooij)
Resolution: DUPLICATE → ---
(Assignee)

Comment 3

2 years ago
(In reply to Boris Zbarsky [:bz] (still digging out from vacation mail) from comment #2)
> This isn't fixed.  LoadDOMPrivate is still in js/src/jit/CodeGenerator.cpp,
> is still used from the visit*DOM* bits, and still does various branching and
> jazz.

Ah sorry, I thought this was about the C++ side bug 1237504 changed.

(1) So in LoadDOMPrivate, I wonder if we can rely on the private value being in a fixed slot for native objects. Do you know offhand?

(2) That still requires an IsNative/IsProxy branch, though. One option would be for NativeObject::slots_ to point to the fixed slots, if any, then we could just do obj->slots_[0] and it would work for proxies and non-proxies with/without dynamic slots. Unfortunately that's quite a large refactoring just for this bug.

(3) If (1) holds, we could use TI to check for "definitely (not) a proxy" in IonBuilder and then we could pass that info down to LoadDOMPrivate and eliminate the dead branches/loads.

Does this sound reasonable to you, Boris?
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 4

2 years ago
(In reply to Jan de Mooij [:jandem] from comment #3)
> One option would
> be for NativeObject::slots_ to point to the fixed slots, if any,

Scratch this, it's bogus. I should think before I type.
> I wonder if we can rely on the private value being in a fixed slot for native objects.

Yes.  It used to be that we could rely on this being true for proxies too, until bug 1073842...  But for non-proxies it's definitely true.

Using TI to do the proxy/nonproxy thing sounds like a good idea, yes.
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 6

a year ago
Posted patch PatchSplinter Review
Here's a patch, to get this bug out of my needinfo queue..

This adds a DOMObjectKind enum. This value is returned as outparam from TemporaryTypeSet::isDOMClass and then passed to LoadDOMPrivate codegen.

Then in LoadDOMPrivate, we can take advantage of this information. I changed the "unknown" case here to guard it's a proxy instead of looking at the shape's number of fixed slots - it's a bit easier to understand and is hopefully okay perf-wise since we don't have to emit this check in most cases.

I also added an MGetDOMPropertyBase class, now both MGetDOMProperty and MGetDOMMember inherit from this class, instead of having MGetDOMMember inherit from MGetDOMProperty. That makes it possible to store DOMObjectKind in MGetDOMProperty (and not in MGetDOMMember).
Assignee: nobody → jdemooij
Status: REOPENED → ASSIGNED
Flags: needinfo?(jdemooij)
Attachment #8940755 - Flags: review?(bzbarsky)
Comment on attachment 8940755 [details] [diff] [review]
Patch

>+    bool possiblyCalls() const override {

Why do you need this override (in MGetDOMProperty; it's already overridden in MGetDOMPropertyBase).

r=me
Attachment #8940755 - Flags: review?(bzbarsky) → review+

Comment 8

a year ago
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ef2425934f2
Don't emit a branch in LoadDOMPrivate if we know the object kind (native or proxy) statically. r=bz
(Assignee)

Comment 9

a year ago
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #7)
> Why do you need this override (in MGetDOMProperty; it's already overridden
> in MGetDOMPropertyBase).

Good catch, I removed it from MGetDOMPropertyBase.

Comment 10

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4ef2425934f2
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years agoa year ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.