Closed Bug 1209118 Opened 4 years ago Closed 4 years ago

Unify Ion GETPROP and GETELEM IC stubs

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: jandem, Assigned: jandem)

References

Details

Attachments

(12 files)

119.20 KB, patch
Details | Diff | Splinter Review
36.25 KB, patch
efaust
: review+
Details | Diff | Splinter Review
44.69 KB, patch
efaust
: review+
Details | Diff | Splinter Review
14.09 KB, patch
efaust
: review+
Details | Diff | Splinter Review
11.09 KB, patch
efaust
: review+
Details | Diff | Splinter Review
8.78 KB, patch
efaust
: review+
Details | Diff | Splinter Review
19.45 KB, patch
efaust
: review+
Details | Diff | Splinter Review
4.30 KB, patch
efaust
: review+
Details | Diff | Splinter Review
1.20 KB, patch
efaust
: review+
Details | Diff | Splinter Review
26.69 KB, patch
efaust
: review+
Details | Diff | Splinter Review
1.04 KB, patch
efaust
: review+
Details | Diff | Splinter Review
960 bytes, patch
efaust
: review+
Details | Diff | Splinter Review
Baseline/Ion can generate GETPROP and SETPROP stubs for the most common cases, but GETELEM and SETELEM coverage is way worse. We do cache some *ELEM cases and there's some ad-hoc code sharing (and duplication..) to emit GETPROP and GETELEM stubs for native object properties, but only for a few cases.

For instance: Ion's GetPropertyCache can emit a Proxy stub. Ion's GetElementCache doesn't do anything to handle such proxies.

Bad *ELEM caching shows up in the wild (eg on Treeherder, bug 1166184) and also affects symbol properties: we'll always use *ELEM ops for those.

Two ways to fix this:

(1) Move most of the GetPropertyIC logic into a new helper class ("GetPropStubGenerator" or something) that's used by both GetPropertyIC and GetElementIC. For GetElementIC, each stub will emit an extra |key == cachedKey| check and then do the usual GetProperty stuff.

(2) Kill GetPropertyIC and always use GetElementIC (we could rename it of course). To avoid performance regressions, we'd have to make sure we don't emit unnecessary |key == cachedKey| guards if the index is a constant string, but that seems doable.

Personally I really like (2). For Ion (1) would work, but requires some verbosity passing stuff to the new class. For Baseline (2) seems a lot easier than (1). Doing (2) also means obj[constant-string-or-symbol] will get exactly the same treatment as a GETPROP, which seems pretty nice for symbols in particular.
Flags: needinfo?(kvijayan)
Flags: needinfo?(efaustbmo)
Flags: needinfo?(bhackett1024)
I agree with the general sentiment.  The only downside I see is that unifying by making everything go through a GetElem path means that all GetProp stubs (which will now be the same as GetElem stubs) will do an additional name-check, instead of the name being implicit.

I suppose the overhead of IC in the first place (branch => code cache invalidation => target object guards => branch back to mainline code) would drown that out, so it's not a really big issue.  We might even be able to avoid the name guard by having a |bool constantName| parameter to the generator code, which indicates that the name is always going to be the same every time the stub is entered (in baseline, this can be stored as a bit in the |extra| bits on the stub type word), and can be used to omit the name-guards.

