RFR [M][7/7]: 8197932: Better split work in rebuild remembered sets phase

[sangheon.kim]

Hi Thomas,

Finally the last piece! :)

On 03/22/2018 03:46 AM, Thomas Schatzl wrote:
> Hi Stefan,
>
>    thanks for your review, and sorry for the late reply...
>
> On Mon, 2018-03-19 at 16:39 +0100, Stefan Johansson wrote:
>> Thanks Thomas,
>>
>> On 2018-03-19 13:03, Thomas Schatzl wrote:
>>> Hi all,
>>>
>>>     Stefan proposed to use some iterator over the work area that
>>> internally handles the boundary at the TAMS wrt to liveness when
>>> scanning.
>>>
>>> This makes the change a bit more compact, performance does not seem
>>> to differ, so why not... :)
>> I really like this approach :) A few additional small things:
>> src/hotspot/share/gc/g1/g1RemSet.cpp
>> 787         if (!is_live(_current)) {
>>
>> I would like to invert the if-statement in the LiveObjIterator
>> constructor and move the call to move_if_below_ntams() into the
>> if-statement updating _current, something like this:
>> if (is_live(_current)) {
>>     // Non-objArrays were scanned by the previous part of that region.
>>     if (_current < mr.start() && !oop(_current)->is_objArray()) {
>>         _current += oop(_current)->size();
>>         // We might have positioned _current on a non-live object.
>> Reposition to the next
>>         // live one if needed.
>>         move_if_below_ntams();
>>       }
>>     }
>> } else {
>>     ...
>> }
>>
>> 926 #ifdef ASSERT
>> 927       // In the final iteration of the loop the region might
>> have been eagerly reclaimed.
>> 928       // Simply filter out those regions. We can not just use
>> region
>> type because there
>> 929       // might have already been new allocations into these
>> regions.
>> 930       HeapWord* const top_at_rebuild_start =
>> _cm->top_at_rebuild_start(region_idx);
>> 931       assert(!hr->is_old() ||
>>
>> What do you think about making this assert into a guarantee or use
>> DEBUG_ONLY() to avoid the #ifdef ASSERT? Also maybe adding something
>> to the comment explaining that it is the check on
>> top_at_rebuild_start that filters out eagerly reclaimed regions.
> Done.
>
>> ---
>> src/hotspot/share/gc/g1/g1_globals.hpp
>> 259   experimental(size_t, G1RebuildRemSetChunkSize, 256 * K,
>>
>> Did you test to see if changing the G1RebuildRemSetChunkSize flag
>> had any impact on performance, otherwise maybe we could make it a
>> constant instead.
> The problem is not so much increasing the value, but decreasing it for
> very slow systems. Decreasing it further adds some overhead.
>
> The other point is that since we track the time a chunk takes, it seems
> strange to not have any knobs to deal with problems in that area.
>
> New webrev:
> http://cr.openjdk.java.net/~tschatzl/8197932/webrev.2_to_3 (diff)
> http://cr.openjdk.java.net/~tschatzl/8197932/webrev.3 (full)
webrev.3 looks good!
Minor nits below.

=============================
src/hotspot/share/gc/g1/g1RemSet.cpp
  805       void move_to_next() {
  810       oop next() const {
(Ignore if you don't agree)
- I like the idea of using iterator here. But for me, current naming 
seems misleading. Current move_to_next() seems next(). And something 
else for current next(). to_oop()?

=============================
src/hotspot/share/gc/g1/g1RemSetTrackingPolicy.cpp
   37   // All non-young and non-closed archive regions need to be 
scanned for references;
   38   // At every gc we gather references to other regions in young, 
and closed archive
   39   // regions by definition do not have references going outside 
the closed archive.
- Add some explanation about r->is_free() case as well?

=============================
src/hotspot/share/gc/g1/g1_globals.hpp
  261           range(4 * K, 32 * 
M)                                              \
- Just question. Why the min is 4K?

Thanks,
Sangheon


>
> In this webrev I also did some somewhat messy looking renaming of nTAMS
> to TAMS (removing the n) and passing the (n)TAMS around in conjunction
> with the bitmap.
> This will make a follow-up change a lot easier to read.
>
> Thanks,
>    Thomas
>