The default bug view has changed. See this FAQ.

Use CacheIR GetPropIRGenerator for Baseline's GetElement IC

RESOLVED FIXED in Firefox 53

Status

()

Core
JavaScript Engine: JIT
P1
normal
RESOLVED FIXED
4 months ago
4 months ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

(Blocks: 1 bug)

unspecified
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

4 months ago
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(-)
(Assignee)

Comment 1

4 months ago
Created attachment 8814890 [details] [diff] [review]
Part 1 - Pass jsid instead of PropertyName*

This prepares for the next patch.
Attachment #8814890 - Flags: review?(hv1989)
(Assignee)

Comment 2

4 months ago
Created attachment 8814893 [details] [diff] [review]
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.
Attachment #8814893 - Flags: review?(hv1989)
(Assignee)

Updated

4 months ago
Depends on: 1320145
(Assignee)

Comment 3

4 months ago
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 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?
Attachment #8814890 - Flags: review?(hv1989) → review+
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?
Attachment #8814893 - Flags: review?(hv1989) → review+
(Assignee)

Comment 6

4 months ago
(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

4 months ago
Pushed by jandemooij@gmail.com:
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

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6b75807ac2e0
https://hg.mozilla.org/mozilla-central/rev/17f5bc695acc
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
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
You need to log in before you can comment on or make changes to this bug.