The approach is good.
Flags: needinfo?(kvijayan)
(In reply to Kannan Vijayan [:djvj] from comment #1)
> We might even be able to
> avoid the name guard by having a |bool constantName| parameter to the
> generator code, which indicates that the name is always going to be the same
> every time the stub is entered (in baseline, this can be stored as a bit in
> the |extra| bits on the stub type word), and can be used to omit the
> name-guards.

Yep, that's what I was thinking as well (the last sentence of (2)). We could modify Ion's GetElementIC to store the index_ as a ConstantOrRegister, so the IC can easily check if it's a constant and that means there's also no regalloc overhead...

I'll try to prototype this tomorrow and see how it turns out.
(2) sounds good to me.
Flags: needinfo?(bhackett1024)
(In reply to Jan de Mooij [:jandem] from comment #2)
> I'll try to prototype this tomorrow and see how it turns out.

I've a stack of patches to use Ion's GetPropertyIC for GETELEM and it's really nice. For instance, after that it was almost trivial to replace PropertyName* with jsid in all the GetPropertyIC stubs to make them work with symbols. Will upload patches after cleaning them up.
Attached patch Combined patchSplinter Review
Combined patch.

 17 files changed, 550 insertions(+), 763 deletions(-)
Adds an operand to MGetPropertyCache for the property id, right now it's just MConstant(StringValue(name)).

Pretty straight-forward for the most part, but I had to add useBoxOrTypedOrConstant to avoid ending up with 4 LGetPropertyCache* LIR instructions for each combination of {Typed, Value} input and output.

This worked very well and I think we should consider using useBoxOrTypedOrConstant for other LIR instructions that have separate typed and Value versions atm.

(MGetElementCache "cheats" here: if the output is a Value, the id is also boxed and vice versa, so it only needs two LIR instructions, but with useBoxOrTypedOrConstant we can do better than that.)
Flags: needinfo?(efaustbmo)
Attachment #8668297 - Flags: review?(efaustbmo)
This patch changes all GetPropertyIC stubs to work with a Value id. This required:

* Using jsid instead of PropertyName*, so the stubs work with symbols.

* Emitting a guard if the id is not a constant. The code for the string case I mostly stole from GetElementIC. GetElementIC will be removed later so I didn't bother factoring it out.
Attachment #8668298 - Flags: review?(efaustbmo)
This moves the arguments element stub from GetElementIC to GetPropertyIC.

I also converted it to the tryAttach* style we use for other GetPropertyIC stubs, it's more readable.
Attachment #8668299 - Flags: review?(efaustbmo)
Attachment #8668302 - Flags: review?(efaustbmo)
Attachment #8668303 - Flags: review?(efaustbmo)
I also had to move allowDoubleResult from MGetElementCache to MGetPropertyCache.
Attachment #8668305 - Flags: review?(efaustbmo)
Attachment #8668303 - Attachment description: Part 4 - Move dense element hole stub → Part 5 - Move dense element hole stub
GetElementIC can disable itself (remove all stubs and give up on attaching a new one) after it fails to attach a stub or it has too many stubs.

This patch adds that mechanism to GetPropertyIC as well.
Attachment #8668306 - Flags: review?(efaustbmo)
Attachment #8668307 - Flags: review?(efaustbmo)
11 files changed, 0 insertions(+), 445 deletions(-)
Attachment #8668309 - Flags: review?(efaustbmo)
Feel free to just review the combined patch, it might be faster in this case.

And FWIW, the micro-benchmark below demonstrates symbols properties are now optimized just like string properties:

before: 603 ms
after:   13 ms

var sym = Symbol("foo");
var o = {[sym]: 3};

function f() {
    var res = 0;
    for (var i=0; i<10000000; i++) {
	res += o[sym];
    }
    return res;
}
var t = new Date;
f();
print(new Date - t);
FWIW after these patches land, I want to do some more Ion GetPropertyIC cleanup, but I'll do that in a separate bug.
Summary: Unify GETPROP and GETELEM IC stubs → Unify Ion GETPROP and GETELEM IC stubs
Eric, any chance you can get to those patches soonish? If not I can try to find another reviewer. The patches are pretty bitrot-prone and it's also blocking my other IC work (I'm waiting until this is in).
Comment on attachment 8668297 [details] [diff] [review]
Part 1 - Add id operand to MGetPropertyCache

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

I believe so. r=me with questions below answered.

::: js/src/jit/CodeGenerator.cpp
@@ +8473,5 @@
>      addCache(ins, allocateCache(cache));
>  }
>  
> +ConstantOrRegister
> +CodeGenerator::toConstantOrRegister(LInstruction* lir, size_t n, MIRType type)

I was going to complain about this being called 'n', but since it means different things for different types, it makes sense. No change required.

::: js/src/jit/IonCaches.cpp
@@ +1934,5 @@
> +    uint32_t dummy;
> +    if (!idval.isString() || !idval.toString()->isAtom() || idval.toString()->asAtom().isIndex(&dummy))
> +        return true;
> +
> +    if (!id().constant())

I think there are several places in this patch that attempt to defend against things other than direct getprops flowing into the IC. I *think* this line is one of them? How are id() and idval different, here?

