RFR: 8133051: Concurrent refinement threads may be activated and deactivated at random

Thu Apr 14 15:21:54 UTC 2016

Hi Kim,

On 2016-04-14 04:54, Kim Barrett wrote:
>> On Apr 13, 2016, at 7:46 AM, Thomas Schatzl <thomas.schatzl at oracle.com> wrote:
>
> Thomas, thanks for looking at this.
>
>> - (pre-existing) in the options that were changed, maybe spell out "rem
>> set" and "rset" for better understanding.
>
> I *think* what you are referring to here is the update_rs_xxx
> parameters?  That abbreviation seems to be fairly common usage in the
> code base, occurring about 30 times, while update_rset occurs fewer
> than 10.  In fact, the phase time tag is "G1GCPhaseTimes::UpdateRS".
> I agree something other than "update_rs" might be clearer, but would
> rather see any such change being done as a separate nomenclature
> cleanup.
>
>> - this is a change of existing behavior: the values for the global
>> flags used are never written back, not even after initialization. It
>> would be nice to do so. That is kind of useful without using adaptive
>> concurrent refinement. The logs won't give you that information in this
>> case.
>
> I don't think writing back the values into the global options is
> useful, and in fact think it would be (and was, for me) confusing,
> since the "real" values are always in the ConcurrentG1Refine object's
> data members.  The usage model here has always been that the global
> options were input parameters used to determine the initial values
> that would actually be used.  Not modifying the global options makes
> that more apparent (at least to me).
>
>> Anyway, a log message printing the initial values would be really nice
>> regardless of whether adaptiveness is turned on.
>
> I think the global option values can be obtained as part of
> -XX:+PrintFlagsFinal.  I don't think it's worth duplicating that
> information.  The initial zone values that are calculated from the
> (defaulted) CLA's *are* logged.
>
> However, the computed minimum yellow zone size was not being logged
> but should be.  Fixing that ended up making the two uses of
> log_refinement_zones different, so that helper has been dropped.
>
>> - is there a reason why this change needs to particularly put the
>> Thresholds struct into a namespace? Please remove the namespace usage
>> declaration.
>
> I think we should be using anonymous namespaces to limit the scope of
> file-local utility functions.  But I don't think this review is the
> place for that discussion, so I'm changing Thresholds to be a typedef
> for Pair<size_t, size_t>.
>
>> - the log message prints the thresholds, not the zones (ranges). This
>> might lead to misunderstandings.
>>
>> The same with variable and parameter names. (this is mostly pre
>> -existing)
>
> I think the nomenclature in the log messages matches the nomenclature
> and semantics for the corresponding command line options.  The term
> "threshold" as used here refers to (de)activation buffer count values
> for the threads.
>
> I agree the chosen terminology is somewhat confusing (especially that
> green and yellow refer to ranges below the value, while red refers to
> the range above the value), but I don't want to mess with that as part
> of this change.  Especially since future improvements to the control
> of the threads might render some of this obsolete.
>
>> - the defines should not use local variables with leading underscore.
>> While there are not particular rules for it, variables with leading
>> underscore are reserved for members.
>> It initially got me confused with these defines being used in the
>> context of some object, and referencing that object's members.
>
> I was trying to make them stand out and be obviously local to these
> macros, but I conceed the point about underscores and member names.
> The ugly prefixes based on the macro name are sufficient anyway.
> Leading underscores removed.
>
>> Please also consider #undef'ing these local defines.
>
> Why?  They are intended to be used anywhere in the file.  Really, they
> ought to be file-scoped functions, but I was recently reminded that
> such assert functions interract poorly with debugging Windows failures.
>
>> - in concurrentG1Refine.cpp:154, the assignment of "size = green * 2"
>> seems quite random, just as the previous factor of 3.
>> Wouldn't it be better to just use min_size, or do nothing here because
>> the following value clamping will fix it up anyway?
>
> The old computation was (3 * green), the new is (2 * green) + green.
> That is, I'm retaining the old computation (when the CLA is
> defaulted), but doing it in a different way.  And then applying the
> new range clamping.  Twice green can certainly be larger than
> min_size.  [Note that I'm not really defending some of these
> calculations; I expect some of them to change substantially as part of
> addressing JDK-8137022, and for now am trying to minimize those sorts
> of changes.]
>
>> - the "update_rs_processed_buffers" parameter in various methods should
>> imho be a size_t, not a double. There does not seem to be a reason to
>> use a double. (pre-existing)
>
> I was annoyed / confused by the use of double here, but had assumed it
> was to match the value from the caller.  Now that I've actually gone
> and looked, you are absolutely right, this should be size_t all the
> way through, to match the type actually provided by the phase_time.
> Thanks for noticing that.
>
>> - I kind of dislike even requiring an unused parameter for a method in
>> static methods like in calc_new_yellow/red_zone, but I kind of see the
>> point for consistency's sake. Let's see what others think.
>
> Those unused parameters are to some extent a failure on my part to pay
> attention to the TODO notes (see below).  Since these are completely
> internal and not exposed API functions, future changes to their
> parameters are easy, so I'm removing the unused parameters, to be
> reinstated later if/when they are actually needed.
>
>> In any case, I would prefer if the TODO's were removed. Most likely
>> they are related to improvements you have in your queue anyway,
>> otherwise I would prefer to store TODO's in the bug tracker.
>
> The TODO's were intended to serve two purposes.  They remind reviewers
> that the existing code is something of a placeholder, merely
> attempting to retain something like the pre-existing calculations.
> They also reminded me to not mess with these calculations as part of
> this change set, even though I think some of them are rather strange,
> because doing so will make this change set bigger (possibly much
> bigger) and make the review process harder.  I'll remove them now.
>
>> - for consistency: all methods but calc_new_*_zone() parameters that
>> contain the thresholds have the _zone suffix, but not those:
>
> I *think* I was consistent in dropping the _zone suffix when in
> functions whose name makes it clear they are about zones, such as
> calc_{new,init}_*_zone.  Counter-examples?
>
>> - concurrentG1Refine.cpp:174ff: indentation of the parameter list does
>> not conform to any other in the same file.
>
> I suspect that was because the line width was getting excessive during
> development of these changes.  It looks like some later changes have
> made that not such a problem, so back to the more typical style.
>
>> - the * 2 in the calc_new_yellow_zone() seems to be the same factor as
>> in calc_init_yellow_zone(). Unless this is changed soon, I would prefer
>> if it were factored out somehow.
>
> I intend to completely redo calc_new_yellow_zone in the near future,
> so I'd rather not spend time on something like that.
>
>> - concurrentRefine.cpp:360+362: instead of the C-style cast, maybe a
>> static_cast should be used here too.
>
> I intend to completely redo the chunk of code related to updating the
> dcqs in the near future, so tried to avoid making unnecessary changes,
> such as touching line 362.
>
>> - in addition to the actual new thresholds, it might be interesting to
>> get information about the values passed to G1ConcurrentRefine::adjust()
>> in a trace message.
>
> Agreed.  Added.
>
>> Looks good overall though.
>
> In addition to changes to address your comments, I'm also addressing
> Jon's log augmenting suggestion, and fixing these:
>
> ------------------------------------------------------------------------------
> src/share/vm/gc/g1/concurrentG1Refine.cpp
>   324   return yellow + (yellow - green);
>
> Missing MIN2 with max_red_zone.
>
> ------------------------------------------------------------------------------
> src/share/vm/gc/g1/concurrentG1Refine.cpp
>   300     if (green > 0) {
>   301       green = MIN2(static_cast<size_t>(green * dec_k), green - 1);
>   302     }
>
> The MIN2 is unnecessary here; this can be just
>
>    green = static_cast<size_t>(green * dec_k);
>
> (which matches the pre-change code).
>
> ------------------------------------------------------------------------------
>
> New webrevs:
> full: http://cr.openjdk.java.net/~kbarrett/8133051/webrev.01/
> incr: http://cr.openjdk.java.net/~kbarrett/8133051/webrev.01.inc/

in concurrentG1RefineThread.hpp
   81   size_t activation_threshold() const { return _threshold; }

We usually name accessors to match the member name, but I realize that 
"threshold()" here is not informative enough.
How about renaming _threshold => _activation_threshold ?

in concurrentG1RefineThread.cpp

  70 void ConcurrentG1RefineThread::update_thresholds

has somewhat unusual formatting, the common style (which we agreed upon 
at some point IIRC) is either

void A::foo(int bar, int baz) {
}

or

void A::foo(int bar,
             int baz) {
}

I'm not sure that I agree that it's necessary to have the CTRL_TAGS and 
LOG_ZONES #defines in order to ensure that all three logging call sites 
use the same set of logging tags.

The rest of the change looks good and it's a lot more understandable 
what the code is trying to achieve.

/Mikael

>
> Local testing looks good.
> Currently running a Nightly G1 batch.
>