RFR: 8305566: ZGC: gc/stringdedup/TestStringDeduplicationFullGC.java#Z failed with SIGSEGV in ZBarrier::weak_load_barrier_on_phantom_oop_slow_path [v2]

[Aleksey Shipilev]

On Mon, 24 Apr 2023 09:25:01 GMT, Kim Barrett <kbarrett at openjdk.org> wrote:

>> Please review this change to the string deduplication thread to make it a kind
>> of JavaThread rather than a ConcurrentGCThread.  There are several pieces to
>> this change:
>> 
>> (1) New class StringDedupThread (derived from JavaThread), separate from
>> StringDedup::Processor (which is now just a CHeapObj instead of deriving from
>> ConcurrentGCThread).  The thread no longer needs to or supports being stopped,
>> like other similar threads.  It also needs to be started later, once Java
>> threads are supported.  Also don't need an explicit visitor, since it will be
>> in the normal Java threads list.  This separation made the changeover a little
>> cleaner to develop, and made the servicability support a little cleaner too.
>> 
>> (2) The Processor now uses the ThreadBlockInVM idiom to be safepoint polite,
>> instead of using the SuspendibleThreadSet facility.
>> 
>> (3) Because we're using ThreadBlockInVM, which has a different usage style
>> from STS, the tracking of time spent by the processor blocked for safepoints
>> doesn't really work.  It's not very important anyway, since normal thread
>> descheduling can also affect the normal processing times being gathered and
>> reported.  So we just drop the so-called "blocked" time and associated
>> infrastructure, simplifying Stat tracking a bit.  Also renamed the
>> "concurrent" stat to be "active", since it's all in a JavaThread now.
>> 
>> (4) To avoid #include problems, moved the definition of
>> JavaThread::is_active_Java_thread from the .hpp file to the .inline.hpp file,
>> where one of the functions it calls also is defined.
>> 
>> (5) Added servicability support for the new thread.
>> 
>> Testing:
>> mach5 tier1-3 with -XX:+UseStringDeduplication.
>> The test runtime/cds/DeterministicDump.java fails intermittently with that
>> option, which is not surprising - see JDK-8306712.
>> 
>> I was never able to reproduce the failure; it's likely quite timing sensitive.
>> The fix of changing the type is based on StefanK's comment that ZResurrection
>> doesn't expect a non-Java thread to perform load-barriers.
>
> Kim Barrett has updated the pull request incrementally with one additional commit since the last revision:
> 
>   fix include order

I like this simplification a lot, thanks! I am running some Shenandoah string-dedup tests now.

(Formally requesting the change of synopsis)

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

Marked as reviewed by shade (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/13607#pullrequestreview-1397574534
Changes requested by shade (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/13607#pullrequestreview-1397576017