803710 - IonMonkey: Warn and prevent Ion code re-entry if a code bails frequently out.

Status: NEW

(Core :: JavaScript Engine, defect)

Description

[Nicolas B. Pierron [:nbp]]

+++ This bug was initially created as a clone of Bug #799818 +++

Before bug 799818, PdfJS was bailing out frequently at 3 locations, solving these bailouts improve a bit the efficiency of PdfJS by falling back on a slower path once such bailing possibility is detected.  Bailing a few times when they are many re-entry is fine, the problem is having multiple times a bailout path which does not cause an invalidation or which does not forbid a re-entry.

Adding such path would be helpful to fallback on JM-compiled script when IonMonkey is failing. As we do not expect IonMonkey to be failing, we should at least produce a warning to at least be aware of such cases and to track them.

Comment 1

[David Anderson [:dvander] - inactive, e-mail if emergency]

See also, bug 784134

Comment 2

[Nicolas B. Pierron [:nbp]]

The goal of this bug is to warn and prevent re-entry of badly optimized cases.  A bailout happens when a guard fails or when a constraint causes an invalidation.  In addition to prevent any bad effects, we would also like to get a warning such as anybody seeing the warning can report it back on Bugzilla.

The principle would be to add a counter on the JSScript to count the number of bailout which occurred in this script.  The counter will be incremented outer-most frame (the most inline one) which is resumed in the interpreter (probably on ThunkToInterpreter in ion/Bailout.cpp).  When this counter reach 100 (for example), we should emit a warning with JS_ReportError and forbid further compilation of the script.  We might also increment the same counter by 5 (to prevent 100 recompilations) when the script is recompiled.

This counter must be reset to 0 when a GC happens.  If we don't do so, we would prevent IonMonkey from running after a while, because some compilation outputs are garbage collected. Even if this will not report every issue that we currently have, there is hope that somebody will trigger the warning with a minimal case.

Once this patch is done. Run benchmarks and a patched firefox to highlight existing issues.  If there is a non empty list of warning, report & file them on Bugzilla as blocker of this Bug. Once all dependent bugs are fixed (if they were any), the patch for this bug should be able to land.


To test if the warning appear correctly, you can locally revert patches of Bug 799818, and check PdfJS.js octane benchmark spew (IONFLAGS=bailout) to make sure we are printing the warning message at the right time.

Comment 3

[Nicolas B. Pierron [:nbp]]

Sorry if people were interested by this Bug, but I have to take it before implementing Bug 812980 as this bug is now on the critical path for PdfJS octane benchmark performance.

Comment 4

[Nicolas B. Pierron [:nbp]]

Attachment: Add a cap for frequent bailout & recompilations.

This patch add a cap for recompilation and for bailouts.  It also add a bit of instrumentation to report as much information as possible without being disruptive.  This information and the shell option, to make them crash, are made  to take advantage of reducer on big benchmarks having such issues.

Recompilations cap:
- if the function is recompile at most 20 times in a different way.
- if the function is recompiled at least 4 times in the same way.

Bailout cap:
- No more than one bailout per a 100 µs (below is considered as frequent).  Only triggered checked after 20 bailouts, and reset after 40.  A more accurate measurement would imply measuring the time inside each function, which sounds like too much to handle.

Caps are made as a per-runtime and are focussed on one at a time, considering that if there are multiple one, we might fix one day … or not.  Having them as a per-runtime saves space and avoid having them all over the place, on the other hand this means that such issues should not be ignored because a website can have 2 interleaving issues causing a performance issue because none of the cap will trigger to forbid Ion entry.

Comment 5

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 690155 [details] [diff] [review]
Add a cap for frequent bailout & recompilations.

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

Recompilation, deoptimization caps are a good idea. Having messages or optional traps on hitting these caps is good too.

However the mechanism in this patch is much too complicated. It should just be a straightforward counter on the JSScript (if we're worried about bloat, we could nest IonScript inside another Ion struct, like JM does). I feel like detecting identical recompilations is an academic use case. With the appropriate caps, we will not have pathological performance faults, which is the real problem.

In cases where we're doing actual performance work, and have profiled a function to be hot path, there's a question of whether it's running in the right JIT. Usually this is pretty easy to determine, and this patch doesn't add any new information to help that. In terms of detecting repeated deopts/recompiles, the existing spew does that already, and a simple cap would make it even clearer.

Comment 6

[Nicolas B. Pierron [:nbp]]

(In reply to David Anderson [:dvander] from comment #5)
> In cases where we're doing actual performance work, and have profiled a
> function to be hot path, there's a question of whether it's running in the
> right JIT. Usually this is pretty easy to determine, and this patch doesn't
> add any new information to help that. In terms of detecting repeated
> deopts/recompiles, the existing spew does that already, and a simple cap
> would make it even clearer.

And this is not the goal.  The goal of this bug is only to prevent running in Ion after we have detected failures.

Comment 7

[Nicolas B. Pierron [:nbp]]

Unassigned my-self: I am not actively working on it right now.