RFR (M): 8153503: Move remset scan iteration claim to remset local data structure

[Mikael Gerdin]

Hi Thomas,

On 2016-04-14 11:56, Thomas Schatzl wrote:
> Hi,
>
> On Wed, 2016-04-13 at 16:33 -0400, Derek White wrote:
>> On 4/13/16 2:06 PM, Thomas Schatzl wrote:
>>> Hi,
>>>
>>> On Wed, 2016-04-13 at 13:27 -0400, Derek White wrote:
>>>> Hi Thomas,
>>>>
>>>> Small comment at the end...
>>> [...]
>>>
> [...]
>>> One can of course separate the tasks (update rs, scan rs and scan
>>> code
>>> roots), but I do not really want to address this here. There is the
>>> separate issue JDK-8153505 already.
>>
>> Oh, I wasn't thinking of anything that grand, just a wrapper (another
>> iterator) around HeapRegionRemSetIterator::has_next() that only
>> returns
>> claimed card_indexes (eg. factoring out all of the filtering). But
>> it's
>> not worth it since this is only done in one place anyway.
>>
>>>> One suggestion to existing code:
>>>>
>>>> g1RemSet.cpp, Line 235 (etc):
>>>>
>>>> Rename "jump_to_card" => "claimed_cards". The "jump" term pre
>>>> -dates
>>>> the
>>>> current parallelization scheme and now isn't very meaningful.
>>> I think nothing changed conceptually in this part. I will see if I
>>> can
>>> find a better name for that variable though. :)
>>
>> Sorry, I was referring to rev 1261 back in 2010 when they switched to
>> a
>> block-based parallelization :-) The previous code was full of "jumps"
>> and "skips", which is why we're still talking about "jump_to_card". A
>> name that mentions "claims" and/or "blocks" would be more useful.
>>> There are much better ways to handle work distribution for scan rs,
>>> but
>>> in this CR (and probably for JDK 9 FC) I would prefer if we did not
>>> change how claiming work works.
>> Agreed.
>>
>> Final review comment:
>>
>> --- heapRegionRemSet.hpp
>>    - Copyright.
>>
>> Ship it!
>
>    new webrevs at
> http://cr.openjdk.java.net/~tschatzl/8153503/webrev.0_to_1/ (diff)
> http://cr.openjdk.java.net/~tschatzl/8153503/webrev.1/ (full)

This looks fine to me.

The indentation is a bit fishy in g1RemSet.cpp
  272   if (_scan_state->set_iter_complete(region_idx)) {
  273      // Scan the strong code root list attached to the current region
  274      scan_strong_code_roots(r);

I don't need to see a new webrev for that, though :)

/Mikael

>
> Thanks,
>    Thomas
>