RFR: 8030646: Track collection set membership in one place

[Kim Barrett]

On Feb 2, 2015, at 5:51 AM, Thomas Schatzl <thomas.schatzl at oracle.com> wrote:
> 
> Hi Kim,
> 
>> Coming in late:
>> 
>> In the comments for JDK-8027959
>> https://bugs.openjdk.java.net/browse/JDK-8027959
>> Early reclamation of large objects in G1
>> 
>> Thomas said:
>> 
>> ------------------------------------------------------------------------------
>>> In reply to the previous comment:
>>> 
>>> Putting the humongous objects (even temporarily) into the collection
>>> set gives issues with remembered set updates not occurring any more.
>>> 
>>> Current prototype changes in_cset_fast_test to include humongous
>>> objects (
>> 
>> There are no issues with remembered set updates when temporarily
>> putting the humongous object into the _in_cset_fast_test collection
>> set test because during rset update we use the _in_collection_set
>> member of the heapregion, not the in_cset_fast_test.
> 
> This mentions an older version of the change, that has not even been out
> for review.
> As the date of the original comment suggests, we had a prototype running
> for months before the review.
> 
> If you look in the update rs closure
> (G1UpdateRSOrPushRefOopClosure::do_oop_work()), it uses
> "to->in_collection_set()" in the condition to either remember the
> reference for a possible evacuation failure, or adding the reference to
> the correct remembered set.
> *If* we really temporarily added humongous into the cset, this would be
> a problem.
> 
> However the current implementation is correct, because the humongous
> objects are never even added temporarily to the collection set as stated
> in that comment. They have their own value (-1) in the
> in_cset_fast_table.
> 
> As long as the new code uses only G1CollectedHeap::is_in_cset() instead
> of G1Collectedheap::is_in_cset_or_humongous(), it is fine.
> 
> Sorry for the misleading comment, I will add another one about the
> current implementation. It was more a very late answer about the to the
> previous one (that predates the recent one by four months, i.e.
> differnet implementation).

Thanks for the explanation, Thomas.  That cleared things up for me
quite well.