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