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/15/2017 04:19 AM, Thomas Schatzl wrote:
> Hi Kim, Sangheon,
>
> On Fri, 2017-05-05 at 10:05 -0700, sangheon wrote:
>> Hi Thomas,
>>
>> On 05/05/2017 05:03 AM, Thomas Schatzl wrote:
> [...]
>>>>> 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."
>>>
> [...]
>>> 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.
>    I revisited this again, and factored out the common parts into a new
> method HeapRegion::block_size_using_bitmap().
>
> While doing so I also did reruns of tier1-5 testing with and without
> ClassUnloadingWithConcurrentRemark and found that my suggestion that
> block_size_during_gc() is never called is wrong, so some asserts are
> failing, although the method is safe to call, just unnecessarily slow.
>
> Instead of just removing the asserts, and penalizing the case when
> class unloading is disabled (as likely or unlikely it is), I did some
> tests with protecting the call to block_size_during_gc() with an
> additional ClassUnloadingWithConcurrentMark clause. Perf testing showed
> no difference (with class unloading enabled), so I kept it.
>
> e.g. in HeapRegion::is_obj_dead_with_size():
>
>
>     assert(!is_archive(), "Archive regions should not have references
> into interesting regions.");
>     assert(!is_humongous(), "Humongous objects not handled here");
>     bool result =
>       !obj_allocated_since_prev_marking(obj) &&
>       !bitmap->isMarked((HeapWord*)obj);
> -  if (result) {
> +  if (ClassUnloadingWithConcurrentMark && result) {
>       *size = is_gc_active ? block_size_during_gc((HeapWord*)obj,
> bitmap)
>                            : block_size((HeapWord*)obj);
>     } else {
> +    assert(block_is_obj((HeapWord*)obj), "must be");
>       *size = obj->size();
>     }
>     return result;
>   }
>
> I also fixed some comments.
>
> Webrevs:
> http://cr.openjdk.java.net/~tschatzl/8071280/webrev.1_to_2/ (diff)
> http://cr.openjdk.java.net/~tschatzl/8071280/webrev.2/ (full)
Webrev.2 still looks good to me.

Thanks,
Sangheon


>
> Testing:
> jprt, gc/common tier 1-5 tests with and without
> ClassUnloadingWithConcurrentMark
>
> Thanks,
>    Thomas
>