Bug 157427 – Add JSC options reportBaselineCompileTimes and reportDFGCompileTimes.

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Mark Lam]

... because the baseline JIT needs some debugging love too sometimes.

[Mark Lam]

Created attachment 278265 [details]
proposed patch

[Mark Lam]

Comment on attachment 278265 [details]
proposed patch

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

> Source/JavaScriptCore/runtime/Options.h:403
> +    v(reportDFGCompileTimes, reportCompileTimes, SameOption) \
> +    v(reportJITCompileTimes, reportBaselineCompileTimes, SameOption) \

Any opinion on these aliases?  My reason for adding them is:
1. reportCompileTimes only applies to DFG compile times.  Might as well add a reportDFGCompileTimes to match reportBaselineCompileTimes and reportFTLCompileTimes.
2. We use the term JIT and Baseline interchangeably in our code.  While JIT is shorter, Baseline is more descriptive.  I added reportJITCompileTimes as alias for ease of use.

If anyone disagrees, I can remove these.

[Keith Miller]

Comment on attachment 278265 [details]
proposed patch

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

r=me with comments

>> Source/JavaScriptCore/runtime/Options.h:403
>> +    v(reportJITCompileTimes, reportBaselineCompileTimes, SameOption) \
> 
> Any opinion on these aliases?  My reason for adding them is:
> 1. reportCompileTimes only applies to DFG compile times.  Might as well add a reportDFGCompileTimes to match reportBaselineCompileTimes and reportFTLCompileTimes.
> 2. We use the term JIT and Baseline interchangeably in our code.  While JIT is shorter, Baseline is more descriptive.  I added reportJITCompileTimes as alias for ease of use.
> 
> If anyone disagrees, I can remove these.

It kinda feels weird for reportCompileTimes to only report the DFG compile times rather than all the tiers compile times. How intensely are we following the rule of not removing existing options? I feel like it would not be unreasonable to change it.

[Filip Pizlo]

(In reply to comment #3)
> Comment on attachment 278265 [details]
> proposed patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=278265&action=review
> 
> r=me with comments
> 
> >> Source/JavaScriptCore/runtime/Options.h:403
> >> +    v(reportJITCompileTimes, reportBaselineCompileTimes, SameOption) \
> > 
> > Any opinion on these aliases?  My reason for adding them is:
> > 1. reportCompileTimes only applies to DFG compile times.  Might as well add a reportDFGCompileTimes to match reportBaselineCompileTimes and reportFTLCompileTimes.
> > 2. We use the term JIT and Baseline interchangeably in our code.  While JIT is shorter, Baseline is more descriptive.  I added reportJITCompileTimes as alias for ease of use.
> > 
> > If anyone disagrees, I can remove these.
> 
> It kinda feels weird for reportCompileTimes to only report the DFG compile
> times rather than all the tiers compile times. How intensely are we
> following the rule of not removing existing options? I feel like it would
> not be unreasonable to change it.

Seems reasonable to change it.

reportCompileTimes -> report compile times in all tiers.
reportDFGCompileTimes -> report compile times in DFG+FTL.
reportFTLCompileTimes -> report compile times in DFG.

I think that this is a reasonable change because someone who uses reportCompileTimes will still see DFG compile times.  They will also see baseline compile times, so they will get more than what they asked for, but I don't see how that would break anything.

[Mark Lam]

(In reply to comment #4)
> (In reply to comment #3)
> > Comment on attachment 278265 [details]
> > proposed patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=278265&action=review
> > 
> > r=me with comments
> > 
> > >> Source/JavaScriptCore/runtime/Options.h:403
> > >> +    v(reportJITCompileTimes, reportBaselineCompileTimes, SameOption) \
> > > 
> > > Any opinion on these aliases?  My reason for adding them is:
> > > 1. reportCompileTimes only applies to DFG compile times.  Might as well add a reportDFGCompileTimes to match reportBaselineCompileTimes and reportFTLCompileTimes.
> > > 2. We use the term JIT and Baseline interchangeably in our code.  While JIT is shorter, Baseline is more descriptive.  I added reportJITCompileTimes as alias for ease of use.
> > > 
> > > If anyone disagrees, I can remove these.
> > 
> > It kinda feels weird for reportCompileTimes to only report the DFG compile
> > times rather than all the tiers compile times. How intensely are we
> > following the rule of not removing existing options? I feel like it would
> > not be unreasonable to change it.
> 
> Seems reasonable to change it.
> 
> reportCompileTimes -> report compile times in all tiers.
> reportDFGCompileTimes -> report compile times in DFG+FTL.
> reportFTLCompileTimes -> report compile times in DFG.
> 
> I think that this is a reasonable change because someone who uses
> reportCompileTimes will still see DFG compile times.  They will also see
> baseline compile times, so they will get more than what they asked for, but
> I don't see how that would break anything.

OK, will change it.

As for the reportJITCompileTimes alias for reportBaselineCompileTimes, I decided not to add that.  I plan to add a baselineWhitelist option shortly, and I think it's not worth it to have to keep adding "jit" to "baseline" aliases.  Will stick with just "baseline".

[Mark Lam]

Created attachment 278285 [details]
proposed patch: new and improved.

[Mark Lam]

Thanks for the reviews.  Landed in r200531: <http://trac.webkit.org/r200531>.