review request (M) - 8030646: track cset membership in one place

[Bengt Rutisson]

Hi John,

On 2014-01-15 20:48, John Coomes wrote:
> Bengt Rutisson (bengt.rutisson at oracle.com) wrote:
>> Hi John,
>>
>> 8030647-cset-rename-fast-test
>> 8030648-cset-move-methods
>> 8030649-cset-oop-in-cset
>> 8030650-cset-rm-length
>>
>> These all look good. No comments.
>>
>> 8030651-cset-unify-obj-in-cs
>>
>> This is a nice cleanup. Thanks for fixing this!
> Hi Bengt,
>
> Thanks for looking at it.  I'm just getting back to this after the
> holidays and some sick time.
>
>> I like the division between in_cset() and oop_in_cset(). Have you
>> checked if there are more occurrences of in_cset() that could be
>> replaced by oop_in_cset()? It looks to me like these in_cset() could be
>> changed to oop_in_cset() in these two methods:
>>
>> G1CollectedHeap::handle_evacuation_failure_par()
>> G1ParScanThreadState::verify_ref()
> The one in verify_ref(oop* ref) is already oop_in_cset(), unless I'm
> misunderstanding you.  But I missed the two in
> handle_evacuation_failure_par(); I'll change them, e.g.:
>
> 	-    assert(old == forward_ptr || !in_cset(forward_ptr),
> 	+    assert(old == forward_ptr || !oop_in_cset(forward_ptr),

Looks good. I am not sure what I was thinking about in 
G1ParScanThreadState::verify_ref(). Now that I look at it I think you 
are right. Sorry for the confusion.

>
>> And I *think* G1STWIsAliveClosure::do_object_b() could also use
>> oop_in_cset() too.
> That one is harder to be certain about.  I'll try some tests with it,
> but may want to defer.

OK. Thanks for trying it.

Just to be sure I'm reviewing the right thing. Are you planning a second 
webrev to address Thomas' comments?

Thanks,
Bengt

>
> -John
>
>> On 2013-12-18 07:00, John Coomes wrote:
>>> Please review a set of changes to eliminate redundant cset membership
>>> tracking in g1:
>>>
>>> http://cr.openjdk.java.net/~jcoomes/8030646-cset-unify-all.html
>>>
>>> -John