840576 - BaselineCompiler: Inline caches for NAME / CALLNAME

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brian Hackett [Laid off!]]

The baseline compiler hits stubs for these ops about 65k times on SS (15% of total stubcalls).  NAMEs are more prevalent on SS than other benchmarks because several tests use eval(), which inhibits use of ALIASEDVAR.  Stubbing these opcodes is not only common, but also very expensive as no caching of NAME ops occurs anymore in the VM.  All of the tests which hit these stubs a lot (date-format-tofte, date-format-xparb, string-tagcloud) are much slower with Baseline+Ion than JM+Ion.

Comment 1

[Brian Hackett [Laid off!]]

Attachment: patch

This pretty much wipes out the name/callname stub calls and gives Baseline+Ion a 5.7% speedup on SS.

Comment 2

[Kannan Vijayan [:djvj]]

Comment on attachment 713039 [details] [diff] [review]
patch

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

This would be better with the IC parameterized on the number of hops (and deriving the kind from that).  Comments mostly sketch that out.

Parameterizing on unsigned instead of the kind seems more natural, since the GetName_Scope stub has a natural definitional extension for every nonnegative integer, but not one for every kind (e.g. ICGetName_Scope<50> has some amount of meaning, whereas ICGetName_Scope<BinaryArith_Int32> definitely does not).  Also, that approach seems to allow for easier extension of the stub to handle higher numbers in the future.

::: js/src/ion/BaselineIC.h
@@ +341,5 @@
> +    _(GetName_Scope0)           \
> +    _(GetName_Scope1)           \
> +    _(GetName_Scope2)           \
> +    _(GetName_Scope3)           \
> +    _(GetName_Scope4)           \

We can still encode the number in the kind.

@@ +2523,5 @@
> +// 'own' property off some scope object. Unlike GETPROP on an object's
> +// prototype, there is no teleporting optimization to take advantage of and
> +// shape checks are required all along the scope chain.
> +template <ICStub::Kind StubKind> 
> +class ICGetName_Scope : public ICMonitoredStub

This can become template <unsigned NumHops> class ICGetName_Scope ...

A static method GetStubKind() can switch on NumHops and return the appropriate Kind.

@@ +2534,5 @@
> +    uint32_t offset_;
> +
> +    ICGetName_Scope(IonCode *stubCode, ICStub *firstMonitorStub,
> +                    AutoShapeVector *shapes, uint32_t offset)
> +      : ICMonitoredStub(StubKind, stubCode, firstMonitorStub),

This can be ICMonitoredStub(GetStubKind(), ...)

@@ +2537,5 @@
> +                    AutoShapeVector *shapes, uint32_t offset)
> +      : ICMonitoredStub(StubKind, stubCode, firstMonitorStub),
> +        offset_(offset)
> +    {
> +        JS_STATIC_ASSERT(StubKind >= GetName_Scope0 && StubKind <= GetName_Scope0 + MAX_HOPS);

JS_STATIC_ASSERT(NumHops < MAX_HOPS)

@@ +2545,5 @@
> +    }
> +
> +    static size_t hops() {
> +        return StubKind - GetName_Scope0;
> +    }

This can be a non-static method that computes the hops from actual stub kind:

size_t hops() const {
  JS_ASSERT(kind_ >= ... && kind_ <= ...);
  return kind_ - GetName_Scope0;
}

@@ +2560,5 @@
> +            MarkShape(trc, &shapes_[i], "baseline-scope-stub-shape");
> +    }
> +
> +    static size_t offsetOfShape(size_t index) {
> +        JS_ASSERT(index <= hops());

becomes index <= NumHops

@@ +2584,5 @@
> +
> +      public:
> +        Compiler(JSContext *cx, ICStub *firstMonitorStub,
> +                 AutoShapeVector *shapes, bool isFixedSlot, uint32_t offset)
> +          : ICStubCompiler(cx, StubKind),

StubKind => ICGetName_Scope<NumHops>::GetStubKind()

Comment 3

[Brian Hackett [Laid off!]]

Attachment: updated

Agreed on all counts, I tried that approach first but ran into problems with space->allocate.  In retrospect though I was just screwing something up and ICGetName_Scope<NumHops> works fine.

Comment 4

[Kannan Vijayan [:djvj]]

Comment on attachment 713186 [details] [diff] [review]
updated

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

::: js/src/ion/BaselineIC.cpp
@@ +2866,5 @@
> +
> +        if (!scopeChain->isNative())
> +            return true;
> +
> +        if (scopeChain->isScope()) {

If scopeChain->isGlobal() is used for this conditional (instead of in the main loop body), and switched with the body of this conditional, then the logic flows a bit more naturally - the break occurs within the conditional block, and the "continue" currently at the end of this conditional block can be made implicit when it becomes the last statement of the loop body.

Comment 5

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/projects/ionmonkey/rev/580b7ba7145a