RFR: 8030646: Track collection set membership in one place
erik.helin at oracle.com
Fri Mar 6 16:15:18 UTC 2015
On 2015-01-27, Thomas Schatzl wrote:
> thanks for taking this over.
Thanks for reviewing and sorry for the (very) late reply. I've uploaded
new patches at:
Please see comments inline.
> On Mon, 2015-01-26 at 12:02 +0100, Erik Helin wrote:
> > Hi all,
> > this (rather) small patch removes the field
> > HeapRegion::_in_collection_set and instead only uses the
> > G1BiasedMappedArray G1CollectedHeap::_in_cset_fast_test to track
> > collection set membership. Given that _in_cset_fast_test already track
> > collection set membership, the _in_collection_set field in HeapRegion is
> > redundant, it is only messy to keep track of this information in two
> > places.
> g1CollectorPolicy.cpp:1754: superfluous whitespace after "assert(".
> heapRegion.cpp: in HeapRegion::hr_clear(), is there a reason to remove
> the assert that checks whether the region is in the collection set or
> not? I think it could be rewritten the new method easily. (I.e. using
> the in_collection_set() method in HeapRegion)
No, you are right, we can keep the assert, I've added it back.
> I think this change could remove the "_fast_test" suffix of the various
> methods because now there is only this location where collection set
> inclusion is tracked. It's just confusing to keep this imo.
> (and possibly extend "cset" to "collection_set" - this has iirc been
> done because of the lengthy "fast_set" suffix).
> It would be possible to separate that renaming step if you want, but I
> somehow tend to that that should be part of this change.
I removed the fast_test sufffix from the
G1CollectedHeap::register_*_with_in_cset_fast_test methods, but kept
cset (collection_set becomes too long IMO). I did not remove the suffix
from the method G1CollectedHeap::clear_cset_fast_test, because the name
clear_cset is too generic.
I could also remove the method G1CollectedHeap::in_cset_fast_test, that
method was apparently unused.
More information about the hotspot-gc-dev