906805 - BaselineCompiler: Implement JSOP_GETELEM handlers which invoke getters.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Kannan Vijayan [:djvj]]

More fallout from investigating bug 856652.  About 5000 of the fallback hits seen are from looking up '.onclick' event handlers using GETELEM.  The slot is a getter slot, and in this case has a native function for the getter.

Comment 1

[Kannan Vijayan [:djvj]]

Attachment: baseline-getelem-getter.patch

Adds an optimized baseline path for GetElem invoking a getter (either scripted or native).  It also expands the "key check" guard in the GetElem patches to have an additional slow path which uses a VM-call to C++.  Currently, the guards would only catch keys that were interned.  The slow-path change allows GetElem guards to also catch GetElems on keys which are ropes, etc.

The following changes are in the patch:

1. GetElem_Native is renamed to GetElem_NativeSlot.
2. GetElem_NativePrototype is renamed to GetElem_NativePrototypeSlot
3. New stubs GetElem_NativePrototypeCallNative and GetElem_NativePrototypeCallScripted are added.
4. All the GetElem stubs code generation is changed to emit a slow-path in the key-check code to call C++ function to compare strings (if the identity match fails).

The GetElem stub class heirarchy is slightly reorganized into:

GetElemNativeStub - containing the receiver object's lastShape and the id to match.
GetElemNativeSlotStub - base class for _NativeSlot and _NativePrototypeSlot.
GetElemNativeCallStub - base class for _NativePrototypeCallScripted and _NativePrototypeCallNative

Comment 2

[Eric Faust [:efaust]]

Comment on attachment 795481 [details] [diff] [review]
baseline-getelem-getter.patch

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

Code gen sections are especially pretty. Very nice.

r=me with CheckGetElemKey perf concerns addressed.

::: js/src/jit/BaselineIC.cpp
@@ +3763,4 @@
>  //
>  
> +static bool
> +DoCheckGetElemKeySlow(JSContext *cx, ICGetElemNativeStub *stub, HandleValue name, uint32_t *result)

We should ensure that this is actually a net win. It will certainly cover the cases where people create the string index on the fly, allowing more coverage, but I fear repeated checking of the same non-matching strings.

@@ +3783,2 @@
>  bool
>  ICGetElemNativeCompiler::generateStubCode(MacroAssembler &masm)

Though I understand why it is, I kinda wish this function wasn't now immensely long. Can we split some of the if blocks out to smaller, more specialized functions for clarity?

@@ +3852,5 @@
> +        masm.loadValue(BaseIndex(holderReg, scratchReg, TimesOne), R0);
> +
> +        if (popR1)
> +            masm.addPtr(ImmWord(sizeof(size_t)), BaselineStackReg);
> +    } else {

In particular, this else block, as mentioned above, seems like it's calling out for it's own function.

::: js/src/jit/BaselineIC.h
@@ +2842,5 @@
>  
>  class ICGetElemNativeStub : public ICMonitoredStub
>  {
> +  public:
> +    enum Source { FixedSlot = 0, DynamicSlot, NativeGetter, ScriptedGetter };

Is "Source" the best name here? I feel like we have the mathematics problem where every name for a type and a collection already has a meaning. Perhaps "AccessType"?

Comment 3

[Kannan Vijayan [:djvj]]

Attachment: baseline-getelem-getter.patch

Updated patch with comments addressed.  Changes are significant enough to warrant a re-review.

The biggest change is the "slow key check" path.  In the previous patch, the slow key check path existed on every stub.  This has been changed to the following:

1. Every GetElem_Native* stub can be generated with with 'needsAtomize' and '!needsAtomize' variants.

2. When new stubs are added with 'needsAtomize' set to true, any existing, equivalent stubs with 'needsAtomize' set to false are removed.

3. 'needsAtomize' stubs check the input key string, and if the string is identified to be a non-atom, call a VMFunction to convert the string to an atom.  The atomized string returned by the VMFunction is used to replace the original R1 for both that stub and for subsequent stubs.  This means that at most one string-atomization will be performed per IC-chain.

The cost of this atomization will be paid even if we didn't do this, because property lookup in the VM requires atomization anyway.

Comment 4

[Eric Faust [:efaust]]

Comment on attachment 797500 [details] [diff] [review]
baseline-getelem-getter.patch

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

r=me with the SOURCE_MASK fix and renaming |Source|. I like this strategy much better. Thanks for reworking this.

::: js/src/jit/BaselineIC.cpp
@@ +3540,5 @@
> +                         iter->kind() != ICStub::GetElem_NativePrototypeCallNative &&
> +                         iter->kind() != ICStub::GetElem_NativePrototypeCallScripted))
> +        {
> +            continue;
> +        }

