953024 - JSOP_GETGNAME should be able to do the sorts of getter optimizations JSOP_GETPROP can in Ion

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Boris Zbarsky [:bzbarsky]]

For a JSOP_GETPROP we end up calling getPropTryCommonGetter and whatnot, which will emit various fast paths for DOM getters.

For a JSOP_NAME, on the other hand, we always fall into IonBuilder::jsop_getname which just uses an MGetNameCache.  This means that bareword lookups on the global won't actually benefit from things like bug 935855 or any of the other DOM getter bits Ion knows about.  That's bad for the case of bareword "document".

Ideally, we'd be able to emit DOM getter calls even in this case, but I assume that depends on making sure we catch scope chain mutations that start shadowing things or whatnot.

On the bright side, I believe the only way you can start shadowing for normal scope chain cases (that is, ones where the only things on the scope chain are closure scopes and the global) is via eval in one of the closure scopes and that's a rare enough operation that something like reshaping the global and its proto chain in that situation, just like we'd reshape stuff up the proto chain on property adds on a proto object, might be ok.  Not sure whether that makes it simpler to detect at ion-compile time that we know exactly where the property will be found, like we apparently can for JSOP_GETPROP.

Comment 1

[Boris Zbarsky [:bzbarsky]]

One other note.  If it helps, the DOM stuff lives as own props on the window nowadays so we don't necessarily need to optimize accesses to things on the proto chain of scope chain objects.  Not sure if that makes things simpler.

Comment 2

[Brian Hackett [Laid off!]]

If the op is a JSOP_GETGNAME or JSOP_CALLGNAME then the lookup will be done on the global object rather than looking for any shadowing on the scope chain.  Unless there is an eval or with or other junk involved, bare 'document' uses will be a JSOP_GETGNAME.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Right you are.  I hadn't realized we statically decided on getgname at compile time even inside functions!

In that case, this can probably just be wontfixed.

Comment 4

[Brian Hackett [Laid off!]]

Well, no, there's still stuff to do here.  Ion compilation of JSOP_GETGNAME either tries to use getStaticName, which only optimizes plain data property accesses, and if that fails it falls back to the same treatment as JSOP_NAME.  It should still be trying to optimize DOM getter calls on global names, but there isn't anything special to worry about wrt shadowing on the scope chain, it's just like a property read on the global.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Though given that. I'm a bit confused.  I'm definitely seeing us end up with a NameIC for "document" right now, even though at the moment it should be a plain data property access.  I'll dig into what's going on there, I guess.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Oh, window.document is non-configurable, but getStaticName for some reason refuses to get such properties (why?).  So we end up taking the IC path instead.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Jan, why can't getStaticName work on non-configurable or non-enumerable properties, exactly?

Comment 8

[Jan de Mooij [:jandem]]

(In reply to Vacation Dec 19 to Jan 1 from comment #7)
> Jan, why can't getStaticName work on non-configurable or non-enumerable
> properties, exactly?

Is it failing because "property.configured(constraints())" returns true?

The TI configured-flag is set whenever a property is something other than a plain data property. It means the property was deleted, given a getter/setter, made non-writable etc. IonBuilder::setStaticName for instance relies on this to prevent overwriting a non-writable property.

According to the web console, window.document is non-writable, right? That's probably why it has the configured flag. Maybe we could add a separate "writable" flag and check it in setStaticName and other places, but still allow fast property reads in getStaticName.

Comment 9

[Jeff Walden [:Waldo]]

Given that properties are described as configurable or non-configurable, using the description "configured" here seems unnecessarily misleading.  I *think* "settable" captures the desired intent as best I read it here, that the property can be assigned to without error (unless a setter decides to throw) -- that might be be a better term to use.

Comment 10

[Boris Zbarsky [:bzbarsky]]

> Is it failing because "property.configured(constraints())" returns true?

Yes, correct.  Right now, window.document is a non-writable, non-configurable data property.

I guess I won't worry too much about that case, since long-term window.document will in fact be a getter prop.  Just wanted to understand why the check is there at all.

Comment 11

[Boris Zbarsky [:bzbarsky]]

So I've tried the following, using this testcase in browser:

  var count = 100000;
  for (var i = 0; i < count; ++i)
    performance;

1) Replaced the "return succeeded || jsop_getname(name);" bit in the gname case with:

        if (succeeded)
            return true;
        MConstant *insGlobal = constant(ObjectValue(*obj));
        current->push(insGlobal);
        return jsop_getprop(name);

2) Fixed BaselineInspector::maybeShapesForPropertyOp to deal with having a stub->isGetName_Fallback() at the end without crashing (by testing stub->isSetProp_Fallback() before doing toSetProp_Fallback()).

3) Changed TryAttachNativeGetPropStub to take a ICMonitoredFallbackStub as "stub".  Same for UpdateExistingGenerationalDOMProxyStub.

4) Changed DoGetNameFallback to do this before TryAttachGlobalNameStub:

        RootedValue val(cx, ObjectValue(*scopeChain));
        bool attachedGetProp = false;
        if (!TryAttachNativeGetPropStub(cx, script, pc, stub, name, val, res,
                                        &attachedGetProp))
            return false;
        if (attachedGetProp)
            return true;

(yes, hacky, I know), so that Ion will have some shape info and whatnot.

With all that, we land in js::jit::TryAttachNativeGetPropStub, it calls IsCacheableGetPropCall, and that returns false because it only does getters on the proto chain, not on the object itself.  And then in IonBuilder::getPropTryCommonGetter when we do inspector->commonGetPropFunction we of course get nothing, so we can't optimize this.

