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

[sangheon.kim]

Hi Thomas,

On 03/28/2018 04:23 AM, Thomas Schatzl wrote:
> Hi,
>
> On Tue, 2018-03-27 at 10:50 -0700, sangheon.kim wrote:
>> Hi Thomas,
>>
>> Finally the last piece! :)
>>
>    yes :)
>
> Unfortunately there are already a few more changes out for review,
> although much smaller in scope and complexity.
:)

>
> [...]
>>> 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()?
> I would like to keep it as is until we in the team figured out how to
> do iterators in hotspot, and then change as needed.
>
> Apart from the naming this style is 1:1 compatible to STL iterators
> (i.e. move_to_next() -> "++" operator, next() -> * operator) (which
> although I expect us to use them in the future, I have no particular
> preference for any style), so I chose that.
Okay!

>
>> =============================
>> 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?
> I will fix this before pushing.
OK

>
>> =============================
>> src/hotspot/share/gc/g1/g1_globals.hpp
>>    261           range(4 * K, 32 *
>> M)                                              \
>> - Just question. Why the min is 4K?
> 4k is like 1024 references max. This seemed to be a small enough
> increment to not cause too much safepointing delay for the smallest
> machine we are going to run with G1.
Thanks for the answer, agree.

>
> I am going to push this change as is - your review did not seem to pose
> any significant issue. Otherwise there is enough opportunity to fix
> this immediately.
Thank you for the nice patches!!

Sangheon


>
> Thanks,
>    Thomas