RFR (M): 8129558: Coalesce dead objects during removal of self-forwarded pointers
Tom Benson
tom.benson at oracle.com
Thu Jun 25 15:51:35 UTC 2015
Hi Thomas,
A couple of minor comments and questions:
It looks like "_last_forwared_object_end" is misspelled throughout.
The RemoveSelfForwardPtrObjClosure constructor was changed to set _g1 to
G1CollectedHeap::heap() and _cm to _g1->concurrent_mark() rather being
given them by RemoveSelfForwardPtrHRClosure. This seems a little odd,
since RemoveSelfForwardPtrHRClosure is given the values by
G1ParRemoveSelfForwardPtrsTask::work . Of course they'll be the same,
but perhaps they should either be consistently inherited or not. Am I
missing something there?
I think this comment is stale, possibly having been cut/pasted from the
old version of do_object. Now, the code is processing a range which may
have contained multiple objects:
69 // The object has been either evacuated or is dead. Fill it with a
70 // dummy object.
In doHeapRegion, I'm a little surprised that both the old and new
versions of the code do the _g1h->check_bitmaps() call before doing the
updates, rather than afterward. Should it be afterward, or should
there be another verification?
Tom
On 6/25/2015 11:30 AM, Thomas Schatzl wrote:
> Hi Mikael,
>
> thanks for the review...
>
> On Thu, 2015-06-25 at 15:20 +0200, Mikael Gerdin wrote:
>> Hi Thomas,
>>
>> On 2015-06-25 10:07, Thomas Schatzl wrote:
>>> Hi all,
>>>
>>> can I have reviews for the first change in a set of changes that speed
>>> up evacuation failure?
>>> [...]
>>> CR:
>>> https://bugs.openjdk.java.net/browse/JDK-8129558
>>>
>>> Webrev:
>>> http://cr.openjdk.java.net/~tschatzl/8129558/webrev
>> Can you make zap_dead_objects a member function of
>> RemoveSelfForwardPtrObjClosure,
>> then it will already have access to _g1 and _cm
>> and then have remove_self_forward_ptr_by_...
>> call a function on rspc to clear the last part of the region?
>> That way the zapping functionality is nicely encapsulated and
>> last_object_end doesn't need to be exposed.
>>
>> Otherwise I think the change looks good and I think it's a nice step
>> towards cleaning up this code.
> New webrevs at
> http://cr.openjdk.java.net/~tschatzl/8129558/webrev.1 (complete)
> http://cr.openjdk.java.net/~tschatzl/8129558/webrev.0_to_1 (diff)
>
> Fixed according to your suggestions I hope - it still needs an extra
> method (zap_remainer()) to hide the last_object_end.
>
> Thanks,
> Thomas
>
>
More information about the hotspot-gc-dev
mailing list