I'm not sure whether this (basically making gname ops take the exact same codepath as getprop ops) is even the right approach, but if it is we need to fix getprop for the own property with getter case.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: Hack I just described

Comment 13

[Brian Hackett [Laid off!]]

(In reply to Jan de Mooij [:jandem] from comment #8)
> (In reply to Vacation Dec 19 to Jan 1 from comment #7)
> > Jan, why can't getStaticName work on non-configurable or non-enumerable
> > properties, exactly?
> 
> Is it failing because "property.configured(constraints())" returns true?
> 
> The TI configured-flag is set whenever a property is something other than a
> plain data property. It means the property was deleted, given a
> getter/setter, made non-writable etc. IonBuilder::setStaticName for instance
> relies on this to prevent overwriting a non-writable property.
> 
> According to the web console, window.document is non-writable, right? That's
> probably why it has the configured flag. Maybe we could add a separate
> "writable" flag and check it in setStaticName and other places, but still
> allow fast property reads in getStaticName.

I just put a patch for this in bug 956072.  This discussion is kind of getting side tracked though and optimizing accesses on non-writable data properties is a separate issue from the problem in this bug.

Comment 14

[Eric Faust [:efaust]]

Attachment: Part 0: Factor out common getter stub logic for own-property getter stubs in BC

Comment 15

[Eric Faust [:efaust]]

Attachment: Part 1: Implement Own getter cacheing in BC, and use it for JSOP_GETGNAME

This patch is pretty busy. It can be split into the two parts, if that's necessary.

It intends to do the following:

1) Rename GetProp_CallNative to GetProp_CallNativePrototype
2) Implement GetProp_CallNative for own property getters
3) Use a GetProp_CallNative stub to cache own property native getters in TryAttachGlobalNameStub().

Note a few things:

1) At present, we cannot use IsCacheableGetPropCall() to check in the name stub stuff, since it doesn't do own property getters. (I have a followup that I will post shortly to fix this)
2) The inputDefinitelyObject_ business deals with the fact that the global is passed directly in R0.scratchReg(), and so to make it useful for later GetProp sites, I have made it do both.
(Please pay some attention to the register set manipulations there. I think it should be "OK", but I'm interested in suggestions to make it cleaner/better)

Comment 16

[Eric Faust [:efaust]]

Attachment: Part 2: Make Ion do the common getter optimization for GETGNAME

Comment 17

[Eric Faust [:efaust]]

Attachment: Part 1 v2: Implement Own getter cacheing in BC, and use it for JSOP_GETGNAME

Turns out the whole inputDefinitelyObject thing doesn't work at all without a getKey function. This patch adds one to the previous.

The same caveats still apply. Please read above comment.

Comment 18

[Kannan Vijayan [:djvj]]

Comment on attachment 8390892 [details] [diff] [review]
Part 0: Factor out common getter stub logic for own-property getter stubs in BC

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

Looks good.

::: js/src/jit/BaselineIC.cpp
@@ +358,5 @@
>          break;
>        }
>        case ICStub::GetProp_CallScripted: {
>          ICGetProp_CallScripted *callStub = toGetProp_CallScripted();
> +        MarkShape(trc, &callStub->receiverShape(), "baseline-getpropcallscripted-stub-shape");

Nit: change string to "baseline-getpropcallscripted-stub-receivershape"

@@ +366,5 @@
>          break;
>        }
>        case ICStub::GetProp_CallNative: {
>          ICGetProp_CallNative *callStub = toGetProp_CallNative();
> +        MarkShape(trc, &callStub->receiverShape(), "baseline-getpropcallnative-stub-shape");

Nit: change string to "baseline-getpropcallnative-strub-receivershape"

::: js/src/jit/BaselineIC.h
@@ +4370,1 @@
>      // Object shape (lastProperty).

Nit: remove comment.  Just having |holder_| and |holderShape_| fields next to each other is self-explanatory.

@@ +4461,5 @@
> +        }
> +    };
> +};
> +
> +// Stub for calling a scripted getter on a native object.

Nit: fix comment.  "Stub for calling a scripted getter on a native object, when the getter is kept on the proto-chain"

@@ +4508,5 @@
>          }
>      };
>  };
>  
>  // Stub for calling a native getter on a native object.

Nit: fix comment as for above.  This is for calling native getter when the getter is on the proto-chain.

Comment 19

[Jan de Mooij [:jandem]]

Comment on attachment 8390899 [details] [diff] [review]
Part 2: Make Ion do the common getter optimization for GETGNAME

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

Looks good. Please make sure we have a jit-test to test this code in the shell though (maybe we can add a simple getter to the shell global if it doesn't have one, should also help the fuzzers).

r=me with that.

::: js/src/jit/IonBuilder.cpp
@@ +6415,5 @@
> +    types::TemporaryTypeSet *types = bytecodeTypes(pc);
> +    // Spoof the stack to call into the getProp path.
> +    // First, make sure there's room.
> +    if (current->stackDepth() == current->nslots()) {
> +        if (!current->increaseSlots(1))

Nit: this pattern appears somewhere else too, maybe add a new method to MBasicBlock so that we can pass it the number of slots we want to push and it can call increaseSlots if needed.

Comment 20

[Kannan Vijayan [:djvj]]

Comment on attachment 8391009 [details] [diff] [review]
Part 1 v2: Implement Own getter cacheing in BC, and use it for JSOP_GETGNAME

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

Nice job.

Comment 21

[Eric Faust [:efaust]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e577832d8893
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c2eb9eda1ebe
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/372fcc7c1444

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e577832d8893
https://hg.mozilla.org/mozilla-central/rev/c2eb9eda1ebe
https://hg.mozilla.org/mozilla-central/rev/372fcc7c1444