[webkit-reviews] review denied: [Bug 129355] Compilation policy management belongs in operationOptimize(), not the DFG Driver. : [Attachment 225246] the patch.

[bugzilla-daemon at webkit.org]

Filip Pizlo <fpizlo at apple.com> has denied Mark Lam <mark.lam at apple.com>'s
request for review:
Bug 129355: Compilation policy management belongs in operationOptimize(), not
the DFG Driver.
https://bugs.webkit.org/show_bug.cgi?id=129355

Attachment 225246: the patch.
https://bugs.webkit.org/attachment.cgi?id=225246&action=review

------- Additional Comments from Filip Pizlo <fpizlo at apple.com>
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.