Closed Bug 767419 Opened 12 years ago Closed 12 years ago

IonMonkey: Support idempotent GetProperty ICs

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: jandem, Assigned: jandem)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

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).
Attached patch Patch (obsolete) — Splinter Review
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.
Attachment #637410 - Flags: review?(dvander)
Attached patch PatchSplinter Review
Ensure the proto chain has no non-native objects or objects with resolve hooks.
Attachment #637410 - Attachment is obsolete: true
Attachment #637410 - Flags: review?(dvander)
Attachment #637451 - Flags: review?(dvander)
Attachment #637451 - Flags: review?(bhackett1024)
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.
Attachment #637451 - Flags: review?(bhackett1024) → review+
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.
Attachment #637451 - Flags: review?(dvander) → review+
(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.
https://hg.mozilla.org/projects/ionmonkey/rev/fdc72b8c249c
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.