[9] RFR(S): 8059606: Enable per-method usage of CompileThresholdScaling (per-method compilation thresholds)

[John Rose]

On Jan 19, 2015, at 8:56 AM, Zoltán Majó <zoltan.majo at oracle.com> wrote:
> ...
>> How have you regression tested this?
> 
> I used our performance infrastructure. I collected performance data on 6 different architectures for 41 benchmark programs/suites. The data show that per-method compilation thresholds do not result in a statistically significant change of performance. One benchmark, Footprint3-Client, degrades ~0.5% on the X86 Client VM, but I think that is negligible.
> 
>> Have you verified that the compilation sequence doesn't change for a non-trivial use case?  A slip in the assembly code (for example) might cause a comparison against a garbage mask or limit that could cause compilation decisions to go crazy quietly.  I didn't spot any such bug, but they are hard to spot and sometimes quiet.
> 
> I did extensive manual testing on all architectures targeted by the patch.
> 
> I checked that a target method (a method for which per-method compilation thresholds have been specified) is indeed compiled sooner/later than methods for which global compilation thresholds are in effect. I ran tests for all combinations of +/-TieredCompilation, +/-ProfileInterpreter, and +/-UseOnStackReplacement
> on each architecture that we support. While working on this issue I've discovered and reported two interpreter-related bugs, 8068652 and 8068505.
> 
> I cannot, unfortunately, guarantee that my changes are error-free, but I did my best to catch any possible error that I can think of and that I can check.

No, but you have clearly done the "due diligence".  Thanks for explaining.

The comments in globals.hpp are good, but watch the spacing.  String concatenation will cause some words to run into each other.  This isn't very important, but it might show up with java -XX:+PrintFlagsWithComments (in a debug build).

> 
>> In MethodCounters, I think the conditional scaling of _interpreter_backward_branch_limit is going to confuse someone, at some point.  It should be scaled, unconditionally, like its sibling variables.  (That would remove another somewhat verbose initialization branch!)
> 
> The value of the *global* interpreter backward branch limit (InvocationCounter::InterpreterBackwardBranchLimit) is computed based on the value of CompileThreshold (in InvocationCounter::reinitialize()). The backward branch limit is assigned a different value depending on whether interpreter profiling is enabled or not.
> 
> The logic of the *per-method* interpreter backward branch limit (MethodCounters::_interpreter_backward_branch_limit) is intended to be identical to that of the global branch limit. Therefore, I'm afraid that we have to keep the if-then-else construct initializing _interpreter_backward_branch_limit in methodCounters.hpp. I hope that I understood right what you've previously suggested.

I see; you are duplicating an odd distinction that was already there.  I still think it is a little confusing, but it's not something you can change easily.

> Here is the new webrev: http://cr.openjdk.java.net/~zmajo/8059606/webrev.03/ <http://cr.openjdk.java.net/~zmajo/8059606/webrev.03/>
Reviewed; it's good.

One last nit, and this doesn't need re-review (from me):  Please use the macro right_n_bits instead of a C expression (1 << x)-1, if possible.  There are lots of ways that the hand-written C expression can go wrong, and the macro is safer.  One more quote from the style guide:

> 	• Use functions from globalDefinitions.hpp when performing bitwise operations on integers. Do not code directly as C operators, unless they are extremely simple. (Examples: round_to, is_power_of_2, exact_log2.)


— John

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20150119/fc9b463b/attachment.html>