RFR(S): 7143490: G1: Remove HeapRegion::_top_at_conc_mark_count

[Tony Printezis]

FWIW: John's analysis is quite spot on. One quick additional point 
though: when I simplified the marking code and adjusted the code that 
handles evacuation failure during initial-mark, I left 
_next_marked_bytes to be 0, even though I calculate the region's marked 
bytes anyway (in order to update _prev_marked_bytes). I instead set 
_top_at_conc_mark_count to bottom() and let the concurrent mark thread 
to do the calculation. See

HeapRegion::note_self_forwarding_removal_start()
HeapRegion::note_self_forwarding_removal_end()
and
RemoveSelfForwardPtrObjClosure::do_object()

(consider the case where during_initial_mark == true)

I just realized that there might be an additional issue with John's 
change. If we have an evac failure during initial-mark, the live bytes 
of the objects that are not evacuated out of a region (but are marked on 
the next bitmap during evac failure handling) will not be correctly 
accounted in _next_marked_bytes. (John, maybe you can  test this with 
your "evac failure a lot" patch - it'd only happen with an evac failure 
during initial mark?) I think the fix is straightforward. Add:

if (during_initial_mark) {
     _next_marked_bytes = marked_bytes;
}

in HeapRegion::note_self_forwarding_removal_end().

Apologies I missed this during my code review.

Tony


On 04/06/2012 05:19 PM, John Cuthbertson wrote:
> Hi Bengt,
>
> Thanks for looking over the changes. Your question is a good one. 
> Hopefully I can answer it to your satisfaction.
>
> The original liveness counting code walked marking bitmap (i.e. the 
> set bits) for the live objects between _top_at_conc_mark_count and 
> NTAMS. At the start of marking, _top_at_conc_mark_count was set to 
> start of the region.  In the event of an evacuation failure while 
> evacuating the objects in a region, R, not all the objects will have 
> been evacuated. So we would tell the counting code to revisit this 
> region bey setting _top_at_conc_mark_count to the region bottom. Any 
> non-evacuated explicitly marked objects would then be walked by the 
> counting code. Any explicitly marked objects that were successfully 
> evacuated would have had their marks propagated so that the new 
> location was marked, and the old marks would have been cleared.
>
> As a result of Tony's marking changes this whole mechanism changed. 
> Now if an evacuation failure occurs during an initial mark pause - any 
> non-evacuated objects in the region are explicitly marked and NTAMS is 
> set to top. Successfully evacuated objects will be copied above NTAMS 
> of the to-space region. When the non-evacuated objects are marked - 
> they will be included in the liveness counting. Objects that are 
> copied above NTAMS are included during the finalization of the 
> liveness counting during cleanup.
>
> The finalization of the liveness counting did include the range 
> [_top_at_conc_mark_count, NTAMS), but since the remark pause set 
> _top_at_conc_mark_count to NTAMS this interval was usually empty. If 
> we did get an evacuation failure during marking (and post remark) then 
> _top_at_conc_mark_count would have been reset. The only issue with 
> this is that we would include the objects in the region that got the 
> evacuation failure twice in the liveness counting data (once as a 
> result of an explicit mark (or successful copy) and the other from the 
> finalization code). This would look like we had more live data than we 
> actually did. When the original liveness counting changes were added 
> we knew that we would lose some accuracy. Removing the field will 
> reduce this inaccuracy.
>
> Does this answer your question?
>
> Thanks,
>
> JohnC
>
> On 04/03/12 06:04, Bengt Rutisson wrote:
>>
>> Hi John,
>>
>> I think your changes look good.
>>
>> Just a question for my understanding. You say "The value of 
>> _top_at_conc_mark_count is now the same as NTAMS". You are probably 
>> correct, but in HeapRegion::note_self_forwarding_removal_start() the 
>> old code was:
>>
>>  if (during_initial_mark) {
>>     ...
>>     _next_top_at_mark_start = top();
>>     set_top_at_conc_mark_count(bottom());
>>     ...
>>
>> Here we set NTAMS and _top_at_conc_mark_count() to different values. 
>> I assume that this did not have any effect, but can you explain why?
>>
>> Thanks,
>> Bengt
>>
>> On 2012-04-03 01:45, John Cuthbertson wrote:
>>> Hi Everyone,
>>>
>>> Here's my contribution to the cleanup week.
>>>
>>> Can I have a couple of volunteers review the changes for this CR? 
>>> The webrev can be found at: 
>>> http://cr.openjdk.java.net/~johnc/7143490/webrev.0/
>>>
>>> Summary:
>>> As a result of Tony's recent marking changes (associated with the 
>>> marking of survivors) the field HeapRegion::_top_at_conc_mark_count 
>>> is no longer required. The value of _top_at_conc_mark_count is now 
>>> the same as NTAMS. Additionally I simplified the verification code 
>>> that sets the bits in the live card bitmap and refactored the 
>>> closures that finalize and verify the liveness counting data to 
>>> avoid code duplication.
>>>
>>> Testing: I inserted a 500ms sleep in the marking cycle between the 
>>> remark and cleanup pauses to ensure that at least one evacuation 
>>> pause was seen during this phase, and ran the GC test suite with 
>>> several marking thresholds (2, 5, and 10%) and verification enabled.
>>>
>>> Thanks,
>>>
>>> JohnC
>>
>