RFR: JDK-8209118: Abstract SATBMarkQueueSet's ThreadLocalData access

[Roman Kennke]

Hi Kim,

Thanks for reviewing!

>> Currently, SATBMarkQueueSet directly accesses G1SATBMarkQueue for
>> getting to thread-local SATBMarkQueue. This should be abstract in order
>> to enable SATBMarkQueue stuff to be used by other GCs.
>>
>> I propose to achieve that by adding a (pure) virtual method:
>> satb_queue_for_thread(JavaThread* const t)
>>
>> to SATBMarkQueueSet, which would have to be implemented by GC specific
>> subclasses. One side effect is that handle_zero_index_for_thread() would
>> have to move to subclass too (which is fine for Shenandoah, we're not
>> using it at all. Maybe this would require some consolidation in the
>> future for G1 on the platforms that use it..?)
>>
>> Bug:
>> https://bugs.openjdk.java.net/browse/JDK-8209118
>> Webrev:
>> http://cr.openjdk.java.net/~rkennke/JDK-8209118/webrev.00/
>> Testing: hotspot/jtreg:tier1 (fastdebug)
>>
>> Can I please get reviews?
>>
>> Thanks,
>> Roman
> 
> Oops, we seem to have mis-communicated; I was working on a similar
> change.  You published first, so we'll go with yours; it makes for an
> easy review, since the code looks very familiar :)

Oh, sorry. Well possible that I missed something between travelling and
some crazy stuff here.

> Since we've ended up with G1SATBMarkQueueSet, I now think the
> filtering that I recently moved into [G1]SATBMarkQueueFilter is better
> placed directly in the QSet.  I already have code to merge the
> filtering into the QSet and eliminate the separate Filter object; I'll
> merge that with your changes here as a followup.

Cool, thanks!

> One other piece of work related to sharing the SATB buffer code is
> G1SATBBufferEnqueueingThresholdPercent.  That's going to need to be
> renamed or something, which will involve the CSR process.

Yeah. Or else introduce new $GCSATBBufferEnqueueingThresholdPercent ;-)

> Looks good, other than a code placement question and a couple of
> whitespace issues.  I don't need another webrev for whitespace-only
> cleanups.
> 
> ------------------------------------------------------------------------------
> src/hotspot/share/gc/g1/satbMarkQueue.hpp
> src/hotspot/share/gc/g1/satbMarkQueue.cpp
> 
> The new G1SATBMarkQueueSet will need to be moved to new file
> g1SATBMarkQueueSet.[ch]pp.  Do you want to do this now, or as another
> step?

I wanted to do this when I actually move around stuff. But what the
hell, I can do it right now just as well ;-) See below.

> ------------------------------------------------------------------------------
> src/hotspot/share/gc/g1/g1BarrierSet.hpp
>   42   static G1SATBMarkQueueSet  _satb_mark_queue_set;
> 
> Remove extra no longer aligning space between type and member name.

Done.

> ------------------------------------------------------------------------------
> cpu/sparc/gc/g1/g1BarrierSetAssembler_sparc.cpp
>  607   __ call_VM_leaf(L7_thread_cache,
>  608                   CAST_FROM_FN_PTR(address,
>  609                                    G1SATBMarkQueueSet::handle_zero_index_for_thread),
>  610                                    G2_thread);
> 
> [pre-existing] The G2_thread argument is seriously misleadingly
> indented.  There's a similarly bad indentation for the
> DirtyCardQueueSet case around line 694.

Ugh. I fixed both too.

Incremental:
http://cr.openjdk.java.net/~rkennke/JDK-8209118/webrev.01.diff/
Full:
http://cr.openjdk.java.net/~rkennke/JDK-8209118/webrev.01/

Better?

Thanks,
Roman