RFR (L): 8060025: Object copy time regressions after JDK-8031323 and JDK-8057536

[Thomas Schatzl]

Hi Kim,

On Wed, 2014-12-17 at 15:19 -0500, Kim Barrett wrote:
> On Dec 17, 2014, at 2:19 PM, Thomas Schatzl <thomas.schatzl at oracle.com> wrote:
> > 
> > Otherwise I would like to separate this naming discussion from this CR, as I would prefer to have this change
> > in asap. A few other changes depend on it.
> 
> I’m OK with that. Except for the naming question, these changes look good to me.
> 

Great.

> >> Regarding is_not_in_cset, what exactly does that state mean?  Specifically, does it mean the region
> >> is presently unused / inactive?  Or could it be used for something other than young/old/humongous?
> >> If the former, then perhaps rename that state (and the predicate) accordingly, e.g. Inactive or something
> >> like that.
> > 
> > The names of the predicates directly derive from the state names.
> > 
> > A region that has NotInCSet state is not inactive in any way, it's just a region that we do not consider
> > for potential collection/reclamation.
> 
> > 
> > The difference between humongous (candidate) regions (state Humongous) and Young/Old region is only that
> > the former is only checked for references, but not evacuated in any way.
> > 
> > Young/Old regions in the collection set are evacuation sources.
> > 
> > One option would be to define a collection set that includes both humongous and young/old regions in the
> > collection set and the young/old regions into some sort of "evacuation set" (in quotes because I do not like
> > the name; typically I would assume that collection set == evacuation set).
> 
> How about NotInCSet => OutsideCSet?  OutOfCSet?  That still conflicts with “collection set == evacuation set” for humongous though.  But see above about deferred naming discussion, and below about alternative names for the predicate at issue.
> 
> > Then we could have the following predicates:
> > 
> > static bool is_in_cset()  const { return _value != NotInCSet; }
> > static bool is_not_in_cset() const { return !is_in_cset(); } // I guess that this is not true is your point
> > of contention
> 
> Correct.
> 
> > static bool is_in_evac_set() const { return _value > NotInCSet; }
> > 
> > instead of
> > 
> > bool is_not_in_cset() const          { return _value == NotInCSet; }
> > bool is_in_cset_or_humongous() const { return _value != NotInCSet; }
> > bool is_in_cset() const              { return _value > NotInCSet; }
> > 
> > Alternatively maybe just renaming
> > 
> >  is_not_in_cset
> > 
> > above to
> > 
> >  is_not_in_cset_or_humongous()
> > 
> > may be as good or better, but is a bit on the long side. It would be the
> > negation of is_in_cset_or_humongous() below then (and could be expressed
> > as such).
> > 
> > Or we could just remove the is_not_in_cset() predicate and use !
> > is_in_cset_or_humongous() everywhere it's needed.
> 
> Any of these seem OK to me.  The last seems like the simplest.

I did that in the last changeset (below).

> 
> >> Unfortunately, I think the in_cset_state_t typedef needs to be public.  Otherwise,
> >> how can a client declare the type of a variable that will hold the result of the
> >> value() function.
> > 
> > It does compile though. I will fix it.
> 
> Does that mean there aren’t any callers of value()?  Or does it mean they are making assumptions
> about the type it returns, possibly with implicit conversions involved?
> 

It seems to have been coerced. I fixed that as well, adding a printf
format define for that type.

Webrev:
http://cr.openjdk.java.net/~tschatzl/8060025/webrev.3/
Diff webrev (unfortunately includes the most recent changes too)
http://cr.openjdk.java.net/~tschatzl/8060025/webrev.2_to_3/

I am going to do another cursory perf run with that, not expecting any
change though from older numbers. If it is fine, and you are okay with
that change, I will push webrev.3 tomorrow.

Thanks,
  Thomas