nit: This block of if is so ugly, and repeated twice. Can we squirrel it away in like an IsIterableGetElemNativeStub?

@@ +3704,5 @@
> +                        getter->nonLazyScript()->filename(), getter->nonLazyScript()->lineno,
> +                        obj.get(), obj->lastProperty(), holder.get(), holder->lastProperty());
> +        } else {
> +            IonSpew(IonSpew_BaselineIC, "  Generating GetElem(Native %s%s call native) stub "
> +                                        "(obj=%p, shape=%p, holder=%p, holderShape=%p)",

nit: should have identical indentation to the spew above, regardless of which is chosen.

@@ +3988,5 @@
> +
> +    return true;
> +}
> +
> +bool 

nit: trailing whitespace

::: js/src/jit/BaselineIC.h
@@ +2852,5 @@
>      HeapPtrShape shape_;
> +    HeapPtrPropertyName name_;
> +
> +    static const unsigned SOURCE_SHIFT = 1;
> +    static const uint16_t SOURCE_MASK = 0x2;

I think we want 0x3 here, or we are unlikely to get the Source back that we expect

@@ +2855,5 @@
> +    static const unsigned SOURCE_SHIFT = 1;
> +    static const uint16_t SOURCE_MASK = 0x2;
> +
> +    static const unsigned NEEDS_ATOMIZE_SHIFT = 0;
> +    static const uint16_t NEEDS_ATOMIZE_MASK = 0x1;

super-nit: Can we keep the bits in order, top to bottom?

@@ +2879,5 @@
> +        return offsetof(ICGetElemNativeStub, name_);
> +    }
> +
> +    Source source() const {
> +        return static_cast<Source>((extra_ >> SOURCE_SHIFT) & SOURCE_MASK);

See comment above. It's unclear to me how this gives a reliable Source back.

@@ +3171,5 @@
> +                    space, getStubCode(), firstMonitorStub_, shape, name_, src_, needsAtomize_,
> +                    getter_, pcOffset_, holder_, holderShape);
> +        }
> +
> +        return NULL;

this |return NULL| isn't live, right? Perhaps a MOZ_ASSUME_UNREACHED()?

Comment 5

[Kannan Vijayan [:djvj]]

Try looks good:
https://tbpl.mozilla.org/?tree=Try&rev=82d36bef10ea

Comment 6

[Kannan Vijayan [:djvj]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9e282f0c00b8

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/9e282f0c00b8

Comment 8

[Ryan VanderMeulen [:RyanVM]]

I suspect that this is the cause of frequent mochitest-bc crashes like the one below. The first instance I can find conveniently lands on this push and it started on other trees once it was merged around. Backed out (with fingers crossed).
https://hg.mozilla.org/integration/mozilla-inbound/rev/dffedf20a02d

https://tbpl.mozilla.org/php/getParsedLog.php?id=27344286&tree=Mozilla-Inbound

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 10

[Kannan Vijayan [:djvj]]

Fixed and re-pushed.  The issue was that the guard ensuring that the callee was scripted didn't operate on the JSScript, but on the JSFunction, which is wrong.  The patch delta is:

     1.1 --- a/js/src/jit/BaselineIC.cpp
     1.2 +++ b/js/src/jit/BaselineIC.cpp
     1.3 @@ -4211,16 +4211,17 @@ ICGetElemNativeCompiler::generateStubCod
     1.4              emitCallNative(masm, objReg);
     1.5  
     1.6          } else {
     1.7              JS_ASSERT(acctype_ == ICGetElemNativeStub::ScriptedGetter);
     1.8  
     1.9              // Load function in scratchReg and ensure that it has a jit script.
    1.10              masm.loadPtr(Address(BaselineStubReg, ICGetElemNativeGetterStub::offsetOfGetter()),
    1.11                           scratchReg);
    1.12 +            masm.loadPtr(Address(scratchReg, JSFunction::offsetOfNativeOrScript()), scratchReg);
    1.13              masm.loadBaselineOrIonRaw(scratchReg, scratchReg, SequentialExecution,
    1.14                                        popR1 ? &failurePopR1 : &failure);
    1.15  
    1.16              // At this point, we are guaranteed to successfully complete.
    1.17              if (popR1)
    1.18                  masm.addPtr(Imm32(sizeof(size_t)), BaselineStackReg);
    1.19  
    1.20              emitCallScripted(masm, objReg);

Its try run:
https://tbpl.mozilla.org/?tree=Try&rev=f1fc0e01a3eb

Comment 11

[Kannan Vijayan [:djvj]]

Re-push:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e6f9d2589dc

Comment 12

[Ryan VanderMeulen [:RyanVM]]

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