1063878 - TryAttachGlobalNameStub doesn't handle properties on the global's proto chain

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Boris Zbarsky [:bzbarsky]]

I ran into this when measuring the speed of performance.now() in workers.  We don't take the Ion fast path for the "performance" getter, because Baseline doesn't IC this get (since the prop is on a proto of the global) and then Ion can't sniff the IC to find the common getter.

Might be worth fixing this.

Comment 1

[Johannes Schulte [:jschulte]]

Attachment: v1.patch

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Worker script

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: HTML testcase

Comment 4

[Boris Zbarsky [:bzbarsky]]

So that patch doesn't quite help, because globals always have a resolve hook (to call JS_ResolveStandardClass), so IonBuilder::objectsHaveCommonPrototype ends up returning false, and we never end up doing the fast-path Ion optimizations.

I assume I should file a separate bug on that interaction?

Comment 5

[Jan de Mooij [:jandem]]

Comment on attachment 8496069 [details] [diff] [review]
v1.patch

Review of attachment 8496069 [details] [diff] [review]:
-----------------------------------------------------------------

Nice work! Looks good at first glance.

Can you build an opt browser with/without the patch, disable Ion in about:config (javascript.options.ion) and get some micro-benchmark numbers, to make sure the patch works as expected? :)

::: js/src/jit/BaselineIC.cpp
@@ +5674,5 @@
> +    RootedObject current(cx, global);
> +    while (true) {
> +        if (shape = current->nativeLookup(cx, id))
> +            break;
> +        RootedObject proto(cx, current->getProto());

Nit: just use JSObject *proto = ...; as it's clear that the code can't GC.

@@ +5701,5 @@
> +        } else {
> +            bool isFixedSlot;
> +            uint32_t offset;
> +            GetFixedOrDynamicSlotOffset(current, shape->slot(), &isFixedSlot, &offset);
> +            if(!IsCacheableGetPropReadSlot(global, current, shape))

Nit: space between 'if' and '('

Comment 6

[Boris Zbarsky [:bzbarsky]]

With ion off, the attached testcase takes about 90ns per call with the patch for me.  Without the patch, it takes about 480ns.

With ion on, both take about 51ns.  

If I hack around bug 1073766 (which I filed on comment 4), the Ion times go down to about 37ns, of which only 2ns is the .performance; the rest is the .now() call.

So this patch seems to be working quite nicely.  ;)

Comment 7

[Johannes Schulte [:jschulte]]

Attachment: v2.patch

Thanks Jan for the quick feedback and Boris for the numbers.

Comment 8

[Jan de Mooij [:jandem]]

Comment on attachment 8497734 [details] [diff] [review]
v2.patch

Review of attachment 8497734 [details] [diff] [review]:
-----------------------------------------------------------------

Johannes, sorry for the delay! This looks great, just a few minor comments below. If you can post an updated patch I'll review it (faster) and send it to Try.

Can you also add a jit-test that exercises these cases? You can use Object.getPrototypeOf(this) to get the global's proto, then you can add properties to it and access them. Let me know if you need help with this.

::: js/src/jit/BaselineIC.cpp
@@ +5681,5 @@
> +    RootedObject current(cx, global);
> +    while (true) {
> +        if (shape = current->nativeLookup(cx, id))
> +            break;
> +        JSObject *proto =  current->getProto();

Nit: rm space between = and 'current'.

@@ +5691,3 @@
>      // Instantiate this global property, for use during Ion compilation.
>      if (IsIonEnabled(cx))
> +        types::EnsureTrackPropertyTypes(cx, current, NameToId(name));

Nit: pre-existing, but you can use |id| here instead of NameToId(name).

@@ +6845,5 @@
>      if (!isFixedSlot_) {
> +        if (!inputDefinitelyObject_) {
> +            regs.add(R0);
> +            regs.takeUnchecked(objReg);
> +        }

Why do we need this now?

Comment 9

[Johannes Schulte [:jschulte]]

Attachment: v3.patch

I'm not too happy with the test for the getter-on-proto-case, any other idea?

Comment 10

[Johannes Schulte [:jschulte]]

(In reply to Jan de Mooij [:jandem] from comment #8)
> @@ +6845,5 @@
> >      if (!isFixedSlot_) {
> > +        if (!inputDefinitelyObject_) {
> > +            regs.add(R0);
> > +            regs.takeUnchecked(objReg);
> > +        }
> 
> Why do we need this now?
Good question :) I don't remember totally, but looking at the code, I thought probably something like:
on x86:
eax R0-type
ebx R0-payload
ecx scratch
edx holderReg
esi tailcallreg
-> no register for nextHolder -> add R0-type.
But in fact, I don't take more registers than before this patch and it seems to work fine atm, so I removed this part.
Would you mind explaining me, what I'm missing ?

Comment 11

[Johannes Schulte [:jschulte]]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=b0be5add5816

Comment 12

[Jan de Mooij [:jandem]]

(In reply to Johannes Schulte [:jschulte] from comment #10)
> Good question :) I don't remember totally, but looking at the code, I
> thought probably something like:
> on x86:
> eax R0-type
> ebx R0-payload
> ecx scratch
> edx holderReg
> esi tailcallreg
> -> no register for nextHolder -> add R0-type.
> But in fact, I don't take more registers than before this patch and it seems
> to work fine atm, so I removed this part.
> Would you mind explaining me, what I'm missing ?

You're right that we have only 5 registers available on x86, but we don't take the TailCallReg here AFAICS. So we have R0 type, R0 payload, scratch, holderReg and that leaves 1 register for nextHolder :)

Comment 13

[Jan de Mooij [:jandem]]

Comment on attachment 8503294 [details] [diff] [review]
v3.patch

Review of attachment 8503294 [details] [diff] [review]:
-----------------------------------------------------------------

Looks perfect, thanks! Sorry again for the review delay.

::: js/src/jit-test/tests/baseline/bug1063878.js
@@ +7,5 @@
> +
> +function main() {
> +    var proto = Object.getPrototypeOf(this);
> +    Object.defineProperty(proto, "x", { value: 5});
> +    // not-scripted getter

Nit: can you also test the scripted getter case?

::: js/src/jit/BaselineIC.cpp
@@ +5682,5 @@
> +    RootedShape shape(cx);
> +    RootedNativeObject current(cx, global);
> +    while (true) {
> +        if (shape = current->lookup(cx, id))
> +            break;

Nit: in case you didn't notice, the Try-push failed because some compilers warn if there are no extra parentheses around the assignment. You can also just do:

shape = ...;
if (shape)
    break;

Comment 14

[Johannes Schulte [:jschulte]]

Attachment: v4.patch

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=3b73b507876e

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ede829d6aa61

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ede829d6aa61