RFR: 8305566: ZGC: gc/stringdedup/TestStringDeduplicationFullGC.java#Z failed with SIGSEGV in ZBarrier::weak_load_barrier_on_phantom_oop_slow_path
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. - Commit messages: - fix stray tab - move is_active_Java_thread - copyrights - servicabilty support - use JavaThread - separate thread class - simplify init - do not pass around STS joiner - remove no longer needed Phase enum - remove block_phase et al Changes: https://git.openjdk.org/jdk/pull/13607/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=13607&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8305566 Stats: 440 lines in 18 files changed: 193 ins; 146 del; 101 mod Patch: https://git.openjdk.org/jdk/pull/13607.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13607/head:pull/13607 PR: https://git.openjdk.org/jdk/pull/13607
Re: RFR: 8305566: ZGC: gc/stringdedup/TestStringDeduplicationFullGC.java#Z failed with SIGSEGV in ZBarrier::weak_load_barrier_on_phantom_oop_slow_path
On Mon, 24 Apr 2023 08:24:53 GMT, Kim Barrett 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. I think this looks sensible to me, though I'm not very familiar with the current StringDedup code, so consider this a partial review only. src/hotspot/share/gc/shared/stringdedup/stringDedupProcessor.hpp line 29: > 27: > 28: #include "memory/allocation.hpp" > 29: #include "gc/shared/stringdedup/stringDedup.hpp" sort order - Marked as reviewed by stefank (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/13607#pullrequestreview-1397500893 PR Review Comment: https://git.openjdk.org/jdk/pull/13607#discussion_r1174989267
Re: RFR: 8305566: ZGC: gc/stringdedup/TestStringDeduplicationFullGC.java#Z failed with SIGSEGV in ZBarrier::weak_load_barrier_on_phantom_oop_slow_path [v2]
On Mon, 24 Apr 2023 09:00:53 GMT, Stefan Karlsson wrote: >> Kim Barrett has updated the pull request incrementally with one additional >> commit since the last revision: >> >> fix include order > > src/hotspot/share/gc/shared/stringdedup/stringDedupProcessor.hpp line 29: > >> 27: >> 28: #include "memory/allocation.hpp" >> 29: #include "gc/shared/stringdedup/stringDedup.hpp" > > sort order oops. fixed. - PR Review Comment: https://git.openjdk.org/jdk/pull/13607#discussion_r1175009838
Re: RFR: 8305566: ZGC: gc/stringdedup/TestStringDeduplicationFullGC.java#Z failed with SIGSEGV in ZBarrier::weak_load_barrier_on_phantom_oop_slow_path [v2]
> 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 - Changes: - all: https://git.openjdk.org/jdk/pull/13607/files - new: https://git.openjdk.org/jdk/pull/13607/files/d4e94b89..f17cc6be Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13607&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13607&range=00-01 Stats: 2 lines in 1 file changed: 1 ins; 1 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/13607.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13607/head:pull/13607 PR: https://git.openjdk.org/jdk/pull/13607
Re: RFR: 8305566: ZGC: gc/stringdedup/TestStringDeduplicationFullGC.java#Z failed with SIGSEGV in ZBarrier::weak_load_barrier_on_phantom_oop_slow_path
On Mon, 24 Apr 2023 08:24:53 GMT, Kim Barrett 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
Re: RFR: 8305566: ZGC: gc/stringdedup/TestStringDeduplicationFullGC.java#Z failed with SIGSEGV in ZBarrier::weak_load_barrier_on_phantom_oop_slow_path [v2]
On Mon, 24 Apr 2023 09:25:01 GMT, Kim Barrett 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
Re: RFR: 8304915: Create jdk.internal.util.Architecture enum and apply [v17]
On Fri, 21 Apr 2023 20:26:12 GMT, Roger Riggs wrote: >> Define an internal jdk.internal.util.Architecture enumeration and static >> methods to replace uses of the system property `os.arch`. >> The enumeration values are defined to match those used in the build. >> The initial values are: `X64, X86, AARCH64, RISCV64, S390, PPC64` >> Note that `amd64` and `x86_64` in the build are represented by `X64`. >> The value of the system property `os.arch` is unchanged. >> >> The API is similar to the jdk.internal.util.OperatingSystem enum created by >> #[12931](https://git.openjdk.org/jdk/pull/12931). >> Uses in `java.base` and a few others are included but other modules will be >> done in separate PRs. > > Roger Riggs has updated the pull request incrementally with one additional > commit since the last revision: > > Correct comment on isPPC64() and refer to isLittleEndian() instead of > mentioning PPC64LE Thanks for the comment updates! - Marked as reviewed by mdoerr (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/13357#pullrequestreview-1397600716
Re: RFR: 8305566: ZGC: gc/stringdedup/TestStringDeduplicationFullGC.java#Z failed with SIGSEGV in ZBarrier::weak_load_barrier_on_phantom_oop_slow_path [v2]
On Mon, 24 Apr 2023 09:39:07 GMT, Aleksey Shipilev wrote: > I like this simplification a lot, thanks! I am running some Shenandoah > string-dedup tests now. Ran `gc/shenandoah/TestStringDedup*` on x86_64 and AArch64 for 100 times without a problem. These tests usually fail when there are bugs in string dedup. So I think we are clear there! - PR Comment: https://git.openjdk.org/jdk/pull/13607#issuecomment-1520010545
Re: RFR: 8301377: adjust timeout for JLI GetObjectSizeIntrinsicsTest.java subtest again
On Mon, 24 Apr 2023 05:24:20 GMT, Tobias Hartmann wrote: >> Trivial fixes to increase timeouts for tests that timeout under heavy stress: >> [JDK-8301377](https://bugs.openjdk.org/browse/JDK-8301377) adjust timeout >> for JLI GetObjectSizeIntrinsicsTest.java subtest again >> [JDK-8305502](https://bugs.openjdk.org/browse/JDK-8305502) adjust timeouts >> in three more M&M tests >> [JDK-8302607](https://bugs.openjdk.org/browse/JDK-8302607) increase timeout >> for ContinuousCallSiteTargetChange.java > > Looks good. @TobiHartmann - Thanks for the review! My jdk-21+19 stress testing round had no issues with these fixes. - PR Comment: https://git.openjdk.org/jdk/pull/13593#issuecomment-1520457554
Integrated: 8301377: adjust timeout for JLI GetObjectSizeIntrinsicsTest.java subtest again
On Fri, 21 Apr 2023 21:35:07 GMT, Daniel D. Daugherty wrote: > Trivial fixes to increase timeouts for tests that timeout under heavy stress: > [JDK-8301377](https://bugs.openjdk.org/browse/JDK-8301377) adjust timeout for > JLI GetObjectSizeIntrinsicsTest.java subtest again > [JDK-8305502](https://bugs.openjdk.org/browse/JDK-8305502) adjust timeouts in > three more M&M tests > [JDK-8302607](https://bugs.openjdk.org/browse/JDK-8302607) increase timeout > for ContinuousCallSiteTargetChange.java This pull request has now been integrated. Changeset: 4b23bef5 Author:Daniel D. Daugherty URL: https://git.openjdk.org/jdk/commit/4b23bef51df9c1a5bc8f43748a8d6c8d5656 Stats: 9 lines in 5 files changed: 0 ins; 0 del; 9 mod 8301377: adjust timeout for JLI GetObjectSizeIntrinsicsTest.java subtest again 8302607: increase timeout for ContinuousCallSiteTargetChange.java 8305502: adjust timeouts in three more M&M tests Reviewed-by: naoto, lmesnik, thartmann - PR: https://git.openjdk.org/jdk/pull/13593
Re: RFR: 8306033: Resolve multiple definition of 'throwIOException' and friends when statically linking with JDK native libraries
On Mon, 17 Apr 2023 16:56:31 GMT, Jiangli Zhou wrote: > - Make functions 'static' when feasible: > - throwByName() in > src/java.security.jgss/share/native/libj2gss/NativeUtil.c. > - throwByName(), throwIOException() and throwNullPointerException() in > src/java.smartcardio/unix/native/libj2pcsc/pcsc_md.c. > - throwByName() in > src/jdk.crypto.cryptoki/share/native/libj2pkcs11/p11_util.c. > - throwOutOfMemoryError() in > src/java.smartcardio/share/native/libj2pcsc/pcsc.c. > - Move throwDisconnectedRuntimeException() to > src/jdk.crypto.cryptoki/share/native/libj2pkcs11/p11_util.c since it's only > used in the file. Make it static. > - Move throw_internal_error() to > src/jdk.management/unix/native/libmanagement_ext/OperatingSystemImpl.c as > it's only used in the file. Make it static. > > - Rename functions by following the existing naming usages in different > libraries code: > - Rename throwOutOfMemoryError() to gssThrowOutOfMemoryError() in libj2gss. > - Rename throwOutOfMemoryError() to p11ThrowOutOfMemoryError() in > libj2pks11. > - Rename throwNullPointerException() to p11ThrowNullPointerException() in > libj2pks11. > - Rename throwIOException() to p11ThrowIOException() in libj2pks11. > - Rename throwPKCS11RuntimeException() to p11ThrowPKCS11RuntimeException() > in libj2pks11. This function only exists in libj2pks11. The rename is done so > the function naming is consistent with the other throw functions. > > - Remove throw_internal_error() from > src/java.management/share/native/libmanagement/management.h and > src/java.management/share/native/libmanagement/management.c. It's not used. > - Remove throw_internal_error() from > src/jdk.management/share/native/libmanagement_ext/management_ext.h and > src/jdk.management/share/native/libmanagement_ext/management_ext.c. Gentle ping, please help review this PR. Thanks. - PR Comment: https://git.openjdk.org/jdk/pull/13497#issuecomment-1520559203
Re: RFR: 8291555: Implement alternative fast-locking scheme [v60]
On Mon, 17 Apr 2023 20:00:58 GMT, Roman Kennke wrote: >> Roman Kennke has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains 156 commits: >> >> - Merge remote-tracking branch 'upstream/master' into JDK-8291555-v2 >> - A few more LM_ prefixes in 32bit code >> - Replace UseHeavyMonitor with LockingMode == LM_MONITOR >> - Prefix LockingMode constants with LM_* >> - Bunch of comments and typos >> - Don't use NativeAccess in LockStack::contains() >> - RISCV update >> - Put back thread type check in OS::is_lock_owned() >> - Named constants for LockingMode >> - Address David's review comments >> - ... and 146 more: https://git.openjdk.org/jdk/compare/d2ce04bb...d0a448c6 > > Hi there, > what is needed to bring this PR over the approval line? > @rkennke - I'm planning to do another crawl thru review next week. Thanks! That is greatly appeciated! - PR Comment: https://git.openjdk.org/jdk/pull/10907#issuecomment-1520728296
Re: RFR: 8291555: Implement alternative fast-locking scheme [v62]
On Thu, 20 Apr 2023 11:15:47 GMT, Roman Kennke wrote: >> This change adds a fast-locking scheme as an alternative to the current >> stack-locking implementation. It retains the advantages of stack-locking >> (namely fast locking in uncontended code-paths), while avoiding the overload >> of the mark word. That overloading causes massive problems with Lilliput, >> because it means we have to check and deal with this situation when trying >> to access the mark-word. And because of the very racy nature, this turns out >> to be very complex and would involve a variant of the inflation protocol to >> ensure that the object header is stable. (The current implementation of >> setting/fetching the i-hash provides a glimpse into the complexity). >> >> What the original stack-locking does is basically to push a stack-lock onto >> the stack which consists only of the displaced header, and CAS a pointer to >> this stack location into the object header (the lowest two header bits being >> 00 indicate 'stack-locked'). The pointer into the stack can then be used to >> identify which thread currently owns the lock. >> >> This change basically reverses stack-locking: It still CASes the lowest two >> header bits to 00 to indicate 'fast-locked' but does *not* overload the >> upper bits with a stack-pointer. Instead, it pushes the object-reference to >> a thread-local lock-stack. This is a new structure which is basically a >> small array of oops that is associated with each thread. Experience shows >> that this array typcially remains very small (3-5 elements). Using this lock >> stack, it is possible to query which threads own which locks. Most >> importantly, the most common question 'does the current thread own me?' is >> very quickly answered by doing a quick scan of the array. More complex >> queries like 'which thread owns X?' are not performed in very >> performance-critical paths (usually in code like JVMTI or deadlock >> detection) where it is ok to do more complex operations (and we already do). >> The lock-stack is also a new set of GC roots, and would be scanned during >> thread scanning, possibly concurrently, via the normal protocols. >> >> The lock-stack is fixed size, currently with 8 elements. According to my >> experiments with various workloads, this covers the vast majority of >> workloads (in-fact, most workloads seem to never exceed 5 active locks per >> thread at a time). We check for overflow in the fast-paths and when the >> lock-stack is full, we take the slow-path, which would inflate the lock to a >> monitor. That case should be very rare. >> >> In contrast to stack-locking, fast-locking does *not* support recursive >> locking (yet). When that happens, the fast-lock gets inflated to a full >> monitor. It is not clear if it is worth to add support for recursive >> fast-locking. >> >> One trouble is that when a contending thread arrives at a fast-locked >> object, it must inflate the fast-lock to a full monitor. Normally, we need >> to know the current owning thread, and record that in the monitor, so that >> the contending thread can wait for the current owner to properly exit the >> monitor. However, fast-locking doesn't have this information. What we do >> instead is to record a special marker ANONYMOUS_OWNER. When the thread that >> currently holds the lock arrives at monitorexit, and observes >> ANONYMOUS_OWNER, it knows it must be itself, fixes the owner to be itself, >> and then properly exits the monitor, and thus handing over to the contending >> thread. >> >> As an alternative, I considered to remove stack-locking altogether, and only >> use heavy monitors. In most workloads this did not show measurable >> regressions. However, in a few workloads, I have observed severe >> regressions. All of them have been using old synchronized Java collections >> (Vector, Stack), StringBuffer or similar code. The combination of two >> conditions leads to regressions without stack- or fast-locking: 1. The >> workload synchronizes on uncontended locks (e.g. single-threaded use of >> Vector or StringBuffer) and 2. The workload churns such locks. IOW, >> uncontended use of Vector, StringBuffer, etc as such is ok, but creating >> lots of such single-use, single-threaded-locked objects leads to massive >> ObjectMonitor churn, which can lead to a significant performance impact. But >> alas, such code exists, and we probably don't want to punish it if we can >> avoid it. >> >> This change enables to simplify (and speed-up!) a lot of code: >> >> - The inflation protocol is no longer necessary: we can directly CAS the >> (tagged) ObjectMonitor pointer to the object header. >> - Accessing the hashcode could now be done in the fastpath always, if the >> hashcode has been installed. Fast-locked headers can be used directly, for >> monitor-locked objects we can easily reach-through to the displaced header. >> This is safe because Java threads partic
Re: RFR: 8305566: ZGC: gc/stringdedup/TestStringDeduplicationFullGC.java#Z failed with SIGSEGV in ZBarrier::weak_load_barrier_on_phantom_oop_slow_path [v2]
On Mon, 24 Apr 2023 09:25:01 GMT, Kim Barrett 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
Re: RFR: 8306034: add support of virtual threads to JVMTI StopThread [v2]
On Sat, 22 Apr 2023 08:01:20 GMT, Alan Bateman wrote: >> Thank you for the catch. Will check it. I have to extend the test to cover >> the BoundVirtualThread case enabled with the flag `-XX:-VMContinuations`. > > The scenario that I'm wondering about is where a virtual thread is resumed at > around the same time that JVMTI StopThread is called. Not easy to test. Seems we should have a stress test for that. - PR Review Comment: https://git.openjdk.org/jdk/pull/13546#discussion_r1175750223
Re: RFR: 8233725: ProcessTools.startProcess() has output issues when using an OutputAnalyzer at the same time
On Fri, 21 Apr 2023 21:43:39 GMT, Leonid Mesnik wrote: > ProcessTools.startProcess() creates process and read it's output error > streams. So the any other using of corresponding Process.getInputStream() and > Process.getErrorStream() doesn't get process streams. > > This fix preserve process streams content and allow to read reuse the date. > The ByteArrayOutputStream is used as a buffer. > It stores all process output, never trying to clean date which has been read. > > The regression test has been provided with issue. > > I closed previous PR https://github.com/openjdk/jdk/pull/13560 by mistake > instead of updating it. > > I run all tests to ensure that no failures are introduced. Marked as reviewed by cjplummer (Reviewer). test/lib/jdk/test/lib/process/ProcessTools.java line 249: > 247: stdout.addOutputStream(out.getOutputStream()); > 248: stderr.addOutputStream(err.getOutputStream()); > 249: Overall this looks good, although I'm a bit unclear on how some of the underpinnings work, allowing the output to appear in these output streams, and also in the LineForwarder output (above), and for that matter, in the lineConsumer output (below). I guess there is some multiplexing of the output that I just don't grasp, but appears to be something that already worked. - PR Review: https://git.openjdk.org/jdk/pull/13594#pullrequestreview-1398781718 PR Review Comment: https://git.openjdk.org/jdk/pull/13594#discussion_r1175775335
Re: RFR: 8306033: Resolve multiple definition of 'throwIOException' and friends when statically linking with JDK native libraries
I'll take a look. From: security-dev on behalf of Jiangli Zhou Sent: Monday, April 24, 2023 10:27 AM To: jmx-...@openjdk.org ; security-...@openjdk.org ; serviceability-dev@openjdk.org Subject: Re: RFR: 8306033: Resolve multiple definition of 'throwIOException' and friends when statically linking with JDK native libraries On Mon, 17 Apr 2023 16:56:31 GMT, Jiangli Zhou wrote: > - Make functions 'static' when feasible: > - throwByName() in > src/java.security.jgss/share/native/libj2gss/NativeUtil.c. > - throwByName(), throwIOException() and throwNullPointerException() in > src/java.smartcardio/unix/native/libj2pcsc/pcsc_md.c. > - throwByName() in > src/jdk.crypto.cryptoki/share/native/libj2pkcs11/p11_util.c. > - throwOutOfMemoryError() in > src/java.smartcardio/share/native/libj2pcsc/pcsc.c. > - Move throwDisconnectedRuntimeException() to > src/jdk.crypto.cryptoki/share/native/libj2pkcs11/p11_util.c since it's only > used in the file. Make it static. > - Move throw_internal_error() to > src/jdk.management/unix/native/libmanagement_ext/OperatingSystemImpl.c as > it's only used in the file. Make it static. > > - Rename functions by following the existing naming usages in different > libraries code: > - Rename throwOutOfMemoryError() to gssThrowOutOfMemoryError() in libj2gss. > - Rename throwOutOfMemoryError() to p11ThrowOutOfMemoryError() in > libj2pks11. > - Rename throwNullPointerException() to p11ThrowNullPointerException() in > libj2pks11. > - Rename throwIOException() to p11ThrowIOException() in libj2pks11. > - Rename throwPKCS11RuntimeException() to p11ThrowPKCS11RuntimeException() > in libj2pks11. This function only exists in libj2pks11. The rename is done so > the function naming is consistent with the other throw functions. > > - Remove throw_internal_error() from > src/java.management/share/native/libmanagement/management.h and > src/java.management/share/native/libmanagement/management.c. It's not used. > - Remove throw_internal_error() from > src/jdk.management/share/native/libmanagement_ext/management_ext.h and > src/jdk.management/share/native/libmanagement_ext/management_ext.c. Gentle ping, please help review this PR. Thanks. - PR Comment: https://git.openjdk.org/jdk/pull/13497#issuecomment-1520559203
Re: RFR: 8306033: Resolve multiple definition of 'throwIOException' and friends when statically linking with JDK native libraries
On Mon, 17 Apr 2023 16:56:31 GMT, Jiangli Zhou wrote:
> - Make functions 'static' when feasible:
> - throwByName() in
> src/java.security.jgss/share/native/libj2gss/NativeUtil.c.
> - throwByName(), throwIOException() and throwNullPointerException() in
> src/java.smartcardio/unix/native/libj2pcsc/pcsc_md.c.
> - throwByName() in
> src/jdk.crypto.cryptoki/share/native/libj2pkcs11/p11_util.c.
> - throwOutOfMemoryError() in
> src/java.smartcardio/share/native/libj2pcsc/pcsc.c.
> - Move throwDisconnectedRuntimeException() to
> src/jdk.crypto.cryptoki/share/native/libj2pkcs11/p11_util.c since it's only
> used in the file. Make it static.
> - Move throw_internal_error() to
> src/jdk.management/unix/native/libmanagement_ext/OperatingSystemImpl.c as
> it's only used in the file. Make it static.
>
> - Rename functions by following the existing naming usages in different
> libraries code:
> - Rename throwOutOfMemoryError() to gssThrowOutOfMemoryError() in libj2gss.
> - Rename throwOutOfMemoryError() to p11ThrowOutOfMemoryError() in
> libj2pks11.
> - Rename throwNullPointerException() to p11ThrowNullPointerException() in
> libj2pks11.
> - Rename throwIOException() to p11ThrowIOException() in libj2pks11.
> - Rename throwPKCS11RuntimeException() to p11ThrowPKCS11RuntimeException()
> in libj2pks11. This function only exists in libj2pks11. The rename is done so
> the function naming is consistent with the other throw functions.
>
> - Remove throw_internal_error() from
> src/java.management/share/native/libmanagement/management.h and
> src/java.management/share/native/libmanagement/management.c. It's not used.
> - Remove throw_internal_error() from
> src/jdk.management/share/native/libmanagement_ext/management_ext.h and
> src/jdk.management/share/native/libmanagement_ext/management_ext.c.
Update copyrights to 2023.
src/java.security.jgss/share/native/libj2gss/GSSLibStub.c line 201:
> 199: cb = malloc(sizeof(struct gss_channel_bindings_struct));
> 200: if (cb == NULL) {
> 201: gssThrowOutOfMemoryError(env,NULL);
While you're fixing this, add a space between arguments, e.g. `(env,NULL)
`becomes `(env, NULL)`.
src/java.security.jgss/share/native/libj2gss/NativeUtil.c line 456:
> 454:
> 455: /* Throws a Java Exception by name */
> 456: static void throwByName(JNIEnv *env, const char *name, const char *msg) {
Why can't you move the few lines of `throwByName()` into
`gssThrowOutOfMemoryError()` and totally remove `throwByName()`?
-
PR Review: https://git.openjdk.org/jdk/pull/13497#pullrequestreview-1398895019
PR Review Comment: https://git.openjdk.org/jdk/pull/13497#discussion_r1175839776
PR Review Comment: https://git.openjdk.org/jdk/pull/13497#discussion_r1175840162
Re: RFR: 8306033: Resolve multiple definition of 'throwIOException' and friends when statically linking with JDK native libraries [v2]
On Mon, 24 Apr 2023 22:41:53 GMT, Mark Powers wrote:
> Update copyrights to 2023.
Done, thanks.
> src/java.security.jgss/share/native/libj2gss/GSSLibStub.c line 201:
>
>> 199: cb = malloc(sizeof(struct gss_channel_bindings_struct));
>> 200: if (cb == NULL) {
>> 201: gssThrowOutOfMemoryError(env,NULL);
>
> While you're fixing this, add a space between arguments, e.g. `(env,NULL)
> `becomes `(env, NULL)`.
Done, thanks.
> src/java.security.jgss/share/native/libj2gss/NativeUtil.c line 456:
>
>> 454:
>> 455: /* Throws a Java Exception by name */
>> 456: static void throwByName(JNIEnv *env, const char *name, const char *msg)
>> {
>
> Why can't you move the few lines of `throwByName()` into
> `gssThrowOutOfMemoryError()` and totally remove `throwByName()`?
Updated as suggested, thanks.
-
PR Comment: https://git.openjdk.org/jdk/pull/13497#issuecomment-1521007212
PR Review Comment: https://git.openjdk.org/jdk/pull/13497#discussion_r1175910048
PR Review Comment: https://git.openjdk.org/jdk/pull/13497#discussion_r1175910110
Re: RFR: 8306033: Resolve multiple definition of 'throwIOException' and friends when statically linking with JDK native libraries [v2]
> - Make functions 'static' when feasible: > - throwByName() in > src/java.security.jgss/share/native/libj2gss/NativeUtil.c. > - throwByName(), throwIOException() and throwNullPointerException() in > src/java.smartcardio/unix/native/libj2pcsc/pcsc_md.c. > - throwByName() in > src/jdk.crypto.cryptoki/share/native/libj2pkcs11/p11_util.c. > - throwOutOfMemoryError() in > src/java.smartcardio/share/native/libj2pcsc/pcsc.c. > - Move throwDisconnectedRuntimeException() to > src/jdk.crypto.cryptoki/share/native/libj2pkcs11/p11_util.c since it's only > used in the file. Make it static. > - Move throw_internal_error() to > src/jdk.management/unix/native/libmanagement_ext/OperatingSystemImpl.c as > it's only used in the file. Make it static. > > - Rename functions by following the existing naming usages in different > libraries code: > - Rename throwOutOfMemoryError() to gssThrowOutOfMemoryError() in libj2gss. > - Rename throwOutOfMemoryError() to p11ThrowOutOfMemoryError() in > libj2pks11. > - Rename throwNullPointerException() to p11ThrowNullPointerException() in > libj2pks11. > - Rename throwIOException() to p11ThrowIOException() in libj2pks11. > - Rename throwPKCS11RuntimeException() to p11ThrowPKCS11RuntimeException() > in libj2pks11. This function only exists in libj2pks11. The rename is done so > the function naming is consistent with the other throw functions. > > - Remove throw_internal_error() from > src/java.management/share/native/libmanagement/management.h and > src/java.management/share/native/libmanagement/management.c. It's not used. > - Remove throw_internal_error() from > src/jdk.management/share/native/libmanagement_ext/management_ext.h and > src/jdk.management/share/native/libmanagement_ext/management_ext.c. Jiangli Zhou has updated the pull request incrementally with one additional commit since the last revision: Minor updates, as suggested by mcpowers: - Update copyright headers in affected files. - Formatting update in src/java.security.jgss/share/native/libj2gss/GSSLibStub.c. - Remove throwByName() and move the logic into gssThrowOutOfMemoryError() (the only caller) in src/java.security.jgss/share/native/libj2gss/NativeUtil.c. - Changes: - all: https://git.openjdk.org/jdk/pull/13497/files - new: https://git.openjdk.org/jdk/pull/13497/files/9d319df6..fcb35192 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13497&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13497&range=00-01 Stats: 35 lines in 25 files changed: 0 ins; 6 del; 29 mod Patch: https://git.openjdk.org/jdk/pull/13497.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13497/head:pull/13497 PR: https://git.openjdk.org/jdk/pull/13497
Re: RFR: 8233725: ProcessTools.startProcess() has output issues when using an OutputAnalyzer at the same time
On Mon, 24 Apr 2023 20:57:31 GMT, Chris Plummer wrote: >> ProcessTools.startProcess() creates process and read it's output error >> streams. So the any other using of corresponding Process.getInputStream() >> and Process.getErrorStream() doesn't get process streams. >> >> This fix preserve process streams content and allow to read reuse the date. >> The ByteArrayOutputStream is used as a buffer. >> It stores all process output, never trying to clean date which has been >> read. >> >> The regression test has been provided with issue. >> >> I closed previous PR https://github.com/openjdk/jdk/pull/13560 by mistake >> instead of updating it. >> >> I run all tests to ensure that no failures are introduced. > > test/lib/jdk/test/lib/process/ProcessTools.java line 249: > >> 247: stdout.addOutputStream(out.getOutputStream()); >> 248: stderr.addOutputStream(err.getOutputStream()); >> 249: > > Overall this looks good, although I'm a bit unclear on how some of the > underpinnings work, allowing the output to appear in these output streams, > and also in the LineForwarder output (above), and for that matter, in the > lineConsumer output (below). I guess there is some multiplexing of the output > that I just don't grasp, but appears to be something that already worked. StreamPumper read process stdout, stderr streams and write read data to registered streams. So it works as multiplexer. - PR Review Comment: https://git.openjdk.org/jdk/pull/13594#discussion_r1175930914
Re: RFR: 8306034: add support of virtual threads to JVMTI StopThread [v2]
On Mon, 24 Apr 2023 20:26:31 GMT, Chris Plummer wrote: >> The scenario that I'm wondering about is where a virtual thread is resumed >> at around the same time that JVMTI StopThread is called. Not easy to test. > > Seems we should have a stress test for that. > The scenario that I'm wondering about is where a virtual thread is resumed > at around the same time that JVMTI StopThread is called. This kind of scenario is not typical. The debugger should keep virtual thread suspended when making a call to JVMTI StopThread. Normally, this kind of race should never happen with the JDWP agent. Chris, why do you think it is important to have a stress test for this? Also, do you have any testing scenario in mind? - PR Review Comment: https://git.openjdk.org/jdk/pull/13546#discussion_r1175944953
Re: RFR: 8233725: ProcessTools.startProcess() has output issues when using an OutputAnalyzer at the same time
On Fri, 21 Apr 2023 21:43:39 GMT, Leonid Mesnik wrote:
> ProcessTools.startProcess() creates process and read it's output error
> streams. So the any other using of corresponding Process.getInputStream() and
> Process.getErrorStream() doesn't get process streams.
>
> This fix preserve process streams content and allow to read reuse the date.
> The ByteArrayOutputStream is used as a buffer.
> It stores all process output, never trying to clean date which has been read.
>
> The regression test has been provided with issue.
>
> I closed previous PR https://github.com/openjdk/jdk/pull/13560 by mistake
> instead of updating it.
>
> I run all tests to ensure that no failures are introduced.
test/lib/jdk/test/lib/process/ProcessTools.java line 792:
> 790: @Override
> 791: public InputStream getInputStream() {
> 792: return out;
This is a little bit confusing that the `getInputStream()` returns `out` stream.
Just wanted to double-check if it is intentional and was not needed for
`getOutputStream()` instead.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13594#discussion_r1175965035
Re: RFR: 8306034: add support of virtual threads to JVMTI StopThread [v2]
On Tue, 25 Apr 2023 02:15:54 GMT, Serguei Spitsyn wrote: >> Seems we should have a stress test for that. > >> The scenario that I'm wondering about is where a virtual thread is resumed >> at around the same time that JVMTI StopThread is called. > > This kind of scenario is not typical. > The debugger should keep virtual thread suspended when making a call to JVMTI > StopThread. > Normally, this kind of race should never happen with the JDWP agent. > Chris, why do you think it is important to have a stress test for this? > Also, do you have any testing scenario in mind? I guess if it is not something that would typically ever be done in realistic situations, then there is no need for a stress test. - PR Review Comment: https://git.openjdk.org/jdk/pull/13546#discussion_r1175979665
Re: RFR: 8233725: ProcessTools.startProcess() has output issues when using an OutputAnalyzer at the same time
On Tue, 25 Apr 2023 03:06:09 GMT, Serguei Spitsyn wrote:
>> ProcessTools.startProcess() creates process and read it's output error
>> streams. So the any other using of corresponding Process.getInputStream()
>> and Process.getErrorStream() doesn't get process streams.
>>
>> This fix preserve process streams content and allow to read reuse the date.
>> The ByteArrayOutputStream is used as a buffer.
>> It stores all process output, never trying to clean date which has been
>> read.
>>
>> The regression test has been provided with issue.
>>
>> I closed previous PR https://github.com/openjdk/jdk/pull/13560 by mistake
>> instead of updating it.
>>
>> I run all tests to ensure that no failures are introduced.
>
> test/lib/jdk/test/lib/process/ProcessTools.java line 792:
>
>> 790: @Override
>> 791: public InputStream getInputStream() {
>> 792: return out;
>
> This is a little bit confusing that the `getInputStream()` returns `out`
> stream.
> Just wanted to double-check if it is intentional and was not needed for
> `getOutputStream()` instead.
Agree, it is confusing, even in standard j.l.Process API . The `InputStream
java.lang.Process.getInputStream()`" returns **output** stream of started
process. So for our implementation ProcessImpl the 'out' and 'err' mean output
and error streams. However they are returned as InputStreams so users could
read them.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13594#discussion_r1175985058
Re: RFR: 8306034: add support of virtual threads to JVMTI StopThread [v4]
> This enhancement adds support of virtual threads to the JVMTI `StopThread` > function. > In preview releases before this enhancement the StopThread returned the > JVMTI_ERROR_UNSUPPORTED_OPERATION error code for virtual threads. > > The `StopThread` supports sending an asynchronous exception to a virtual > thread only if it is current or suspended at mounted state. For instance, a > virtual thread can be suspended at a JVMTI event. If the virtual thread is > not suspended and is not current then the `JVMTI_ERROR_THREAD_NOT_SUSPENDED` > error code is returned. If the virtual thread was suspended at unmounted > state then the `JVMTI_ERROR_OPAQUE_FRAME` error code is returned. > > The `StopThread` has the following description for `JVMTI_ERROR_OPAQUE_FRAME` > error code: >> The thread is a suspended virtual thread and the implementation >> was unable to throw an asynchronous exception from this frame. > > A couple of the `serviceability/jvmti/vthread` tests has been updated to > adopt to new `StopThread` behavior. > > The CSR is: https://bugs.openjdk.org/browse/JDK-8306434 > > Testing: > The mach5 tears 1-6 are in progress. > Preliminary test runs were good in general. > The JDB test `vmTestbase/nsk/jdb/kill/kill001/kill001.java` has been > problem-listed and will be fixed by the corresponding debugger enhancement > which is going to adopt JDWP/JDI specs to new behavior of the JVMTI > `StopThread` related to virtual threads. > > Also, two JCK JVMTI tests are failing in the tier-6 : >> vm/jvmti/StopThread/stop001/stop00103/stop00103.html >> vm/jvmti/StopThread/stop001/stop00103/stop00103a.html > > These two tests will be excluded from the test runs by the JCK team and then > adjusted to new `StopThread` behavior. Serguei Spitsyn has updated the pull request incrementally with one additional commit since the last revision: 1. Address review comments 2. Clear interrupt bit in the TestTaskThread - Changes: - all: https://git.openjdk.org/jdk/pull/13546/files - new: https://git.openjdk.org/jdk/pull/13546/files/d2cc010e..dbdb4edd Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13546&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13546&range=02-03 Stats: 53 lines in 1 file changed: 31 ins; 7 del; 15 mod Patch: https://git.openjdk.org/jdk/pull/13546.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13546/head:pull/13546 PR: https://git.openjdk.org/jdk/pull/13546
Re: RFR: 8306034: add support of virtual threads to JVMTI StopThread [v2]
On Tue, 25 Apr 2023 03:49:09 GMT, Chris Plummer wrote: >>> The scenario that I'm wondering about is where a virtual thread is resumed >>> at around the same time that JVMTI StopThread is called. >> >> This kind of scenario is not typical. >> The debugger should keep virtual thread suspended when making a call to >> JVMTI StopThread. >> Normally, this kind of race should never happen with the JDWP agent. >> Chris, why do you think it is important to have a stress test for this? >> Also, do you have any testing scenario in mind? > > I guess if it is not something that would typically ever be done in realistic > situations, then there is no need for a stress test. Okay, thanks. I've extended the test to cover BoundVirtualThread's as well. - PR Review Comment: https://git.openjdk.org/jdk/pull/13546#discussion_r1175995873
Re: RFR: 8306034: add support of virtual threads to JVMTI StopThread [v3]
On Sat, 22 Apr 2023 00:09:27 GMT, Serguei Spitsyn wrote: >> For the JDI tests I added, I execute them in both modes, with the >> appropriate adjustments to account for the errors we except for virtual >> threads. We should be testing to make sure that StopThread works with >> platform threads under a variety of situations. > > Extending this test to cover platform threads does not look natural and is > going to be a little ugly. > But I can extend it to provide coverage for BoundVirtualThread case > which is highjacking the platform thread implementation. > Would it help? > We should have pretty good coverage of the JVMTI `StopThread` for platform > threads in `nsk.jvmti` test suite. > It includes: > - `stopthrd006` and `stopthrd007` > - a number of `scenarios/capability/CM01 `tests > > Also, this extension does not touch the code path of platform threads support. I've extended the test to cover platform threads as well. - PR Review Comment: https://git.openjdk.org/jdk/pull/13546#discussion_r1175995443
