On Mon, 24 Apr 2023 09:25:01 GMT, Kim Barrett <kbarr...@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 SA changes look good ------------- Marked as reviewed by cjplummer (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/13607#pullrequestreview-1398739628
