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

Kim Barrett kim.barrett at oracle.com
Wed Aug 8 14:01:14 UTC 2018


> On Aug 8, 2018, at 5:23 AM, Roman Kennke <rkennke at redhat.com> wrote:
> 
> 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 :)

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.

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.

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?

------------------------------------------------------------------------------
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.

------------------------------------------------------------------------------
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.

------------------------------------------------------------------------------




More information about the hotspot-gc-dev mailing list