Re: JEP draft: Disallow the Dynamic Loading of Agents by Default
Hi Ron, Thanks for writing up the JEP draft outlining the proposal to disallow dynamic loading of agents by default. The Red Hat Java team has continued to discuss this proposal internally and with our stakeholders. While there is a general agreement (or at least acceptance) with the overall direction of this proposal, we still see the concerns raised by Andrew [0] as needing to be addressed. So let’s start with the high-order bit: timing. The JEP draft states it “intends to adopt the proposal [to disable agents by default] six years later, in 2023, for JDK 21.” We would like to see this targeted to JDK 22 instead as the change has not been widely evangelized yet and comes as a surprise to many, both to those actively developing OpenJDK and to those monitoring its development. We owe it to the community to give this proposal enough bake time, especially given that JDK 21 is an LTS release. Though the official position is that LTS releases are no different than any other release, the actions of JDK consumers make them special. Users will be surprised by this change when they migrate from JDK 17 to 21. If we delay till JDK 22, we give the ecosystem time to adapt to the change before the next LTS. Additionally, users who have tested with the -XX:-EnableDynamicAgentLoading option will have false expectations if they validated their use of jcmd to load the agent as the behaviour was not correct prior to JDK 21 [1]. The next concern is one you call out in the draft that “Java's excellent serviceability has long been a source of pride for the platform.” We agree! Java currently has an excellent, prime position in Observability capabilities. For better or for worse, there are many Observability tools that have relied on dynamic attach to provide the necessary monitoring for workloads It’s important we give Java’s monitoring tools sufficient time to migrate comfortably without shocking the ecosystem by changing the default in an LTS. By delaying the change till JDK 22, we give the ecosystem 2 years to migrate and to prepare users for the change. Additionally, this provides the time for Java’s profiling tools to adapt as well. And for the “ad-hoc troubleshooting” tools - like Byteman - to retrain their users. Finally, while it’s easy to agree with the principle that “the application owner is given the final say on whether to allow dynamic loading of agents”, the argument can (and should!) be made that those application owners have made that final decision by deploying the libraries and tools that use dynamic attach. A JVM command line argument is not the only way to express their consent for code to run on their system. For many production uses, the reality is more complicated than a single “application owner”. Take a Kubernetes cluster for example. Who is the application owner when deploying to a Kubernetes cluster? The dev team that develops the application? The operations team that manages the cluster? The monitoring team that monitors the applications? The Support team that troubleshoots issues with the deployment? We should be careful not to understate or misrepresent the complex web of “owners” that are currently able (and, for business reasons, need) to apply agents dynamically. Downplaying the complexity many organizations experience when dealing with changes to command line options (as an example) weakens the argument for changing today’s status quo. We also know that in many cases customers and users may not be in a position to modify startup scripts (e.g. even to add in an extra parameter) as to do so may invalidate support contracts, etc. Dynamically attached agents have been a “superpower” of Java and their use has greatly benefited the ecosystem as they’ve helped service and support use cases that otherwise aren’t possible, as they helped propel Java to the forefront of the Observability tooling, and allowed many other useful libraries to be developed. Let’s delay flipping the default until JDK 22 to give the breadth of the ecosystem time to absorb this change. –Dan [0] https://mail.openjdk.org/pipermail/serviceability-dev/2023-March/047084.html [1] https://bugs.openjdk.org/browse/JDK-8304438
Re: RFR: 8306471: Add virtual threads support to JDWP ThreadReference.Stop and JDI ThreadReference.stop() [v4]
> Note this PR depends on the #13546 PR for the following: > > [JDK-8306434](https://bugs.openjdk.org/browse/JDK-8306434): add support of > virtual threads to JVMTI StopThread > > So it can't be finalized and push until after JDK-8306434 is pushed. There > will also be GHA failures until then. If JDK-8306434 results in any > additional spec changes, they will likely impact this CR also. > > > [JDK-8306034](https://bugs.openjdk.org/browse/JDK-8306034) is adding some > virtual thread support to JVMTI StopThread. This will allow JDWP > ThreadReference.Stop and JDI ThreadReference.stop() to have the same level > support for virtual threads. > > Mostly this is a spec update for JDWP and JDI, but there are also some minor > changes needed to the ThreadReference.stop() handling of JDWP errors, and > throwing the appropriate exceptions. Also some minor cleanup in jdb. The > debug agent doesn't need changes since JVMTI errors are just passed through > as the corresponding JDWP errors, and the code for this is already in place. > These errors are not new nor need any special handling. > > Our existing testing for ThreadReference.stop() is fairly weak: > > - nsk/jdb/kill/kill001, which tests stop() when the thread is suspended at a > breakpoint. It will get problem listed by > [JDK-8306034](https://bugs.openjdk.org/browse/JDK-8306034). I have fixes for > it already working and will push it separately. > - nsk/jdi/stop/stop001, which is problem listed and only tests when the > thread is blocked in Object.wait() > - nsk/jdi/stop/stop002, which only tests that throwing an invalid exception > fails properly > > I decided to take stop002 and make it a more thorough test of > ThreadReference.stop(). See the comment at the top for a list of what is > tested. As for reviewing this test, it's probably best to look at it as a > completely new test rather than looking at diffs since so much has been > changed and added. Chris Plummer has updated the pull request incrementally with one additional commit since the last revision: Some test logging improvements. - Changes: - all: https://git.openjdk.org/jdk/pull/13548/files - new: https://git.openjdk.org/jdk/pull/13548/files/745ff33d..801362e5 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13548=03 - incr: https://webrevs.openjdk.org/?repo=jdk=13548=02-03 Stats: 19 lines in 2 files changed: 7 ins; 1 del; 11 mod Patch: https://git.openjdk.org/jdk/pull/13548.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13548/head:pull/13548 PR: https://git.openjdk.org/jdk/pull/13548
Re: RFR: 8291555: Implement alternative fast-locking scheme [v67]
On Fri, 28 Apr 2023 19:23:24 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
Re: RFR: 8306471: Add virtual threads support to JDWP ThreadReference.Stop and JDI ThreadReference.stop() [v3]
> Note this PR depends on the #13546 PR for the following: > > [JDK-8306434](https://bugs.openjdk.org/browse/JDK-8306434): add support of > virtual threads to JVMTI StopThread > > So it can't be finalized and push until after JDK-8306434 is pushed. There > will also be GHA failures until then. If JDK-8306434 results in any > additional spec changes, they will likely impact this CR also. > > > [JDK-8306034](https://bugs.openjdk.org/browse/JDK-8306034) is adding some > virtual thread support to JVMTI StopThread. This will allow JDWP > ThreadReference.Stop and JDI ThreadReference.stop() to have the same level > support for virtual threads. > > Mostly this is a spec update for JDWP and JDI, but there are also some minor > changes needed to the ThreadReference.stop() handling of JDWP errors, and > throwing the appropriate exceptions. Also some minor cleanup in jdb. The > debug agent doesn't need changes since JVMTI errors are just passed through > as the corresponding JDWP errors, and the code for this is already in place. > These errors are not new nor need any special handling. > > Our existing testing for ThreadReference.stop() is fairly weak: > > - nsk/jdb/kill/kill001, which tests stop() when the thread is suspended at a > breakpoint. It will get problem listed by > [JDK-8306034](https://bugs.openjdk.org/browse/JDK-8306034). I have fixes for > it already working and will push it separately. > - nsk/jdi/stop/stop001, which is problem listed and only tests when the > thread is blocked in Object.wait() > - nsk/jdi/stop/stop002, which only tests that throwing an invalid exception > fails properly > > I decided to take stop002 and make it a more thorough test of > ThreadReference.stop(). See the comment at the top for a list of what is > tested. As for reviewing this test, it's probably best to look at it as a > completely new test rather than looking at diffs since so much has been > changed and added. Chris Plummer has updated the pull request incrementally with one additional commit since the last revision: Don't rely in Thread.sleep() to sychronize between debugger and debuggee. - Changes: - all: https://git.openjdk.org/jdk/pull/13548/files - new: https://git.openjdk.org/jdk/pull/13548/files/f1ed2c6f..745ff33d Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13548=02 - incr: https://webrevs.openjdk.org/?repo=jdk=13548=01-02 Stats: 39 lines in 2 files changed: 26 ins; 0 del; 13 mod Patch: https://git.openjdk.org/jdk/pull/13548.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13548/head:pull/13548 PR: https://git.openjdk.org/jdk/pull/13548
Re: RFR: 8306851: Move Method access flags [v5]
> This change moves the flags from AccessFlags to either ConstMethodFlags or > MethodFlags, depending on whether they are set at class file parse time, > which makes them essentially const, or at runtime, which makes them needing > atomic access. > > This leaves AccessFlags int size because Klass still has JVM flags that are > more work to move, but this change doesn't increase Method size. I didn't > remove JVM_RECOGNIZED_METHOD_MODIFIERS with this change since there are > several of these in other places, and with this change the code is benign. > > Tested with tier1-6, and some manual verification of printing. Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision: Fix constMethod printing. - Changes: - all: https://git.openjdk.org/jdk/pull/13654/files - new: https://git.openjdk.org/jdk/pull/13654/files/9b05ab84..d0de72ac Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13654=04 - incr: https://webrevs.openjdk.org/?repo=jdk=13654=03-04 Stats: 6 lines in 3 files changed: 1 ins; 1 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/13654.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13654/head:pull/13654 PR: https://git.openjdk.org/jdk/pull/13654
Re: RFR: 8306851: Move Method access flags [v3]
On Fri, 28 Apr 2023 18:39:21 GMT, Coleen Phillimore wrote:
>> src/hotspot/share/oops/constMethod.cpp line 438:
>>
>>> 436: }
>>> 437: st->cr();
>>> 438: st->print(" - flags: "); _flags.print_on(st);
>>>st->cr();
>>
>> Method prints its flags as an int and in decoded form, but ConstMethod only
>> prints the decoded form. Any particular reason for this difference?
>
> No reason for this difference. The only reason I added the int form for
> MethodFlags was because AccessFlags were printed also with the int form.
I fixed the constMethod printing with the last commit.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13654#discussion_r1180757104
RFR: 8307067: remove broken EnableThreadSMRExtraValidityChecks option
A trivial fix to remove broken EnableThreadSMRExtraValidityChecks option. - Commit messages: - 8307067: remove broken EnableThreadSMRExtraValidityChecks option Changes: https://git.openjdk.org/jdk/pull/13704/files Webrev: https://webrevs.openjdk.org/?repo=jdk=13704=00 Issue: https://bugs.openjdk.org/browse/JDK-8307067 Stats: 11 lines in 3 files changed: 0 ins; 7 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/13704.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13704/head:pull/13704 PR: https://git.openjdk.org/jdk/pull/13704
Re: RFR: 8306851: Move Method access flags [v4]
On Fri, 28 Apr 2023 15:42:58 GMT, Coleen Phillimore wrote: >> This change moves the flags from AccessFlags to either ConstMethodFlags or >> MethodFlags, depending on whether they are set at class file parse time, >> which makes them essentially const, or at runtime, which makes them needing >> atomic access. >> >> This leaves AccessFlags int size because Klass still has JVM flags that are >> more work to move, but this change doesn't increase Method size. I didn't >> remove JVM_RECOGNIZED_METHOD_MODIFIERS with this change since there are >> several of these in other places, and with this change the code is benign. >> >> Tested with tier1-6, and some manual verification of printing. > > Coleen Phillimore has updated the pull request incrementally with one > additional commit since the last revision: > > Updates Looks good, thanks! - Marked as reviewed by matsaave (Committer). PR Review: https://git.openjdk.org/jdk/pull/13654#pullrequestreview-1406485565
Re: RFR: 8291555: Implement alternative fast-locking scheme [v67]
> 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 p rotocols. > > 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 participate in monitor deflation protocol. > This would be implemented in a separate PR > > Also, and I might be mistaken here, this new
Re: RFR: 8291555: Implement alternative fast-locking scheme [v66]
On Fri, 28 Apr 2023 16:48:12 GMT, Daniel D. Daugherty wrote: > http://bugs.openjdk.java.net/browse/JDK-8307103 Should be based on JDK-8307103 now. Thanks for all your testing! - PR Comment: https://git.openjdk.org/jdk/pull/10907#issuecomment-1527972396
Re: RFR: 8306851: Move Method access flags [v3]
On Fri, 28 Apr 2023 18:29:50 GMT, Frederic Parain wrote:
>> Coleen Phillimore has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Remove bool argument from ConstMethodFlags.set function.
>
> src/hotspot/share/oops/constMethod.cpp line 438:
>
>> 436: }
>> 437: st->cr();
>> 438: st->print(" - flags: "); _flags.print_on(st);
>> st->cr();
>
> Method prints its flags as an int and in decoded form, but ConstMethod only
> prints the decoded form. Any particular reason for this difference?
No reason for this difference. The only reason I added the int form for
MethodFlags was because AccessFlags were printed also with the int form.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13654#discussion_r1180712196
Re: RFR: 8306851: Move Method access flags [v3]
On Thu, 27 Apr 2023 14:21:23 GMT, Coleen Phillimore wrote:
>> This change moves the flags from AccessFlags to either ConstMethodFlags or
>> MethodFlags, depending on whether they are set at class file parse time,
>> which makes them essentially const, or at runtime, which makes them needing
>> atomic access.
>>
>> This leaves AccessFlags int size because Klass still has JVM flags that are
>> more work to move, but this change doesn't increase Method size. I didn't
>> remove JVM_RECOGNIZED_METHOD_MODIFIERS with this change since there are
>> several of these in other places, and with this change the code is benign.
>>
>> Tested with tier1-6, and some manual verification of printing.
>
> Coleen Phillimore has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Remove bool argument from ConstMethodFlags.set function.
Thank you for all those cleanings!
Looks good to me.
src/hotspot/share/oops/constMethod.cpp line 438:
> 436: }
> 437: st->cr();
> 438: st->print(" - flags: "); _flags.print_on(st);
> st->cr();
Method prints its flags as an int and in decoded form, but ConstMethod only
prints the decoded form. Any particular reason for this difference?
-
Marked as reviewed by fparain (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/13654#pullrequestreview-1406421552
PR Review Comment: https://git.openjdk.org/jdk/pull/13654#discussion_r1180705106
Re: RFR: 8306471: Add virtual threads support to JDWP ThreadReference.Stop and JDI ThreadReference.stop() [v2]
> Note this PR depends on the #13546 PR for the following: > > [JDK-8306434](https://bugs.openjdk.org/browse/JDK-8306434): add support of > virtual threads to JVMTI StopThread > > So it can't be finalized and push until after JDK-8306434 is pushed. There > will also be GHA failures until then. If JDK-8306434 results in any > additional spec changes, they will likely impact this CR also. > > > [JDK-8306034](https://bugs.openjdk.org/browse/JDK-8306034) is adding some > virtual thread support to JVMTI StopThread. This will allow JDWP > ThreadReference.Stop and JDI ThreadReference.stop() to have the same level > support for virtual threads. > > Mostly this is a spec update for JDWP and JDI, but there are also some minor > changes needed to the ThreadReference.stop() handling of JDWP errors, and > throwing the appropriate exceptions. Also some minor cleanup in jdb. The > debug agent doesn't need changes since JVMTI errors are just passed through > as the corresponding JDWP errors, and the code for this is already in place. > These errors are not new nor need any special handling. > > Our existing testing for ThreadReference.stop() is fairly weak: > > - nsk/jdb/kill/kill001, which tests stop() when the thread is suspended at a > breakpoint. It will get problem listed by > [JDK-8306034](https://bugs.openjdk.org/browse/JDK-8306034). I have fixes for > it already working and will push it separately. > - nsk/jdi/stop/stop001, which is problem listed and only tests when the > thread is blocked in Object.wait() > - nsk/jdi/stop/stop002, which only tests that throwing an invalid exception > fails properly > > I decided to take stop002 and make it a more thorough test of > ThreadReference.stop(). See the comment at the top for a list of what is > tested. As for reviewing this test, it's probably best to look at it as a > completely new test rather than looking at diffs since so much has been > changed and added. Chris Plummer has updated the pull request incrementally with one additional commit since the last revision: Fix test when suspended in a loop. Cleanup sme comments. - Changes: - all: https://git.openjdk.org/jdk/pull/13548/files - new: https://git.openjdk.org/jdk/pull/13548/files/fcf29b1e..f1ed2c6f Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13548=01 - incr: https://webrevs.openjdk.org/?repo=jdk=13548=00-01 Stats: 18 lines in 1 file changed: 2 ins; 9 del; 7 mod Patch: https://git.openjdk.org/jdk/pull/13548.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13548/head:pull/13548 PR: https://git.openjdk.org/jdk/pull/13548
Re: RFR: 8282384: [LOOM] Need test for ThreadReference.interrupt() on a vthread [v4]
> Convert this ThreadReference.interrupt() test to support virtual threads. I > believe this is the only test for ThreadReference.interrupt() that we have. > > Tested by running with and without -Dmain.wrapper=Virtual on all supported > platforms. Chris Plummer has updated the pull request incrementally with one additional commit since the last revision: update comment - Changes: - all: https://git.openjdk.org/jdk/pull/13696/files - new: https://git.openjdk.org/jdk/pull/13696/files/a7f277a9..d2ee0c31 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13696=03 - incr: https://webrevs.openjdk.org/?repo=jdk=13696=02-03 Stats: 3 lines in 1 file changed: 0 ins; 2 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/13696.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13696/head:pull/13696 PR: https://git.openjdk.org/jdk/pull/13696
Re: RFR: 8291555: Implement alternative fast-locking scheme [v66]
On Fri, 28 Apr 2023 11:32:54 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
Re: RFR: 8306929: Avoid CleanClassLoaderDataMetaspaces safepoints when previous versions are shared
On Fri, 28 Apr 2023 12:48:44 GMT, Stefan Johansson wrote: > Hi all, > > Please review this change to avoid CleanClassLoaderDataMetaspaces safepoint > when there is nothing that can be cleaned up. > > **Summary** > When transforming/redefining classes a previous version list is linked > together in the InstanceKlass. The original class is added to this list if it > is still used or shared. The difference between shared and used is not > currently noted. This leads to a problem when doing concurrent class > unloading, because during that we postpone some potential work to a safepoint > (since we are not in one). This is the CleanClassLoaderDataMetaspaces and it > is triggered by the ServiceThread if there is work to be done, for example if > InstanceKlass::_has_previous_versions is true. > > Since we currently does not differentiate between shared and "in use" we > always set _has_previous_versions if anything is on this list. This together > with the fact that shared previous versions should never be cleaned out leads > to this safepoint being triggered after every concurrent class unloading even > though there is nothing that can be cleaned out. > > This can be avoided by making sure the _previous_versions list is only > cleaned when there are non-shared classes on it. This change renames > `_has_previous_versions` to `_clean_previous_versions` and only updates it if > we have non-shared classes on the list. > > **Testing** > * A lot of manual testing verifying that we do get the safepoint when we > should. > * Added new test to verify expected behavior by parsing the logs. The test > uses JFR to trigger redefinition of some shared classes (when -Xshare:on). > * Mach5 run of new test and tier 1-3 This looks good. Thanks for all the testing and adding the new test. - Marked as reviewed by coleenp (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/13716#pullrequestreview-1406222927
Re: JEP draft: Disallow the Dynamic Loading of Agents by Default
Hello,Want to mention another point. In the past there has been a tendency of JRE to hide internal implementations. That’s somewhat understandable, however it was always a convinience to be able to poke at internals. I mean it took so long to offer a API for base64, or being able to actually create self signed certificates or parse CRLs, or to extend JSSE. With the stronger encapsulation this will now become a no go, making it more important for more official APIs - for example how to access the Top Level Domain List - I could probably give more examples. The JEP could acknowledge this.GrussBernd-- http://bernd.eckenfels.net
Von: serviceability-dev im Auftrag von Volker Simonis Gesendet: Freitag, April 28, 2023 5:39 PMAn: Ron Pressler Cc: jigsaw-...@openjdk.org ; serviceability-dev@openjdk.org Betreff: Re: JEP draft: Disallow the Dynamic Loading of Agents by Default On Wed, Apr 19, 2023 at 12:29 AM Ron Pressler wrote:
>
> Hi.
>
> Following last month’s email about changing the default of
> EnableDynamicAgentLoading [1], we’ve now published two JEP drafts.
>
> The first is an informational JEP describing what integrity is and why we need it,
> and motivates what changes are required to get it (which include the
> restriction of dynamically loaded agents among others):
>
> https://openjdk.org/jeps/8305968 Integrity and Strong Encapsulation
>
>- Use sun.misc.Unsafe to access and modify private fields.
>
>- Load a native library that employs JNI to call private methods and
> set private fields. (The JNI API is not subject to access checks.)
>
>- Load an agent that changes code in a running application,
> using an API intended for tools only.
>
> To attain our goal of integrity by default, we will gradually restrict
> these APIs and close all loopholes in a series of upcoming JEPs, ensuring
> that no library can assume superpowers without the application's consent.
> Libraries that rely on these APIs should spend the time remaining until
> they are restricted to prepare their users for any necessary changes.
I think it is a little unfortunate to put the usage of s.m.Unsafe and
JNI/Instrumentation/JVMTI into the same category, especially when it
comes to blaming developers for their usage. While s.m.Unsafe has
always been an internal, undocumented and unsupported API, the latter
three are part of the Java Platform (e.g. "native" is a Java keyword
and Runtime.loadLibrary() is part of the Java API).
Do you really plan to make JNI an optional feature which will have to
be manually enabled at startup? What will be the benefit? I understand
that in an ideal world where you had no user-supplied JNI libraries at
all, you might be able to perform more/better optimizations. But as
you'd have to support JNI anyway, wouldn't the maintenance of the
resulting code become a nightmare. How many "if (JNI) {..} else {..}"
would we get? And what would be the benefit of disabling it by default
for the user except increased "integrity"? I.e. do you have some
concrete examples of planned features X, Y, Z which will only work
with disabled JNI? Will these features be Java SE features or
implementation specific OpenJDK-only features?
> The second touches on the specifics of dynamically loaded agents and the
> proposed change:
>
> https://openjdk.org/jeps/8306275 Disallow the Dynamic Loading of Agents by Default
>
>Agents are used by profiling tools to instrument Java applications,
>but agents can also be misused to undermine the integrity of the
>Java Platform.
I don't think it is fair to assume that profilers are the only "valid"
use case for agents and imply that all other use cases are a mis-use
of the API.
>- It is not a goal to change the Attach API that allows a tool to
> connect to a running JVM for monitoring and management purposes.
I don't understand this "Non-Goal"? The Attach API [1] allows to
dynamically attach to a running JVM and "Once a reference to a virtual
machine is obtained, the loadAgent, loadAgentLibrary, and
loadAgentPath methods are used to load agents into target virtual
machine". So how can you achieve this JEP's goals without
changing/restricting the Attach API? I therefore think this "Non-Goal"
should be rephrased to explain which parts of the Attach API will be
changed and moved to the "Goal" section instead.
General comments:
- You go into great detail to explain why a human-operated tool is
"superior" (in the sense of trust and security) to a library and
"would ideally not be subject to the integrity constraints imposed on
the application". I can't follow this argument, because both, the
decision to use a specific tool as well as the decision to rely on a
library is taken by a human. I'd even argue that the decision to
depend on a specific library which requires the dynmaic attach
mechanism is taken by a more knowledgeable user (i.e. the developer
himself). Of course both, a tool as well as a library
Re: RFR: 8306851: Move Method access flags [v3]
On Fri, 28 Apr 2023 14:18:11 GMT, Matias Saavedra Silva
wrote:
>> Coleen Phillimore has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Remove bool argument from ConstMethodFlags.set function.
>
> src/hotspot/share/oops/method.hpp line 615:
>
>> 613: // has not been computed yet.
>> 614: bool guaranteed_monitor_matching() const { return
>> monitor_matching(); }
>> 615: void set_guaranteed_monitor_matching() {
>> set_monitor_matching(); }
>
> Is this method just obsolete now? If so it might be worth replacing the
> callers with `set_monitor_matching()` unless `set_monitor_matching()` is
> still meant to be private.
The reason I left that was to anchor the comment. There is nowhere good to put
that in the X macro. Also, didn't want to fix the callers. It's a good point
about making monitor_matching() private, but also not really doable with the X
macro. So that's why I left it.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13654#discussion_r1180548707
Re: RFR: 8306851: Move Method access flags [v4]
> This change moves the flags from AccessFlags to either ConstMethodFlags or > MethodFlags, depending on whether they are set at class file parse time, > which makes them essentially const, or at runtime, which makes them needing > atomic access. > > This leaves AccessFlags int size because Klass still has JVM flags that are > more work to move, but this change doesn't increase Method size. I didn't > remove JVM_RECOGNIZED_METHOD_MODIFIERS with this change since there are > several of these in other places, and with this change the code is benign. > > Tested with tier1-6, and some manual verification of printing. Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision: Updates - Changes: - all: https://git.openjdk.org/jdk/pull/13654/files - new: https://git.openjdk.org/jdk/pull/13654/files/6687cc0e..9b05ab84 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13654=03 - incr: https://webrevs.openjdk.org/?repo=jdk=13654=02-03 Stats: 9 lines in 1 file changed: 5 ins; 0 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/13654.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13654/head:pull/13654 PR: https://git.openjdk.org/jdk/pull/13654
Re: JEP draft: Disallow the Dynamic Loading of Agents by Default
On Wed, Apr 19, 2023 at 12:29 AM Ron Pressler wrote:
>
> Hi.
>
> Following last month’s email about changing the default of
> EnableDynamicAgentLoading [1], we’ve now published two JEP drafts.
>
> The first is an informational JEP describing what integrity is and why we
> need it,
> and motivates what changes are required to get it (which include the
> restriction of dynamically loaded agents among others):
>
> https://openjdk.org/jeps/8305968 Integrity and Strong Encapsulation
>
>- Use sun.misc.Unsafe to access and modify private fields.
>
>- Load a native library that employs JNI to call private methods and
> set private fields. (The JNI API is not subject to access checks.)
>
>- Load an agent that changes code in a running application,
> using an API intended for tools only.
>
> To attain our goal of integrity by default, we will gradually restrict
> these APIs and close all loopholes in a series of upcoming JEPs, ensuring
> that no library can assume superpowers without the application's consent.
> Libraries that rely on these APIs should spend the time remaining until
> they are restricted to prepare their users for any necessary changes.
I think it is a little unfortunate to put the usage of s.m.Unsafe and
JNI/Instrumentation/JVMTI into the same category, especially when it
comes to blaming developers for their usage. While s.m.Unsafe has
always been an internal, undocumented and unsupported API, the latter
three are part of the Java Platform (e.g. "native" is a Java keyword
and Runtime.loadLibrary() is part of the Java API).
Do you really plan to make JNI an optional feature which will have to
be manually enabled at startup? What will be the benefit? I understand
that in an ideal world where you had no user-supplied JNI libraries at
all, you might be able to perform more/better optimizations. But as
you'd have to support JNI anyway, wouldn't the maintenance of the
resulting code become a nightmare. How many "if (JNI) {..} else {..}"
would we get? And what would be the benefit of disabling it by default
for the user except increased "integrity"? I.e. do you have some
concrete examples of planned features X, Y, Z which will only work
with disabled JNI? Will these features be Java SE features or
implementation specific OpenJDK-only features?
> The second touches on the specifics of dynamically loaded agents and the
> proposed change:
>
> https://openjdk.org/jeps/8306275 Disallow the Dynamic Loading of Agents by
> Default
>
>Agents are used by profiling tools to instrument Java applications,
>but agents can also be misused to undermine the integrity of the
>Java Platform.
I don't think it is fair to assume that profilers are the only "valid"
use case for agents and imply that all other use cases are a mis-use
of the API.
>- It is not a goal to change the Attach API that allows a tool to
> connect to a running JVM for monitoring and management purposes.
I don't understand this "Non-Goal"? The Attach API [1] allows to
dynamically attach to a running JVM and "Once a reference to a virtual
machine is obtained, the loadAgent, loadAgentLibrary, and
loadAgentPath methods are used to load agents into target virtual
machine". So how can you achieve this JEP's goals without
changing/restricting the Attach API? I therefore think this "Non-Goal"
should be rephrased to explain which parts of the Attach API will be
changed and moved to the "Goal" section instead.
General comments:
- You go into great detail to explain why a human-operated tool is
"superior" (in the sense of trust and security) to a library and
"would ideally not be subject to the integrity constraints imposed on
the application". I can't follow this argument, because both, the
decision to use a specific tool as well as the decision to rely on a
library is taken by a human. I'd even argue that the decision to
depend on a specific library which requires the dynmaic attach
mechanism is taken by a more knowledgeable user (i.e. the developer
himself). Of course both, a tool as well as a library can contain
malicious code, but I don't see a fundamental difference between the
two.
- You may argue that users have to be protected from malicious
libraries which gain their superpowers by secretly loading agents at
runtime. But users who don't know and don't care about their library
dependencies will just as easy and without reflection (pun intended :)
add the -XX:+EnableDynamicAgentLoading to their command line arguments
(making this the new, most often used command line option even
surpassing the usage of --add-opens :)
- I still can't understand the benefit of "only" changing the default
behavior for dynamic agent loading. I could understand this if you'd
do it with a plan to deprecate and completely remove the dynamic agent
loading capability. But what are the benefits of changing the default
if you'll have to support the functionality anyway? As mentioned in
earlier discussions, my main
Re: RFR: 8306851: Move Method access flags [v3]
On Thu, 27 Apr 2023 14:21:23 GMT, Coleen Phillimore wrote:
>> This change moves the flags from AccessFlags to either ConstMethodFlags or
>> MethodFlags, depending on whether they are set at class file parse time,
>> which makes them essentially const, or at runtime, which makes them needing
>> atomic access.
>>
>> This leaves AccessFlags int size because Klass still has JVM flags that are
>> more work to move, but this change doesn't increase Method size. I didn't
>> remove JVM_RECOGNIZED_METHOD_MODIFIERS with this change since there are
>> several of these in other places, and with this change the code is benign.
>>
>> Tested with tier1-6, and some manual verification of printing.
>
> Coleen Phillimore has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Remove bool argument from ConstMethodFlags.set function.
Nice change! Just some small nits but it otherwise looks good.
src/hotspot/share/oops/method.hpp line 606:
> 604:
> 605: bool compute_has_loops_flag();
> 606: bool set_has_loops() { set_has_loops_flag();
> set_has_loops_flag_init(); return true; }
Since this has multiple statements it should probably be on different lines
src/hotspot/share/oops/method.hpp line 615:
> 613: // has not been computed yet.
> 614: bool guaranteed_monitor_matching() const { return
> monitor_matching(); }
> 615: void set_guaranteed_monitor_matching() {
> set_monitor_matching(); }
Is this method just obsolete now? If so it might be worth replacing the callers
with `set_monitor_matching()` unless `set_monitor_matching()` is still meant to
be private.
-
Changes requested by matsaave (Committer).
PR Review: https://git.openjdk.org/jdk/pull/13654#pullrequestreview-1406036462
PR Review Comment: https://git.openjdk.org/jdk/pull/13654#discussion_r1180462644
PR Review Comment: https://git.openjdk.org/jdk/pull/13654#discussion_r1180472698
Re: RFR: JDK-8306441: Segmented heap dump [v3]
> Hi, heap dump brings about pauses for application's execution(STW), this is a > well-known pain. JDK-8252842 have added parallel support to heapdump in an > attempt to alleviate this issue. However, all concurrent threads > competitively write heap data to the same file, and more memory is required > to maintain the concurrent buffer queue. In experiments, we did not feel a > significant performance improvement from that. > > The minor-pause solution, which is presented in this PR, is a two-stage > segmented heap dump: > > 1. Stage One(STW): Concurrent threads directly write data to multiple heap > files. > 2. Stage Two(Non-STW): Merge multiple heap files into one complete heap dump > file. > > Now concurrent worker threads are not required to maintain a buffer queue, > which would result in more memory overhead, nor do they need to compete for > locks. It significantly reduces 73~80% application pause time. > > | memory | numOfThread | STW | Total | > | --- | - | -- | | > | 8g | 1 thread | 15.612 secs | 15.612 secs | > | 8g | 32 thread | 2.5617250 secs | 14.498 secs | > | 8g | 96 thread | 2.6790452 secs | 14.012 secs | > | 16g | 1 thread | 26.278 secs | 26.278 secs | > | 16g | 32 thread | 5.2313740 secs | 26.417 secs | > | 16g | 96 thread | 6.2445556 secs | 27.141 secs | > | 32g | 1 thread | 48.149 secs | 48.149 secs | > | 32g | 32 thread | 10.7734677 secs | 61.643 secs | > | 32g | 96 thread | 13.1522042 secs | 61.432 secs | > | 64g | 1 thread | 100.583 secs | 100.583 secs | > | 64g | 32 thread | 20.9233744 secs | 134.701 secs | > | 64g | 96 thread | 26.7374116 secs | 126.080 secs | > | 128g | 1 thread | 233.843 secs | 233.843 secs | > | 128g | 32 thread | 72.9945768 secs | 207.060 secs | > | 128g | 96 thread | 67.6815929 secs | 336.345 secs | > >> **Total** means the total heap dump including both two phases >> **STW** means the first phase only. >> For parallel dump, **Total** = **STW** + **Merge**. For serial dump, >> **Total** = **STW** > >  > > In actual testing, two-stage solution can lead to an increase in the overall > time for heapdump(See table above). However, considering the reduction of STW > time, I think it is an acceptable trade-off. Furthermore, there is still room > for optimization in the second merge stage(e.g. > sendfile/splice/copy_file_range instead of read+write combination). Since > number of parallel dump thread has a considerable impact on total dump time, > I added a parameter that allows users to specify the number of parallel dump > thread they wish to run. > > # Open discussion > > - Pauseless heap dump solution? > An alternative pauseless solution is to fork a child process, set the parent > process heap to read-only, and dump the heap in child process. Once writing > happens in parent process, child process observes them by userfaultfd and > corresponding pages are prioritized for dumping. I'm also looking forward to > hearing comments and discussions about this solution. > > - Client parser support for segmented heap dump > This patch provides a possibility that whether heap dump needs to be complete > or not, can the VM directly generate segmented heapdump, and let the client > parser complete the merge process? Looking forward to hearing comments from > the Eclipse MAT community Yi Yang has updated the pull request incrementally with one additional commit since the last revision: refactor VM_HeapDumpMerge - Changes: - all: https://git.openjdk.org/jdk/pull/13667/files - new: https://git.openjdk.org/jdk/pull/13667/files/00b49e4e..9e563ca7 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13667=02 - incr: https://webrevs.openjdk.org/?repo=jdk=13667=01-02 Stats: 83 lines in 1 file changed: 43 ins; 35 del; 5 mod Patch: https://git.openjdk.org/jdk/pull/13667.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13667/head:pull/13667 PR: https://git.openjdk.org/jdk/pull/13667
RFR: 8306929: Avoid CleanClassLoaderDataMetaspaces safepoints when previous versions are shared
Hi all, Please review this change to avoid CleanClassLoaderDataMetaspaces safepoint when there is nothing that can be cleaned up. **Summary** When transforming/redefining classes a previous version list is linked together in the InstanceKlass. The original class is added to this list if it is still used or shared. The difference between shared and used is not currently noted. This leads to a problem when doing concurrent class unloading, because during that we postpone some potential work to a safepoint (since we are not in one). This is the CleanClassLoaderDataMetaspaces and it is triggered by the ServiceThread if there is work to be done, for example if InstanceKlass::_has_previous_versions is true. Since we currently does not differentiate between shared and "in use" we always set _has_previous_versions if anything is on this list. This together with the fact that shared previous versions should never be cleaned out leads to this safepoint being triggered after every concurrent class unloading even though there is nothing that can be cleaned out. This can be avoided by making sure the _previous_versions list is only cleaned when there are non-shared classes on it. This change renames `_has_previous_versions` to `_clean_previous_versions` and only updates it if we have non-shared classes on the list. **Testing** * A lot of manual testing verifying that we do get the safepoint when we should. * Added new test to verify expected behavior by parsing the logs. The test uses JFR to trigger redefinition of some shared classes (when -Xshare:on). * Mach5 run of new test and tier 1-3 - Commit messages: - Add test wih JFR - 8306929: Avoid CleanClassLoaderDataMetaspaces safepoints when previous versions are shared Changes: https://git.openjdk.org/jdk/pull/13716/files Webrev: https://webrevs.openjdk.org/?repo=jdk=13716=00 Issue: https://bugs.openjdk.org/browse/JDK-8306929 Stats: 139 lines in 6 files changed: 116 ins; 3 del; 20 mod Patch: https://git.openjdk.org/jdk/pull/13716.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13716/head:pull/13716 PR: https://git.openjdk.org/jdk/pull/13716
Re: RFR: 8291555: Implement alternative fast-locking scheme [v66]
> 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 p rotocols. > > 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 participate in monitor deflation protocol. > This would be implemented in a separate PR > > Also, and I might be mistaken here, this new
Re: RFR: 8291555: Implement alternative fast-locking scheme [v65]
On Fri, 28 Apr 2023 07:43:24 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
Re: RFR: 8291555: Implement alternative fast-locking scheme [v60]
On Wed, 26 Apr 2023 18:58:13 GMT, Roman Kennke wrote: >>> @rkennke - I'm planning to do another crawl thru review next week. >> >> Thanks! That is greatly appeciated! > >> @rkennke - finished my second crawl thru review of 60/68 files changed. I >> only skipped the RISC-V files since I know nada about that platform... >> >> My Mach5 testing of v61 is running Tier7 and I hope to start Tier8 later >> tonight. So far all testing looks good, but I'll include the usual summary >> comment in the bug report... > > Thanks so much for reviewing this large PR (so many times)! I believe I have > incorporated all your suggestions (or left a comment/question when it wasn't > clear). > > Cheers, > Roman @rkennke Last ARM32 fixes: https://gist.github.com/tstuefe/8a0fd30618f1d0e085b5ca12d7c156cd I removed the superfluous flush from sharedRuntime. For a test, I applied https://github.com/openjdk/jdk/pull/13596 patch and built and tested arm (starting fastlockbench with interpreted, c1, c2), all seems to be well. - PR Comment: https://git.openjdk.org/jdk/pull/10907#issuecomment-1527233512
Re: RFR: 8291555: Implement alternative fast-locking scheme [v62]
On Wed, 26 Apr 2023 18:35:45 GMT, Thomas Stuefe wrote: >> src/hotspot/cpu/arm/sharedRuntime_arm.cpp line 649: >> >>> 647: >>> 648: __ flush(); >>> 649: return AdapterHandlerLibrary::new_entry(fingerprint, i2c_entry, >>> c2i_entry, c2i_unverified_entry); >> >> This change seems out of place... what's the story here? > > This is a local revert of *8303154: Investigate and improve instruction cache > flushing during compilation* - the missing flush caused random crashes, but I > did not have time to investigate. I reverted the flush, crashes were gone. > > If needed I may revisit this when there is time. I'll remove it. Tests have to wait on arm for https://bugs.openjdk.org/browse/JDK-8305387 though. - PR Review Comment: https://git.openjdk.org/jdk/pull/10907#discussion_r1180108118
Re: RFR: 8291555: Implement alternative fast-locking scheme [v65]
On Fri, 28 Apr 2023 07:43:24 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
Re: RFR: 8291555: Implement alternative fast-locking scheme [v65]
> 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 p rotocols. > > 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 participate in monitor deflation protocol. > This would be implemented in a separate PR > > Also, and I might be mistaken here, this new
Re: RFR: 8306851: Move Method access flags [v3]
On Thu, 27 Apr 2023 14:21:23 GMT, Coleen Phillimore wrote: >> This change moves the flags from AccessFlags to either ConstMethodFlags or >> MethodFlags, depending on whether they are set at class file parse time, >> which makes them essentially const, or at runtime, which makes them needing >> atomic access. >> >> This leaves AccessFlags int size because Klass still has JVM flags that are >> more work to move, but this change doesn't increase Method size. I didn't >> remove JVM_RECOGNIZED_METHOD_MODIFIERS with this change since there are >> several of these in other places, and with this change the code is benign. >> >> Tested with tier1-6, and some manual verification of printing. > > Coleen Phillimore has updated the pull request incrementally with one > additional commit since the last revision: > > Remove bool argument from ConstMethodFlags.set function. Marked as reviewed by dnsimon (Committer). Thankfully all these changes only impact values read by JVMCI Java code and none in [Graal Java code](https://github.com/oracle/graal/blob/114067fc41d97e6c07f6de9bd745196d6f967ae4/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java#L47). Looks good to me. - PR Review: https://git.openjdk.org/jdk/pull/13654#pullrequestreview-1405368377 PR Comment: https://git.openjdk.org/jdk/pull/13654#issuecomment-1527109990
Re: RFR: 8282384: [LOOM] Need test for ThreadReference.interrupt() on a vthread [v3]
On Fri, 28 Apr 2023 04:56:17 GMT, Serguei Spitsyn wrote:
>> Chris Plummer has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> use NamedTask library class
>
> test/hotspot/jtreg/vmTestbase/nsk/jdi/ThreadReference/interrupt/interrupt001.java
> line 214:
>
>> 212: }
>> 213:
>> 214: log2("..thread2 is " + (thread2.isVirtual() ? "" : "not ")
>> + "a virtual thread");
>
> Nit: Would it better to print "is a virtual thread" instead of "is not a
> virtual thread"?
I don't see your point. It prints one or the other depending on whether or not
the thread is virtual.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13696#discussion_r1179956252
Re: RFR: 8306851: Move Method access flags [v3]
On Thu, 27 Apr 2023 14:21:23 GMT, Coleen Phillimore wrote:
>> This change moves the flags from AccessFlags to either ConstMethodFlags or
>> MethodFlags, depending on whether they are set at class file parse time,
>> which makes them essentially const, or at runtime, which makes them needing
>> atomic access.
>>
>> This leaves AccessFlags int size because Klass still has JVM flags that are
>> more work to move, but this change doesn't increase Method size. I didn't
>> remove JVM_RECOGNIZED_METHOD_MODIFIERS with this change since there are
>> several of these in other places, and with this change the code is benign.
>>
>> Tested with tier1-6, and some manual verification of printing.
>
> Coleen Phillimore has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Remove bool argument from ConstMethodFlags.set function.
Thanks for the updates. I understand about the fanout from making `is` naming
fully consistent.
src/hotspot/share/oops/method.hpp line 875:
> 873: boolis_not_c1_osr_compilable() const{ return
> is_not_c1_compilable(); }
> 874: void set_is_not_c1_osr_compilable() {
> set_is_not_c1_compilable(); }
> 875: void clear_is_not_c1_osr_compilable() {
> clear_is_not_c1_compilable(); }
Nit: don't need extra spaces after `{`
-
Marked as reviewed by dholmes (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/13654#pullrequestreview-1405267857
PR Review Comment: https://git.openjdk.org/jdk/pull/13654#discussion_r1179956725
