JIT debugging features that selectively disable the JITs for code blocks need to stay out of the way of the critical path of JIT management

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Filip Pizlo]

Long ago, we used to selectively disable compilation of CodeBlocks for debugging purposes by adding hacks to DFGDriver.cpp.  This was all well and good.  It used the existing CompilationFailed mode of the DFG driver to signal failure of CodeBlocks that we didn't want to compile.  That's great because CompilationFailed is a well-supported return value on the critical path, usually used for when we run out of JIT memory.

Later, this was moved into DFGCapabilities. This was basically incorrect. It introduced a bug where disabling compiling of a CodeBlock meant that we stopped inlining it as well.  So if you had a compiler bug that arose if foo was inlined into bar, and you bisected down to bar, then foo would no longer get inlined and you wouldn't see the bug.  That's busted.

So then we changed the code in DFGCapabilities to mark bar as CanCompile and foo as CanInline. Now, foo wouldn't get compiled alone but it would get inlined.

But then we removed CanCompile because that capability mode only existed for the purpose of our old varargs hacks.  After that removal, "CanInline" became CannotCompile.  This means that if you bisect down on bar in the "foo inlined into bar" case, you'll crash in the DFG because the baseline JIT wouldn't have known to insert profiling on foo.

We could fix this by bringing back CanInline.

But this is all a pile of nonsense.  The debug support to selectively disable compilation of some CodeBlocks shouldn't cross-cut our entire engine and should most certainly never involve adding new capability modes.  This support is a hack at best and is for use by JSC hackers only.  It should be as unintrusive as possible.

So, as in the ancient times, the only proper place to put this hack is in DFGDriver.cpp, and return CompilationFailed.  This is correct not just because it takes capability modes out of the picture (and obviates the need to introduce new ones), but also because it means that disabling compilation doesn't change the profiling mode of other CodeBlocks in the Baseline JIT.  Capability mode influences profiling mode which in turn influences code generation in the Baseline JIT, sometimes in very significant ways - like, we sometimes do additional double-to-int conversions in Baseline if we know that we might tier-up into the DFG, since this buys us more precise profiling.

[Filip Pizlo]

Created attachment 247794 [details]
the patch

[Mark Lam]

Comment on attachment 247794 [details]
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=247794&action=review

r=me

> Source/JavaScriptCore/ChangeLog:12
> +        at timelike infinity. That makes these hacks much more likely to continue working as we make

"timelike"?

[Mark Lam]

Comment on attachment 247794 [details]
the patch

What happened to MacroAssembler::supportsFloatingPoint()?  You didn't include that in the new check.

[Benjamin Poulain]

View in context: https://bugs.webkit.org/attachment.cgi?id=247794&action=review

r=me too

> Source/JavaScriptCore/ChangeLog:8
> +        See bug description for complete analysis. This reduces the intrusiveness of debugging hacks

You should copy the description here. We assume that the ChangeLogs will survive bugzilla.

[Mark Lam]

(In reply to comment #3)
> Comment on attachment 247794 [details]
> the patch
> 
> What happened to MacroAssembler::supportsFloatingPoint()?  You didn't
> include that in the new check.

My bad.  This didn't need to be moved.

[Filip Pizlo]

(In reply to comment #2)
> Comment on attachment 247794 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=247794&action=review
> 
> r=me
> 
> > Source/JavaScriptCore/ChangeLog:12
> > +        at timelike infinity. That makes these hacks much more likely to continue working as we make
> 
> "timelike"?

It's a term of art, see for example http://en.wikipedia.org/wiki/Closed_timelike_curve

[Filip Pizlo]

(In reply to comment #4)
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=247794&action=review
> 
> r=me too
> 
> > Source/JavaScriptCore/ChangeLog:8
> > +        See bug description for complete analysis. This reduces the intrusiveness of debugging hacks
> 
> You should copy the description here. We assume that the ChangeLogs will
> survive bugzilla.

OK.

[Filip Pizlo]

Landed in http://trac.webkit.org/changeset/180956