BaselineCompiler: Implement JSOP_GETELEM handlers which invoke getters.

RESOLVED FIXED in mozilla26

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: djvj, Assigned: djvj)

Tracking

unspecified
mozilla26
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
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.
(Assignee)

Updated

5 years ago
Assignee: general → kvijayan
(Assignee)

Comment 1

5 years ago
Created attachment 795481 [details] [diff] [review]
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
Attachment #795481 - Flags: review?(efaustbmo)

Comment 2

5 years ago
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"?
Attachment #795481 - Flags: review?(efaustbmo) → review+
(Assignee)

Comment 3

5 years ago
Created attachment 797500 [details] [diff] [review]
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.
Attachment #795481 - Attachment is obsolete: true
Attachment #797500 - Flags: review?(efaustbmo)

Comment 4

5 years ago
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()?
Attachment #797500 - Flags: review?(efaustbmo) → review+

Comment 7

5 years ago
https://hg.mozilla.org/mozilla-central/rev/9e282f0c00b8
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla26 → ---
(Assignee)

Comment 10

5 years ago
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
https://hg.mozilla.org/mozilla-central/rev/4e6f9d2589dc
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Depends on: 913978
You need to log in before you can comment on or make changes to this bug.