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

[bugzilla-daemon at webkit.org]

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





--- Comment #30 from Filip Pizlo <fpizlo at apple.com>  2011-09-05 17:23:45 PST ---
> > Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:1006
> > +#if CPU(X86_64)
> 
> This should be using 'scratchRegister', not 'X86Registers::r11'.

I wanted to do that, but that won't work. :-(  scratchRegister is in MacroAssemblerX86_64.h.  I also can't move this branchAdd32 overload to the subclasses (MacroAssemblerX86_64 and MacroAssemblerX86) because if you have a subclass overloading something in a superclass, the compiler gets confused.  Basically if I have this particular overload of branchAdd32 in MacroAssemblerX86_64 then the branchAdd32's in MacroAssemblerX86Common get hidden.

So we have a number of choices for workarounds:

1) Keep it as is, and accept this as local ugliness.

2) Move the scratchRegister declaration to MacroAssemblerX86Common.

3) Move all branchAdd32 overloads to the MacroAssemblerX86_64 and MacroAssemblerX86.

4) Rename this particular overload of branchAdd32 and put it in MacroAssemblerX86_64.

Which do you prefer?  I'm starting to like (4) best.

> 
> > 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.

+1

> 
> > Source/JavaScriptCore/dfg/DFGPropagation.cpp:35
> > +class Propagator {
> 
> Sorry to be a pain, but this .cpp & .h should really probably be DFGPropagator.cpp/.h.

+1

> 
> > 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.

What about just renaming DynamicPrediction to StrongPrediction and StaticPrediction to WeakPrediction?  

> 
> > 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.

There isn't a lot of code that relies on this enum.  So do we really need to future-proof this?

Right now we do indeed have a tie between mechanism and policy, because we're using the BaselineJIT as our bottom tier, and the DFGJIT for the top tier.  So to me it makes sense to have one enum for both policy and mechanism, does it not?

> 
> > 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.

+1

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