RFR (7xS): 8071280: Specialize HeapRegion::oops_on_card_seq_iterate_careful() for use during concurrent refinement and updating the rset

[sangheon]

Hi Thomas,

On 05/05/2017 05:03 AM, Thomas Schatzl wrote:
> Hi Sangheon,
>
>    thanks for your review.
>
> On Tue, 2017-05-02 at 15:07 -0700, sangheon wrote:
>> Hi Thomas,
>>
>> On 04/11/2017 04:30 AM, Thomas Schatzl wrote:
>>> Hi all,
>>>
>>>     I would like you to ask for reviews of a set of related changes
>>> that optimize the way G1 updates/scans/refines cards of the
>>> remembered set.
>>>
>>>
>>> [...]
>>> Here we go:
>>>
>>> 8071280: Specialize HeapRegion::oops_on_card_seq_iterate_careful()
>>> for use during concurrent refinement and updating the rset
>>>
>>> CR: https://bugs.openjdk.java.net/browse/JDK-8071280
>>> Webrev: http://cr.openjdk.java.net/~tschatzl/8071280/webrev
>>>
>>> As the title indicates, this change allows specializations of
>>> HeapRegion::oops_on_card_seq_iterate_careful() according to current
>>> GC
>>> phase (concurrent or during gc) to allow to remove some checks.
>>> This is mostly a preparatory change for the next ones.
>> 8071280, seems good to me.
>>
>> src/share/vm/gc/g1/heapRegion.inline.hpp
>>    149   assert(ClassUnloadingWithConcurrentMark,
>>
>> I have same question as Kim about this assertion.
>> In addition, block_size_during_gc() and block_size() are very similar
>> except block_size() could return earlier.
>> Could you give me some explanation for this?
> As mentioned in Kim's reply: "If ClassUnloadingWithConcurrentMark is
> false, this method will not be called, and this is what the assert
> checks. HeapRegion::is_obj_dead_with_size() then always uses the obj-
>> size() path."
> Block_size_during_gc() is a very aggressive specialization of
> block_size(), so of course they ultimately do the same thing. The
> difference is that during GC a lot of cases that block_size() needs to
> check for always evaluate to a particular value.
>
> The other important optimization that block_size_during_gc() does is
> that we pass the bitmap to look at, as the triple indirection (load
> G1CollectedHeap*, load G1ConcurrentMark*, load G1CMBitMapR0*) to find
> it has shown to be very expensive.
>
> The compiler is unable to see all this.
>
> The reason why the two methods look similar is that
> block_size_during_gc() changes all ifs into asserts checking that it is
> called only in the right circumstances. I.e. that all assumptions that
> we can make during GC are actually true.
Thank you very much for the explanation!

>
> Looking at it now (it has been a long way to get to this version),
> maybe block_size() could just call block_size_during_gc() at the end?
Doesn't hurt your goal with above idea?
Current version seems good to me but if you are considering a change 
adding inline function with common stuffs could be one of idea.

Thanks,
Sangheon


>
> Thanks,
>    Thomas
>