1113014 - Need to follow up on getting rid of LoadDOMPrivate

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Boris Zbarsky [:bzbarsky]]

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...

Comment 1

[Jan de Mooij [:jandem]]

I fixed this as part of bug 1237504.

Comment 2

[Boris Zbarsky [:bzbarsky]]

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.

Comment 3

[Jan de Mooij [:jandem]]

(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?

Comment 4

[Jan de Mooij [:jandem]]

(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.

Comment 5

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 6

[Jan de Mooij [:jandem]]

Attachment: Patch

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).

Comment 7

[Boris Zbarsky [: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

Comment 8

[Pulsebot]

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

Comment 9

[Jan de Mooij [:jandem]]

(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

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/4ef2425934f2