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

[John Rose]

On Jan 16, 2015, at 5:05 AM, Zoltán Majó <zoltan.majo at oracle.com> wrote:
> 
> Here is the new webrev: http://cr.openjdk.java.net/~zmajo/8059606/webrev.02/

Reviewed, with some comments.

Overall, I like the cleanups along the way.

The basic idea of replacing a hard-coded 'mask' with an addressable variable is sound and nicely executed.

I suppose that idea by itself is "S" small, but this really is a "M" or even "L" change, as Vladimir says, especially since the enhanced logic is spread all around many files.

How have you regression tested this?  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.

In the sparc assembly code (more than one file), the live range of Rcounters has increased, since it is used to supply limits as well as to update the counter (which happens early in the code).

To make it easier to maintain the code, I suggest renaming Rcounters to G3_method_counters.

(As you can see, when a register has a logical name but has a complicated live range, we add the hardware name is to the logical name, to make it easier to spot interfering uses, when manually editing code.)

If scale==0.0 is a valid input checked specially in compileBroker, perhaps the effect of a zero should be documented?
Suggest adding to globals.hpp:
   "; values greater than one delay counter overflow; zero forces counter overflow immediately"
   "; can be set as a per-method  option."

Question:  What if both a global scale and a method option for scale are both set?  Is the global one ignored?  Do they multiply?  It's worth specifying it explicitly (and checking that the logic DTRT).

Question:  How are the log values (like Tier0InvokeNotifyFreqLog or the result of get_scaled_freq_log) constrained to fit in a reasonable range?  (I would suppose that range is 0..31 or less.)  Should we have range clipping code in the scaler functions?

It would give notably simpler code, in MethodData::init, to use a branch-free setup of tier_0_invoke_notify_freq_log etc.  Set scale = 1.0 and then update it conditionally.  Special-case scale=1.0 in get_scaled_freq_log to taste.

Same comment about less-branchy code for methodCounters.hpp.  (It's better to have a one-way branch that sets up 'scale' than a two-way branch with duplicate setups of one or more variables.)

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!)

Small style nit:  The noise-word "get_" is discouraged in the style doc:

>  • Getter accessor names are noun phrases, with no "get_" noise word. Boolean getters can also begin with "is_" or "has_".
>   https://wiki.openjdk.java.net/display/HotSpot/StyleGuide

Arguments.cpp follows this rule partially (in older code maybe?).  It would be better to decrease counter-examples to the rule instead of increase them.

Bigger style nit:  Since the functions are not getting a preset value (from the arguments) but rather normalizing a provided argument value, I suggest naming them "scale_compile_threshold" (i.e., a verb phrase instead of a noun phrase).  Again from the style doc:

>  • Other method names are verb phrases, as if commands to the receiver.


Since you are providing overloads of the scaling functions, the header file should either contain inline code for the convenience methods, or else document how the optional argument ('scale') defaults.  I'd prefer inline code, since it is simple.  It's as much text to document with a comment as just to supply the inline overload.

As I said before, nice work!

— John