1320670 - Use CacheIR GetPropIRGenerator for Baseline's GetElement IC

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect, P1)

Description

[Jan de Mooij [:jandem]]

Baseline has a GetElement IC that duplicates a lot of the GetProperty code, is really complicated (because it was difficult to handle both PropertyName/Symbol keys with the old Baseline IC architecture), and it doesn't handle some interesting cases that we support for GETPROP.

With some small changes to GetPropIRGenerator, we can reuse it for the GetElement IC. This is similar to what bug 1209118 did for the Ion ICs, but the payoff here is even bigger:

 13 files changed, 320 insertions(+), 1449 deletions(-)

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Part 1 - Pass jsid instead of PropertyName*

This prepares for the next patch.

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Part 2 - Use GetPropIRGenerator for GETELEM

This changes GetPropIRGenerator to emit id guards (similar to Ion's GetPropertyIC::emitIdGuard), uses GetPropIRGenerator for GETELEM, and removes the now-redundant GETELEM stubs.

Comment 3

[Jan de Mooij [:jandem]]

Comment on attachment 8814893 [details] [diff] [review]
Part 2 - Use GetPropIRGenerator for GETELEM

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

::: js/src/jit/VMFunctions.cpp
@@ +1370,5 @@
> +    if (!ValueToId<CanGC>(cx, idVal, &id))
> +        return false;
> +
> +    RootedValue receiver(cx, ObjectValue(*proxy));
> +    return Proxy::get(cx, proxy, receiver, id, vp);    

Removed the trailing whitespace locally.

Comment 4

[Hannes Verschore [:h4writer]]

Comment on attachment 8814890 [details] [diff] [review]
Part 1 - Pass jsid instead of PropertyName*

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

Lgtm

::: js/src/jit/CacheIR.cpp
@@ +51,5 @@
>      AutoAssertNoPendingException aanpe(cx_);
>  
>      ValOperandId valId(writer.setInputOperandId(0));
>  
> +    RootedId id(cx_, INTERNED_STRING_TO_JSID(cx_, idVal_.toString())); //XXX

Not sure what this does? I know it is removed. But why not NameToString?

@@ +71,5 @@
> +        if (tryAttachModuleNamespace(obj, objId, id))
> +            return true;
> +        if (tryAttachWindowProxy(obj, objId, id))
> +            return true;
> +        if (tryAttachProxy(obj, objId, id))

If I understand correctly this "id" is actually not needed. It should be retrievable since we have the idVal_. Having the 'id' as argument is just an optimization to make sure those tryXXX functions don't need to "peel" the id from the idVal_ in every function.

::: js/src/jit/SharedIC.cpp
@@ +2285,2 @@
>                         JSObject** lastProto, size_t* protoChainDepthOut)
>  {

Nice that we don't have to do the Id -> Name -> Id dance here anymore

@@ +2404,5 @@
>      }
>  
>      if (!attached && !JitOptions.disableCacheIR) {
> +        RootedValue idVal(cx, StringValue(name));
> +        GetPropIRGenerator gen(cx, pc, engine, &isTemporarilyUnoptimizable, val, idVal, res);

Where is this idVal created? I don't see it in the current code?

Comment 5

[Hannes Verschore [:h4writer]]

Comment on attachment 8814893 [details] [diff] [review]
Part 2 - Use GetPropIRGenerator for GETELEM

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

Removal of code \0/. Looks very nice!

We don't look at the Baseline GetElem caches to decide optimization? Because I don't see any changes in BaselineInspector? Interesting.

::: js/src/jit/BaselineCacheIR.cpp
@@ +1130,5 @@
> +
> +    // We have a non-atomized string with the same length. Call a helper
> +    // function to do the comparison.
> +    LiveRegisterSet volatileRegs(RegisterSet::Volatile());
> +    masm.PushRegsInMask(volatileRegs);

Should we eventually have OOL code for this? IIUC This shouldn't happen too often. In most cases we shouldn't need to call the helper function. And it would improve code locality?

@@ +1364,5 @@
> +
> +bool
> +BaselineCacheIRCompiler::emitCallProxyGetByValueResult()
> +{
> +    AutoStubFrame stubFrame(*this);

:D

@@ +1680,5 @@
> +    if (numInputs >= 1) {
> +        allocator.initInputLocation(0, R0);
> +        if (numInputs >= 2)
> +            allocator.initInputLocation(1, R1);
> +    }

Yay. Two inputs :D

::: js/src/jit/CacheIR.cpp
@@ +410,5 @@
> +        writer.callProxyGetResult(objId, id);
> +    } else {
> +        // We could call emitIdGuard here and then emit CallProxyGetResult, but
> +        // it's nicer to attach a stub that can handle any Value so we don't
> +        // attach a new stub for every id.

Maybe rephrase to:

// We could call emitIdGuard here and then emit CallProxyGetResult, but
// for GetElem we prefer to attach a stub that can handle any Value so we don't
// attach a new stub for every id.

Definitely a not scope of this patch, but follow-up if deemed interesting.
But would it be possible to have tryAttachGenericProxyPerId and tryAttachGenericProxy.
In tryAttachGenericProxyPerId we would use emitIdGuard and CallProxyGetResult. in tryAttachGenericProxy we would use allProxyGetByValueResult. And we would try tryAttachGenericProxyPerId for a max of "x ids" and when we 
encounter to many ids we remove the "GenericProxyPerId" stubs and use the tryAttachGenericProxy?
Not sure if we hit this enough to warrent such a specialization though?

@@ +800,5 @@
>  }
> +
> +void
> +GetPropIRGenerator::emitIdGuard(jsid id)
> +{

Should this be called: maybeEmitIdGuard?

Comment 6

[Jan de Mooij [:jandem]]

(In reply to Hannes Verschore [:h4writer] from comment #5)
> Should we eventually have OOL code for this? IIUC This shouldn't happen too
> often. In most cases we shouldn't need to call the helper function. And it
> would improve code locality?

I don't know if it would be a measurable win, for Ion it will matter more probably, but an OOL path could be slower if we have to call the VM function a lot, it depends on the input values.

> Definitely a not scope of this patch, but follow-up if deemed interesting.
> But would it be possible to have tryAttachGenericProxyPerId and
> tryAttachGenericProxy.
> In tryAttachGenericProxyPerId we would use emitIdGuard and
> CallProxyGetResult. in tryAttachGenericProxy we would use
> allProxyGetByValueResult. And we would try tryAttachGenericProxyPerId for a
> max of "x ids" and when we 
> encounter to many ids we remove the "GenericProxyPerId" stubs and use the
> tryAttachGenericProxy?
> Not sure if we hit this enough to warrent such a specialization though?

Interesting idea, but I agree it's not common enough atm to optimize for. Also with multiple stubs + potential id guard failures in some of them, it might be faster to just call the generic VM function and do the ValueToId call. ValueToId is pretty fast if the input is an atom/symbol.

> Maybe rephrase to:

Done.

> Should this be called: maybeEmitIdGuard?

Good idea, done.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b75807ac2e0
part 1 - Use jsid instead of PropertyName in GetPropIRGenerator. r=h4writer
https://hg.mozilla.org/integration/mozilla-inbound/rev/17f5bc695acc
part 2 - Use GetPropIRGenerator for GETELEM stubs in Baseline. r=h4writer

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6b75807ac2e0
https://hg.mozilla.org/mozilla-central/rev/17f5bc695acc

Comment 9

[Hannes Verschore [:h4writer]]

Nice work (I think the regressions are just noise):

== Change summary for alert #4462 (as of Dezember 06 2016 01:52 UTC) ==

Regressions:

  3%  ss 1.0.1 unpack-code linux64 opt shell      35.87 -> 36.94
  2%  ss 1.0.1 format-xparb linux64 opt shell     15.94 -> 16.33

Improvements:

 10%  misc 0.8 typedobj-splay-typedobj linux32 opt shell       564.93 -> 508.39
  5%  kraken 1.1 crypto-sha256-iterative linux64 opt shell     47.38 -> 45.13
  5%  misc 0.8 map-iterator linux32 opt shell                  61.05 -> 58.3
  3%  kraken 1.1 crypto-pbkdf2 linux64 opt shell               114.63 -> 110.95
  2%  kraken 1.1 crypto-sha256-iterative linux32 opt shell     54.3 -> 53.07
  2%  misc 0.8 basic-array-forof linux64 opt shell             25.6 -> 25.02
  2%  misc 0.8 react-shell linux64 opt shell                   129.3 -> 126.48
  2%  ss 1.0.1 format-tofte linux64 opt shell                  13.75 -> 13.48

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=4462