Last Comment Bug 767419 - IonMonkey: Support idempotent GetProperty ICs
: IonMonkey: Support idempotent GetProperty ICs
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal with 1 vote (vote)
: ---
Assigned To: Jan de Mooij [:jandem]
:
:
Mentors:
Depends on: 780936
Blocks: IonSpeed 768739
  Show dependency treegraph
 
Reported: 2012-06-22 09:15 PDT by Jan de Mooij [:jandem]
Modified: 2012-08-07 11:58 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (25.08 KB, patch)
2012-06-28 01:48 PDT, Jan de Mooij [:jandem]
no flags Details | Diff | Splinter Review
Patch (26.70 KB, patch)
2012-06-28 03:50 PDT, Jan de Mooij [:jandem]
dvander: review+
bhackett1024: review+
Details | Diff | Splinter Review

Description Jan de Mooij [:jandem] 2012-06-22 09:15:29 PDT
v8-richards and v8-deltablue have some GETPROP's where the property does not have a definite/fixed slot, but we do know it's not a getter. In these cases we should be able to mark MGetPropertyCache as idempotent so that we can still use LICM and GVN.

I prototyped this today and this seemed fairly straight-forward (assuming I'm not missing something).
Comment 1 Jan de Mooij [:jandem] 2012-06-28 01:48:12 PDT
Created attachment 637410 [details] [diff] [review]
Patch

Uses the JM IC to mark ops as not-idempotent. I tried the opposite (marking as idempotent) but this gave better results and was more robust. IonMonkey only tries to use idempotent caches if JM is enabled (or, for testing/fuzzing, with --ion-eager).

If a property does not exist or we see a non-native object, the cache invalidates the script and we will redo the op in the interpreter. A flag on the script is set to avoid doing this repeatedly.

This patch is a 10% deltablue win, maybe more once we have polymorphic inlining. On the whole V8 benchmark, most caches are marked as idempotent, but interestingly we never trigger invalidation from an idempotent cache.
Comment 2 Jan de Mooij [:jandem] 2012-06-28 03:50:14 PDT
Created attachment 637451 [details] [diff] [review]
Patch

Ensure the proto chain has no non-native objects or objects with resolve hooks.
Comment 3 Brian Hackett (:bhackett) 2012-06-28 07:39:27 PDT
Comment on attachment 637451 [details] [diff] [review]
Patch

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

::: js/src/ion/IonCaches.cpp
@@ +295,5 @@
> +        if (!obj->isNative())
> +            return false;
> +
> +        if (obj->getClass()->resolve != JS_ResolveStub)
> +            return false;

You also might want to guard on lookup hooks not being present on the class' ObjectOps.

@@ +317,5 @@
> +    if (!obj->isNative())
> +        return true;
> +
> +    // If the cache is idempotent, watch out for resolve hooks or non-native
> +    // objects on the proto chain.

A bigger comment here would be good, explaining why we need these checks before calling obj->lookupProperty.
Comment 4 David Anderson [:dvander] 2012-06-28 16:52:39 PDT
Comment on attachment 637451 [details] [diff] [review]
Patch

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

Nice work. Thinking about deltablue, there is a pattern like:

for (var i = 0; i < ...; i++) {
    var c = ...;
    c.execute();
}

Where we inline through every |c.execute| (with djvj's patch), but with this patch we still don't eliminate every MGetNameCache, which means a load on every loop edge. Crankshaft however, fuses the getprop+call and determines that each shape check is enough to inline the callee, rather than guarding on the value coming out of the property. Realizing this, that seems like a separate bug now.

When I write a microbenchmark, we do in fact eliminate dead MGetNameCaches now. I assume that's safe since we'll invalidate if a getter is added?

::: js/src/ion/Ion.cpp
@@ +1329,5 @@
> +{
> +    JS_ASSERT(script->hasIonScript());
> +
> +    Vector<types::RecompileInfo> scripts(cx);
> +    if (!scripts.append(types::RecompileInfo(script)))

nit: Vector<types::RecompileInfo, 1> and then you don't have to make this function boolean.

::: js/src/ion/IonCaches.cpp
@@ +360,5 @@
>      AutoDetectInvalidation adi(cx, vp, topScript);
>  
> +    // If the cache is idempotent, we will redo the op in the interpreter.
> +    if (cache.idempotent())
> +        adi.disable();

Maybe ADI should just take in a cache? This pattern looks like it could be dangerous.

::: js/src/ion/IonCaches.h
@@ +261,5 @@
>          return *(IonCacheName *)this;
>      }
>  
>      void setScriptedLocation(JSScript *script, jsbytecode *pc) {
> +        JS_ASSERT(!idempotent_);

Why is this assert needed? (Just noticing, for profiling, it's nice to always have the script+pc in ICs).

::: js/src/jit-test/tests/ion/getprop-idempotent-cache.js
@@ +12,5 @@
> +}
> +
> +f(new O(true));
> +
> +// "o.x" is now missing so the idempotent cache should invalidate f.

FWIW, we probably want to cache the non-existence of a property someday. Installing a getter might be a better test

::: js/src/jsscript.h
@@ +541,5 @@
>      bool            failedBoundsCheck:1; /* script has had hoisted bounds checks fail */
>  #endif
> +#ifdef JS_ION
> +    bool            invalidatedIdempotentCache:1; /* idempotent cache has triggered invalidation */
> +#endif

It's okay to remove the JS_ION here.
Comment 5 Jan de Mooij [:jandem] 2012-07-04 07:05:43 PDT
(In reply to David Anderson [:dvander] from comment #4)
> 
> When I write a microbenchmark, we do in fact eliminate dead MGetNameCaches
> now. I assume that's safe since we'll invalidate if a getter is added?

Hm good catch, the IC itself guards against non-native objects on the prototype and resolve/lookup hooks etc, so it seems safest to not eliminate it, I will add a setGuard().

> 
> nit: Vector<types::RecompileInfo, 1> and then you don't have to make this
> function boolean.

I tried this but I had to change Vector<types::RecompileInfo> to Vector<types::RecompileInfo, 1> everywhere else..

> 
> Maybe ADI should just take in a cache? This pattern looks like it could be
> dangerous.

ADI is also used in other VM functions, and I don't want ADI and Ioncache to depend on each other. I don't think it's too dangerous though, if you forget to disable() it it will crash/assert soon (adding it fixed many jit-test failures).

> >      void setScriptedLocation(JSScript *script, jsbytecode *pc) {
> > +        JS_ASSERT(!idempotent_);
> 
> Why is this assert needed? (Just noticing, for profiling, it's nice to
> always have the script+pc in ICs).

The main problem is that, due to GVN, a single idempotent cache can have multiple script/pc's associated with them. Requiring the script/pc to be NULL avoids bugs where we update something based on only one script/pc, I will add a comment.
Comment 6 Jan de Mooij [:jandem] 2012-07-06 06:06:53 PDT
https://hg.mozilla.org/projects/ionmonkey/rev/fdc72b8c249c

Note You need to log in before you can comment on or make changes to this bug.