On Mon, 24 Apr 2023 08:24:53 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. Could we please change the synopsis to something more relevant? This PR does not only fix the ZGC test failure, but does a more fundamental change: switching string dedup thread from being `ConcurrentGCThread` to `JavaThread`, and so it affects more than one GC. The synopsis should reflect that, I think. (This would also be cleaner for potential backports, if any). ------------- PR Comment: https://git.openjdk.org/jdk/pull/13607#issuecomment-1519707606
