Bug 129355 – Compilation policy management belongs in operationOptimize(), not the DFG Driver.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Filip Pizlo]

If you want to poll for an opportunity to compile code, then you should do that outside of the DFG::compile() driver.  Such polling belongs in operationOptimize().  We already do such polling for shouldAlwaysBeInlined and for the execution counter itself.

[Radar WebKit Bug Importer]

<rdar://problem/16169484>

[Mark Lam]

Moving the previous title here for reference:
"CompilationInvalidated is reserved for signaling concurrent JIT race conditions and should never be used to mean "I didn't feel like compiling right now so ask me again later”.

By compilation policy, I mean the rules for determining whether to compile, when to compile, when to attempt compilation again, etc.  The few of these policy decisions being made in the DFG driver are now moved to operationOptimize() where we keep the rest of the policy logic.  Decisions that are based on the capabilities supported by the DFG are moved to DFG capabiliityLevel().

Patch coming.

[Mark Lam]

Created attachment 225246 [details]
the patch.

[Filip Pizlo]

Comment on attachment 225246 [details]
the patch.

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

> Source/JavaScriptCore/dfg/DFGCapabilities.cpp:232
> +
> +    if (!MacroAssembler::supportsFloatingPoint())
> +        return CannotCompile;
> +
> +    if (!Options::bytecodeRangeToDFGCompile().isInRange(codeBlock->instructionCount()))
> +        return CannotCompile;
> +

I would put these checks into the mightCompile... and mightInline... methods.  There are a handful of places that will call the mightInline() methods instead of calling capabilityLevel().  You could give them all a common helper function like "isSupported(CodeBlock*)" that will do checks like supportsFloatingPoint and the bytecodeRange.

> Source/JavaScriptCore/jit/JITOperations.cpp:1053
> +    if (!Options::useDFGJIT()) {
> +        codeBlock->dontOptimizeAnytimeSoon();
> +        return encodeResult(0, 0);
> +    }
> +
> +    if (vm.enabledProfiler()) {
> +        codeBlock->optimizeAfterWarmUp();
> +        return encodeResult(0, 0);
> +    }
> +
> +    Debugger* debugger = codeBlock->globalObject()->debugger();
> +    if (debugger && (debugger->isStepping() || codeBlock->baselineAlternative()->hasDebuggerRequests())) {
> +        codeBlock->optimizeAfterWarmUp();
> +        return encodeResult(0, 0);
> +    }
> +

This should all be placed below the call to checkIfOptimizationThresholdReached().  That call must be the first thing you do on an ExecutionCounter slow path.

Also, the !useDFGJIT() part should be in DFGCapabilities.

[Mark Lam]

Created attachment 225270 [details]
patch 2: applied Filip’s feedback.

[Filip Pizlo]

Comment on attachment 225270 [details]
patch 2: applied Filip’s feedback.

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

Almost there.

> Source/JavaScriptCore/dfg/DFGCapabilities.cpp:45
> +    if (!Options::useDFGJIT()
> +        || !MacroAssembler::supportsFloatingPoint()
> +        || !Options::bytecodeRangeToDFGCompile().isInRange(codeBlock->instructionCount()))
> +        return false;
> +    return true;

Why not just turn this into a single return statement?

> Source/JavaScriptCore/jit/JITOperations.cpp:1065
> +        codeBlock->optimizeAfterWarmUp();

You could call updateAllPredictions() here.

> Source/JavaScriptCore/jit/JITOperations.cpp:1071
> +        codeBlock->optimizeAfterWarmUp();

And here.

You could even have a helper that calls both optimizeAfterWarmUp and updateAllPredictions.

[Mark Lam]

Created attachment 225273 [details]
patch 3: applied more feedback.

[Mark Lam]

Thanks.  Landed in r164734: <http://trac.webkit.org/r164734>.