@@ +2011,5 @@
>      }
>  
> +    jsbytecode* pc = cache.idempotent() ? nullptr : cache.pc();
> +
> +    if (!pc || *pc == JSOP_GETPROP || *pc == JSOP_CALLPROP || *pc == JSOP_LENGTH) {

Is this good with the type system? Is the jsbytecode -> JSOp conversion OK as implicit without warning?

Further, I don't see why this is necessary yet. It will certainly be useful later in this stack, but it should be OK without it, as we are only seeing PROP operations at the moment, right?
Attachment #8668297 - Flags: review?(efaustbmo) → review+
Comment on attachment 8668298 [details] [diff] [review]
Part 2 - Fix GetPropertyIC stubs

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

This is quite nice. I'm excited to see this all coming together.

::: js/src/jit/IonCaches.cpp
@@ +1349,5 @@
> +
> +    if (this->id().constant())
> +        return;
> +
> +    TypedOrValueRegister reg = this->id().reg();

can we call this idReg instead? There's a lot flying around in there.

@@ -1934,5 @@
> -    uint32_t dummy;
> -    if (!idval.isString() || !idval.toString()->isAtom() || idval.toString()->asAtom().isIndex(&dummy))
> -        return true;
> -
> -    if (!id().constant())

OK, this makes more sense now in light of the Id checks, but I still think it was superfluous, as we couldn't actually see any non-constant ops yet.
Attachment #8668298 - Flags: review?(efaustbmo) → review+
Comment on attachment 8668299 [details] [diff] [review]
Part 3 - Move arguments element stub

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

LGTM
Attachment #8668299 - Flags: review?(efaustbmo) → review+
Comment on attachment 8668302 [details] [diff] [review]
Part 4 - Move dense element stub

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

Straightforward.
Attachment #8668302 - Flags: review?(efaustbmo) → review+
Comment on attachment 8668303 [details] [diff] [review]
Part 5 - Move dense element hole stub

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

Yep
Attachment #8668303 - Flags: review?(efaustbmo) → review+
Attachment #8668305 - Flags: review?(efaustbmo) → review+
Comment on attachment 8668306 [details] [diff] [review]
Part 7 - Allow disabling GetPropertyIC

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

Sure.
Attachment #8668306 - Flags: review?(efaustbmo) → review+
Comment on attachment 8668307 [details] [diff] [review]
Part 8 - Use MGetPropertyCache for GETELEM

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

PULL THE SWITCH!
Attachment #8668307 - Flags: review?(efaustbmo) → review+
Comment on attachment 8668309 [details] [diff] [review]
Part 9 - rm GetElement IC

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

Bet this one felt good. Good riddance. :)
Attachment #8668309 - Flags: review?(efaustbmo) → review+
(In reply to Eric Faust [:efaust] from comment #18)
> > +    if (!id().constant())
> 
> I think there are several places in this patch that attempt to defend
> against things other than direct getprops flowing into the IC. I *think*
> this line is one of them? How are id() and idval different, here?

id() is the register-or-constant holding the id, and idval is the actual HandleValue. These checks aren't really needed at this point though, as your other comment pointed out.

> Is this good with the type system? Is the jsbytecode -> JSOp conversion OK
> as implicit without warning?

Yup, we do this in several places and it's a bit less verbose than JSOp(*pc) == JSOP_FOO.

> Further, I don't see why this is necessary yet. It will certainly be useful
> later in this stack, but it should be OK without it, as we are only seeing
> PROP operations at the moment, right?

Right, I moved some code out of this patch and could/should have moved this too :)
This seems to have regressed Octane-TypeScript by ~11%. It looks like it's caused by the disable-cache mechanism I ported from GetElementIC to GetPropertyIC, I'll look into this.

Other than that, things are fine I think. Some small wins on the assorted benchmarks like lodash1.

Let's see what Dromaeo does..
Don't disable the cache if we have a constant id (GETPROP) and attached the maximum number of stubs.

Pretty lame but it restores the old behavior in this case and fixes the Octane-TypeScript regression. The right long-term fix is to handle megamorphism better.
Attachment #8672661 - Flags: review?(efaustbmo)
Attached patch One-line fixSplinter Review
I noticed we didn't increment failedUpdates_ anywhere. The patch also simplifies the condition.

Posting this as separate patch because these changes easily regress perf, so I want to land this later.
Attachment #8672665 - Flags: review?(efaustbmo)
Keywords: leave-open
Comment on attachment 8672661 [details] [diff] [review]
Fix TypeScript regression

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

This is a little nose-wrinkly, but it makes sense. File the followup before landing.
Attachment #8672661 - Flags: review?(efaustbmo) → review+
Attachment #8672665 - Flags: review?(efaustbmo) → review+
This was a 17.2% regression on octane-typescript:

http://arewefastyet.com/#machine=28&view=breakdown&suite=octane
Flags: needinfo?(jdemooij)
(In reply to Sean Stangl [:sstangl] from comment #33)
> This was a 17.2% regression on octane-typescript:
> 
> http://arewefastyet.com/#machine=28&view=breakdown&suite=octane

See previous comments, the patch I just pushed should fix this. Fingers crossed...

(In reply to Eric Faust [:efaust] from comment #31)
> This is a little nose-wrinkly, but it makes sense. File the followup before
> landing.

I updated the comment to mention bug 1107515, I really want to fix that bug after the other IC changes...
Blocks: 1214116
Flags: needinfo?(jdemooij)
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/e1d668ff156d
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Depends on: 1264767
No longer depends on: 1264767
You need to log in before you can comment on or make changes to this bug.