[webkit-reviews] review denied: [Bug 67176] JavaScriptCore does not have tiered compilation : [Attachment 106276] the patch - fix GTK

[bugzilla-daemon at webkit.org]

Gavin Barraclough <barraclough at apple.com> has denied Filip Pizlo
<fpizlo at apple.com>'s request for review:
Bug 67176: JavaScriptCore does not have tiered compilation
https://bugs.webkit.org/show_bug.cgi?id=67176

Attachment 106276: the patch - fix GTK
https://bugs.webkit.org/attachment.cgi?id=106276&action=review

------- Additional Comments from Gavin Barraclough <barraclough at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=106276&action=review


r- for a few comments to look at, basically all looks good though.

> Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:1006
> +#if CPU(X86_64)

This should be using 'scratchRegister', not 'X86Registers::r11'.

> Source/JavaScriptCore/bytecode/CodeBlock.h:227
> +	   PassOwnPtr<CodeBlock> releaseAlternative() { return
m_alternative.release(); }

I think the code could probably be refactored quite easily to remove
releaseAlternative(), and I think it would be easier to follow if we did so.

The code currently sets the m_fooBarCodeBlock variables on Executables quite
aggressively, and then has to reset this value if compilation fails.  Now this
involves switching CodeBlocks & switching back - I think it would be
conceptually cleaner to just store the new code block in a local RefPtr, and
assign across (releasing from the local) to the member only if compilation
succeeds.

> Source/JavaScriptCore/dfg/DFGPropagation.cpp:35
> +class Propagator {

Sorry to be a pain, but this .cpp & .h should really probably be
DFGPropagator.cpp/.h.

> Source/JavaScriptCore/dfg/DFGPropagation.cpp:138
> +	       setPrediction(makePrediction(PredictInt32, DynamicPrediction));

If I understand this correctly, the value 'DynamicPrediction' seems to be being
used here to cover two different types of static predictions.
In the case of bitops & valuetoint32, we can statically infer with absolute
certainty that the result is an int32.
In the case of ArithMod & UInt32ToNumber I guess you are assuming the results
are highly likely to be int32, and as such want to give a string prediction
rather than a weak one, despite this being static & not dynamic?

If so, I'd suggest adding a couple of new enum values, say
StrongStaticPrediction & StaticInference.  You could make their values equal to
DynamicPrediction such that they will currently be handled the same for now,
but give us the option to change how these predictions are merged with each
other in the future.

> Source/JavaScriptCore/jit/JITCode.h:52
> +	   

It might be more helpful to have a separate type indicating the compilation
kind.  Using the JITType here artificially makes a tie between a policy (tiered
compilation), and a set of particular mechanisms (BaselineJIT, DFGJIT).  Using
the same enum to select may make it more difficult to reconfigure this in the
future.  E.g. should we want to use the non-spec path of the DFG-JIT with
optimization hooks as our low tier JIT & a speculating compile from the DFG-JIT
exploiting value profiling.

In such a case, I guess we can just replace DFGJIT with two new enum entries, &
deal with working out in all cases whether DFGJIT checks were checking for top
tier or the new compiler.  But we may regret not having an explicit type for
the tiers.

> Source/JavaScriptCore/wtf/Platform.h:978
> +#endif

Slightly verbose set of config, & requires two switches setting - could just
make this:

#if !defined(ENABLE_VALUE_PROFILER)
#define ENABLE_VALUE_PROFILER ENABLE_TIERED_COMPILATION
#endif
#if !defined(ENABLE_DYNAMIC_OPTIMIZATION)
#define ENABLE_DYNAMIC_OPTIMIZATION ENABLE_TIERED_COMPILATION
#endif

So by default both follow the ENABLE_TIERED_COMPILATION setting.