Ion currently just gets info about monomorphic caches from JM, but should also get this state from Baseline. This optimization seems to be worth 100-150 points on trunk, but adding this currently slows down Baseline due to bug 843483 and bug 844253.

Same basic idea as what you posted, but bulked up a bit: 1. Adds a BaselineInspector class similar to TypeInferenceOracle. Instances of this class are responsible for answering questions about a particular Script by querying it's baseline script's ICs (and other info, if any). 2. GetProp and SetProp ICs now use their 'extra_' field to store a bit indicating if they've seen an unoptimizable access. Shape snooping treats the presence of that bit as an indication that mono shape caching is not a good idea for that operation. 3. IonBuilder is modified to contain a pointer to a BaselineInspector.

Comment on attachment 718265 [details] [diff] [review] Alternate patch Jan, pinging you on this as well as I figured you would have thoughts about the design of the Ion => Baseline information querying.

Comment on attachment 718265 [details] [diff] [review] Alternate patch Review of attachment 718265 [details] [diff] [review]: ----------------------------------------------------------------- The other stuff is minor, but it looks like you're only calling maybeMonomorphicShapeForPropertyOp in the GETPROP case. Similar code should be used near where IonBuilder::jsop_setprop calls mjit::GetPICSingleShape. ::: js/src/ion/BaselineBailouts.cpp @@ +1076,5 @@ > mjit::PurgeCaches(outerScript->ion->getScript(i)); > > + // No need to purge baseline ICs. Baseline will do one of two things: add a new > + // optimized stub (preventing monomorphic IC caching), or set a flag indicating that > + // an unoptimizable access was made, also preventing mono IC caching. I think the PurgeCaches call above is pretty pointless actually, after invalidating in Ion we will end up back in JM and just refill the mjit cache. So long as the new shape which caused the failure is seen by baseline and its cache updated, things should work fine. ::: js/src/ion/BaselineIC.cpp @@ +3942,4 @@ > } > > + if (!attached) > + stub->noteUnoptimizableAccess(); The restructuring to avoid a premature 'return true' in this function seems cumbersome and doesn't actually affect semantics. If the function returns immediately after attaching a new stub, then if control gets to the end of the function there was no stub attached and the unoptimizable access can be noted. @@ +4201,5 @@ > if (!TryAttachSetPropStub(cx, script, stub, obj, id, rhs, &attached)) > return false; > + > + if (!attached) > + stub->noteUnoptimizableAccess(); This looks fine, though if you change DoGetPropFallback then this should also change to mirror its structure. ::: js/src/ion/BaselineInspector.cpp @@ +17,5 @@ > + if (!hasBaselineScript()) > + return NULL; > + > + JS_ASSERT(isValidPC(pc)); > + const ICEntry &entry = icEntryFromPC(pc); Separate issue, but since Ion can now be expected to query icEntryFromPC O(n) times on a given script, icEntryFromPC should be updated to do a binary search rather than a linear one. Easier to just fix eagerly rather than wait for a testcase where the quadratic cost is noticeable. ::: js/src/ion/BaselineInspector.h @@ +38,5 @@ > + return script->baseline; > + } > + > + private: > + bool isValidPC(jsbytecode *pc) { This function should be #ifdef DEBUG, or you could inline it at its two call sites as JS_ASSERT(unsigned(pc - script->code) < script->length) ::: js/src/ion/IonBuilder.cpp @@ +6742,5 @@ > + > + if (!objShape) { > + objShape = inspector->maybeMonomorphicShapeForPropertyOp(pc); > + if (objShape) > + spew("KVKV got monomorphic shape for PC on script!"); rm spew() call

Comment on attachment 718265 [details] [diff] [review] Alternate patch Review of attachment 718265 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, nice to have the query-baseline logic in one place. Agreed about the O(n^2) issue though, would be good to fix that.

(In reply to Brian Hackett (:bhackett) from comment #3) > Separate issue, but since Ion can now be expected to query icEntryFromPC > O(n) times on a given script, icEntryFromPC should be updated to do a binary > search rather than a linear one. Easier to just fix eagerly rather than > wait for a testcase where the quadratic cost is noticeable. I'm going to leave this for a follow-up patch. We can make this access effectively constant time for most use cases. Most IC accesses by Ion will be sequential in nature. A lookup on PC |a| will most likely be followed by a lookup for PC |a + Delta| for some small delta. We can make the general lookup on BaselineScript be a binary search, and also add a scanning search method which takes a prior (pc, index) pair as input, and scans forward from there. BaselineInspector can keep a 1-entry cache of the (pc, index) pair for a previous lookup, and if it notices that the next lookup is for a closeby PC, invoke the scanning method instead of the binary search method to do the lookup.

Initial push: https://hg.mozilla.org/projects/ionmonkey/rev/5aa6636cf79a Secondary push with BaselineInspector source files which were not included in the first patch: https://hg.mozilla.org/projects/ionmonkey/rev/b1e216ed414c Waiting until TBPL green to close.

Looks good.

This bug broke BC on AWFY, didn't it?

I've just fixed AWFY to reautoconf if the build fails.

There seem to be some jit-test hangs with --ion-eager now on 32-bit. The tests hang because we bailout at a LOOPENTRY op, enter Ion and immediately bailout again without doing any work. This patch adds some code to resume at the next op in this case, it's the same workaround as in ion::ThunkToInterpreter.

Comment on attachment 718905 [details] [diff] [review] Followup fix Review of attachment 718905 [details] [diff] [review]: ----------------------------------------------------------------- Nice catch.

Pushed follow-up: https://hg.mozilla.org/projects/ionmonkey/rev/a261716e1b62