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

[Derek White]

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...
> [...]
>
>>>> I'll look at this more on Monday.
>>>>     - Derek
>> Sorry for dripping this out. I spent a lot of time trying to
>> understand what G1ScanRSClosure::doHeapRegion() was really doing -
>> what it was iterating over, how it was filtering out the claimed
>> cards, etc.  If that iteration were done anywhere else I'd suggest
>> separating it out into a "claimed" iterator class (but it's not done
>> anywhere else).
> Not sure what you mean, but probably something like SubTasksDone or so.
>
> The problem is that in this case, you want other threads to help each
> other when one region takes too long. So really every thread wants to
> iterate over all heap regions.
>
> Further, there is this matter that parts of the work should only ever
> done by a single thread if possible (adding the region to the dirty
> cards regions) or must be (code root scan).
>
> 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!

  - Derek