RFR (M): 8151126: Clean up duplicate code for clearing the mark bitmaps

Jon Masamitsu jon.masamitsu at oracle.com
Tue Mar 15 18:15:20 UTC 2016



On 3/11/2016 5:19 AM, Thomas Schatzl wrote:
> Hi Jon,
>
> On Thu, 2016-03-10 at 10:42 -0800, Jon Masamitsu wrote:
>> http://cr.openjdk.java.net/~tschatzl/8151126/webrev/src/share/vm/gc/g
>> 1/g1ConcurrentMark.cpp.frames.html
>>
>> 728 void G1ConcurrentMark::clear_prev_bitmap(WorkGang* workers) {
>> 729 assert(SafepointSynchronize::is_at_safepoint(), "Should only
>> clear
>> the entire prev bitmap at a safepoint.");
>> 730 clear_bitmap((G1CMBitMap*)_prevMarkBitMap, workers, false);
>>    731 }
>>
>> Would it be appropriate to move the assertion at 729 to
>>
>> 698 void G1ConcurrentMark::clear_bitmap(G1CMBitMap* bitmap, WorkGang*
>> workers, bool may_yield) { assert(may_yield
>> ||SafepointSynchronize::is_at_safepoint(), "Should only clear the
>> entire
>> bitmap at a safepoint."
>>
>>    699 G1ClearBitMapTask task(bitmap, this, workers->active_workers(),
>> may_yield);
>> 700 workers->run_task(&task);   701 guarantee(!may_yield ||
>> task.is_complete(), "Must have completed
>> iteration when not yielding.");
>>
>>    702 }
>>
>> Seems like you would get more coverage when needed for the assertion.
>    I think these are separate concerns: that clear_prev_bitmap is not
> called outside a safepoint, and that clear_bitmap() in general is not
> called without yielding outside of a safepoint. (Not sure if that mean
> with "additional coverage").

Ok.  The change is fine then.

Jon

>
> New webrevs:
> http://cr.openjdk.java.net/~tschatzl/8151614/webrev.0_to_1/ (diff)
> http://cr.openjdk.java.net/~tschatzl/8151614/webrev.1/ (full)
>
> Thanks,
>    Thomas
>



More information about the hotspot-gc-dev mailing list