[Webkit-unassigned] [Bug 67176] JavaScriptCore does not have tiered compilation

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=67176


Gavin Barraclough <barraclough at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #106276|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #29 from Gavin Barraclough <barraclough at apple.com>  2011-09-05 16:21:54 PST ---
(From update of attachment 106276)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.