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
Re: RFR: 8282384: [LOOM] Need test for ThreadReference.interrupt() on a vthread [v3]
On Thu, 27 Apr 2023 21:08:23 GMT, Chris Plummer wrote:
>> 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:
>
> use NamedTask library class
Looks good.
Thanks,
Serguei
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"?
-
Marked as reviewed by sspitsyn (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/13696#pullrequestreview-1405223335
PR Review Comment: https://git.openjdk.org/jdk/pull/13696#discussion_r1179927700
Integrated: 8305566: Change StringDedup thread to derive from JavaThread
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. This pull request has now been integrated. Changeset: d3abfec8 Author:Kim Barrett URL: https://git.openjdk.org/jdk/commit/d3abfec8b7ce901150952356f9f1109d09a8cb2a Stats: 440 lines in 18 files changed: 193 ins; 146 del; 101 mod 8305566: Change StringDedup thread to derive from JavaThread Reviewed-by: stefank, cjplummer, tschatzl - PR: https://git.openjdk.org/jdk/pull/13607
Re: RFR: 8305566: Change StringDedup thread to derive from JavaThread [v3]
> 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 with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 12 additional commits since the last revision: - Merge branch 'master' into jt-strdedup - fix include order - fix stray tab - move is_active_Java_thread - copyrights - servicabilty support - use JavaThread - separate thread class - simplify init - do not pass around STS joiner - ... and 2 more: https://git.openjdk.org/jdk/compare/8957f67e...da07a420 - Changes: - all: https://git.openjdk.org/jdk/pull/13607/files - new: https://git.openjdk.org/jdk/pull/13607/files/f17cc6be..da07a420 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13607&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13607&range=01-02 Stats: 46789 lines in 794 files changed: 30868 ins; 11396 del; 4525 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: Change StringDedup thread to derive from JavaThread [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 Thanks all for reviews. I've changed the bug and PR summaries as requested. - PR Comment: https://git.openjdk.org/jdk/pull/13607#issuecomment-1526905175
Re: RFR: 8306034: add support of virtual threads to JVMTI StopThread [v6]
On Fri, 28 Apr 2023 00:46:23 GMT, Serguei Spitsyn wrote: >> Ok. How about "the function cannot be performed on the thread's current >> frame". We already have a couple references to "the function" in the error >> codes section. > > We have two suggestions: >> - "or a function on a thread cannot be performed at the thread's current >> frame". >> - "the function cannot be performed on the thread's current frame." > > So, we need to pick one. The second one looks simpler to me but > I'm not completely sure that it reflects the full meaning correctly. > I wonder about a mix of the two suggestions above: > >> "the function cannot be performed at the thread's current frame." We need to account for the `SetLocalXXX` functions with the `depth` parameter which also return `OPAQUE_FRAME` error code for virtual frames. My concern is if the "current frame" part is fully correct. - PR Review Comment: https://git.openjdk.org/jdk/pull/13546#discussion_r1179834952
Re: RFR: 8306034: add support of virtual threads to JVMTI StopThread [v6]
On Thu, 27 Apr 2023 19:14:56 GMT, Chris Plummer wrote: >> I think the first part is okay because it's for functions that are about >> frames. The NotifyFramePop specifies the depth so it may not be the current >> frame. The second usage is the functions on a thread where we might do >> better than "not suitable". > > Ok. How about "the function cannot be performed on the thread's current > frame". We already have a couple references to "the function" in the error > codes section. We have two suggestions: > - "or a function on a thread cannot be performed at the thread's current > frame". > - "the function cannot be performed on the thread's current frame." So, we need to pick one. The second one looks simpler to me but I'm not completely sure that it reflects the full meaning correctly. I wonder about a mix of the two suggestions above: > "the function cannot be performed at the thread's current frame." - PR Review Comment: https://git.openjdk.org/jdk/pull/13546#discussion_r1179833278
Re: RFR: 8291555: Implement alternative fast-locking scheme [v64]
On Thu, 27 Apr 2023 07:52:23 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: 8282384: [LOOM] Need test for ThreadReference.interrupt() on a vthread [v3]
On Thu, 27 Apr 2023 21:08:23 GMT, Chris Plummer wrote: >> 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: > > use NamedTask library class Marked as reviewed by lmesnik (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/13696#pullrequestreview-1404929979
Re: RFR: 8282384: [LOOM] Need test for ThreadReference.interrupt() on a vthread [v3]
> 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: use NamedTask library class - Changes: - all: https://git.openjdk.org/jdk/pull/13696/files - new: https://git.openjdk.org/jdk/pull/13696/files/e417fee0..a7f277a9 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13696&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13696&range=01-02 Stats: 5 lines in 1 file changed: 0 ins; 2 del; 3 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: 8282384: [LOOM] Need test for ThreadReference.interrupt() on a vthread [v2]
On Thu, 27 Apr 2023 19:43:51 GMT, Leonid Mesnik wrote:
>> Chris Plummer has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> some thread -> task renaming
>
> test/hotspot/jtreg/vmTestbase/nsk/jdi/ThreadReference/interrupt/interrupt001a.java
> line 209:
>
>> 207: }
>> 208:
>> 209: class interrupt001aThread implements Runnable {
>
> Does it make sense to rename class to something like Interrupt001aTask? Just
> to let know that it is not a thread. Also, there is
> ./nsk/share/jdi/NamedTask.java for task with names in JDI tests.
Ok. I've made both these changes.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13696#discussion_r1179660333
Re: RFR: 8282384: [LOOM] Need test for ThreadReference.interrupt() on a vthread [v2]
> 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: some thread -> task renaming - Changes: - all: https://git.openjdk.org/jdk/pull/13696/files - new: https://git.openjdk.org/jdk/pull/13696/files/9e7d4f3b..e417fee0 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13696&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13696&range=00-01 Stats: 7 lines in 1 file changed: 0 ins; 0 del; 7 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: 8282384: [LOOM] Need test for ThreadReference.interrupt() on a vthread
On Thu, 27 Apr 2023 17:42:41 GMT, Chris Plummer wrote:
> 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.
Changes requested by lmesnik (Reviewer).
test/hotspot/jtreg/vmTestbase/nsk/jdi/ThreadReference/interrupt/interrupt001a.java
line 209:
> 207: }
> 208:
> 209: class interrupt001aThread implements Runnable {
Does it make sense to rename class to something like Interrupt001aTask? Just to
let know that it is not a thread. Also, there is ./nsk/share/jdi/NamedTask.java
for task with names in JDI tests.
-
PR Review: https://git.openjdk.org/jdk/pull/13696#pullrequestreview-1404771083
PR Review Comment: https://git.openjdk.org/jdk/pull/13696#discussion_r1179608600
Re: RFR: 8306034: add support of virtual threads to JVMTI StopThread [v6]
On Thu, 27 Apr 2023 18:59:34 GMT, Alan Bateman wrote: >> The wording starts off with "Information about the frame...", and you >> haven't suggested to change that to "the current frame". We should be >> consistent. Can't we just change both "the frame" references to "the current >> frame", and leave the rest the same as what Serguei has in place here? > > I think the first part is okay because it's for functions that are about > frames. The NotifyFramePop specifies the depth so it may not be the current > frame. The second usage is the functions on a thread where we might do better > than "not suitable". Ok. How about "the function cannot be performed on the thread's current frame". We already have a couple references to "the function" in the error codes section. - PR Review Comment: https://git.openjdk.org/jdk/pull/13546#discussion_r1179584919
Re: RFR: 8306034: add support of virtual threads to JVMTI StopThread [v6]
On Thu, 27 Apr 2023 18:49:40 GMT, Chris Plummer wrote: >> src/hotspot/share/prims/jvmti.xml line 11984: >> >>> 11982: >>> 11983: Information about the frame is not available (e.g. for native >>> frames), >>> 11984: or the frame is not suitable for the requested operation. >> >> After re-reading the spec changes, I'm wondering if we can improve on "or >> the frame is not suitable for the requested operation". StopThread doesn't >> have a frame parameter. ForceEarlyReturn doesn't have a frame parameter >> either as it's implicit (the current frame). I wonder if wording something >> like this might be better: >> "or a function on a thread cannot be performed at the thread's current >> frame". > > The wording starts off with "Information about the frame...", and you haven't > suggested to change that to "the current frame". We should be consistent. > Can't we just change both "the frame" references to "the current frame", and > leave the rest the same as what Serguei has in place here? I think the first part is okay because it's for functions that are about frames. The NotifyFramePop specifies the depth so it may not be the current frame. The second usage is the functions on a thread where we might do better than "not suitable". - PR Review Comment: https://git.openjdk.org/jdk/pull/13546#discussion_r1179571945
Re: RFR: 8306034: add support of virtual threads to JVMTI StopThread [v6]
On Thu, 27 Apr 2023 10:58:14 GMT, Alan Bateman wrote: >> Serguei Spitsyn has updated the pull request incrementally with one >> additional commit since the last revision: >> >> install_async_exception: set interrupt status for platform threads only > > src/hotspot/share/prims/jvmti.xml line 11984: > >> 11982: >> 11983: Information about the frame is not available (e.g. for native >> frames), >> 11984: or the frame is not suitable for the requested operation. > > After re-reading the spec changes, I'm wondering if we can improve on "or the > frame is not suitable for the requested operation". StopThread doesn't have > a frame parameter. ForceEarlyReturn doesn't have a frame parameter either as > it's implicit (the current frame). I wonder if wording something like this > might be better: > "or a function on a thread cannot be performed at the thread's current frame". The wording starts off with "Information about the frame...", and you haven't suggested to change that to "the current frame". We should be consistent. Can't we just change both "the frame" references to "the current frame", and leave the rest the same as what Serguei has in place here? - PR Review Comment: https://git.openjdk.org/jdk/pull/13546#discussion_r1179561498
RFR: 8282384: [LOOM] Need test for ThreadReference.interrupt() on a vthread
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. - Commit messages: - Add vthread support to interrupt test Changes: https://git.openjdk.org/jdk/pull/13696/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=13696&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8282384 Stats: 18 lines in 2 files changed: 5 ins; 0 del; 13 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: 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. @dougxc can you look at the JVMCI changes? - PR Comment: https://git.openjdk.org/jdk/pull/13654#issuecomment-1525977245
Re: RFR: 8306851: Move Method access flags [v3]
> 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. - Changes: - all: https://git.openjdk.org/jdk/pull/13654/files - new: https://git.openjdk.org/jdk/pull/13654/files/fc5bcaa6..6687cc0e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13654&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13654&range=01-02 Stats: 9 lines in 2 files changed: 0 ins; 5 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: 8306836: Remove pinned tag for G1 heap regions [v3]
On Thu, 27 Apr 2023 10:38:17 GMT, Albert Mingkun Yang wrote:
> > I think you are right about using is_humongous() directly here: the reason
> > we skip compacting of humongous regions during the "main" compaction is
> > intentional here
>
> However, I am unable to discern the difference -- why `is_young_gc_movable`
> is semantically-correct in one place, but not in the other in this concrete
> example.
>
> ```
> bool G1CollectionSetChooser::should_add(HeapRegion* hr) {
> return !hr->is_young() &&
> G1CollectedHeap::heap()->policy()->is_young_gc_movable(hr) &&
> ```
>
`G1CollectionSetChooser::should_add` asks: can/should I add this region to the
collection set candidates to evacuate (reclaim via moving) this region during
young gc?
> vs
>
> ```
> void G1FullCollector::before_marking_update_attribute_table(HeapRegion* hr) {
> if (hr->is_free()) {
> _region_attr_table.set_free(hr->hrm_index());
> } else if (hr->is_humongous()) {
> ```
`G1FullCollector::before_marking_update_attribute_table` asks: can I
compact/move this region in the (small object) compaction phase later?
So they are asking the question for different types of gc, where in the second
case it is actually asking that question for a phase that is about compacting
regular object regions. So it seems somewhat obvious to exclude non-regular
object regions at the outset, or at least not use this predicate (which you
criticized as non-obvious why full gc uses a predicate with "young-gc" inside).
Then there is the matter of documentation: if one writes `!is_humongous()`
there, there is need for a comment like "we do not move humongous objects
during young gc" everywhere you need it, while the method name also acts as the
documentation, saying "exclude everything that we are not moving during young
gc".
>
> Looking at where `G1CollectionSetChooser::should_add` is called, can one use
> `hr->is_old()` instead of `!hr->is_young() &&
> G1CollectedHeap::heap()->policy()->is_young_gc_movable(hr)`? (In fact, I
> believe that inlining `should_add` to the caller would result in a smoother
> code flow and prevent some duplicate region-type queries.)
>
Combining the two into the single predicate may be correct from an execution
POV. However the two predicates filter for different reasons: The `!is_young`
filters out regions that are not allowed to be put in the collection set
candidates at all (it's a set of old regions that young gc may evacuate later
by definition), the second filters those that can't be reclaimed by
moving/evacuation.
Otherwise one would need to add comments, this way it is self-commenting (and
this isn't performance sensitive).
> In my opinion, introducing a new `is_young_gc_movable` API in this particular
> PR may not be entirely justified. It may make more sense to introduce it in
> later PRs where region-pinning is supported and the API is actually utilized.
`is_young_gc_movable` and pinning are separate concerns. `is_young_gc_movable`
is a static view on the region. Pinning is assumed to be very transient, and
assumed to not pin too much (generating lots of garbage in pinned regions
basically - everything but the potentially pinned objects are still evacuated
out).
So it is more than likely advantageous to put pinned regions into the
candidates for proactive evacuation.
Thanks,
Thomas
-
PR Comment: https://git.openjdk.org/jdk/pull/13643#issuecomment-1525668118
Re: RFR: 8306851: Move Method access flags [v2]
On Thu, 27 Apr 2023 12:09:21 GMT, Coleen Phillimore wrote:
>> src/hotspot/share/oops/constMethodFlags.hpp line 53:
>>
>>> 51:flag(has_type_annotations , 1 << 9) \
>>> 52:flag(has_default_annotations , 1 << 10) \
>>> 53:flag(caller_sensitive , 1 << 11) \
>>
>> Nit: we should consistently use either `x` or `is_x` for `x` in `overpass,
>> caller_sensitive, hidden, scoped, ...`
>
> That's my preference too, but I was trying to not change all the callers to
> is_caller_sensitive, for example, or providing a set of wrappers to change
> the "x" calls to "is_x" calls to the flags interface.
The 'is' in not_x_compilable can be added without too much fanout of lines and
files that I don't want to change, so I added the 'is' to these.
>> src/hotspot/share/oops/method.cpp line 735:
>>
>>> 733: case Bytecodes::_jsr:
>>> 734: if (bcs.dest() < bcs.next_bci()) {
>>> 735: return set_has_loops();
>>
>> I don't understand the new return logic here. The break gets us out of the
>> switch but we are still in the while loop, but the return takes us all the
>> way out. ???
>
> Yes, I had a version of this change where we only let has_loops get set once,
> and this code set it over and over again, which is unnecessary. Once it's
> true, it stays true.
The early break and not resetting has_loops once it's true, saves the atomic
access also.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13654#discussion_r1179084771
PR Review Comment: https://git.openjdk.org/jdk/pull/13654#discussion_r1179080249
Re: RFR: 8306851: Move Method access flags [v2]
On Thu, 27 Apr 2023 04:35:10 GMT, David Holmes wrote:
>> Coleen Phillimore has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Add is prefixes and some cleanups.
>
> src/hotspot/share/oops/constMethodFlags.hpp line 53:
>
>> 51:flag(has_type_annotations , 1 << 9) \
>> 52:flag(has_default_annotations , 1 << 10) \
>> 53:flag(caller_sensitive , 1 << 11) \
>
> Nit: we should consistently use either `x` or `is_x` for `x` in `overpass,
> caller_sensitive, hidden, scoped, ...`
That's my preference too, but I was trying to not change all the callers to
is_caller_sensitive, for example, or providing a set of wrappers to change the
"x" calls to "is_x" calls to the flags interface.
> src/hotspot/share/oops/method.hpp line 808:
>
>> 806:
>> 807: bool is_hidden() const { return constMethod()->is_hidden(); }
>> 808: void set_hidden() { constMethod()->set_is_hidden(); }
>
> The naming is inconsistent here regarding `is` - either both should have it
> or neither IMO.
This one is easy to fix because there aren't calls everywhere.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13654#discussion_r1179054655
PR Review Comment: https://git.openjdk.org/jdk/pull/13654#discussion_r1179057923
Re: RFR: 8306851: Move Method access flags [v2]
> 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: Add is prefixes and some cleanups. - Changes: - all: https://git.openjdk.org/jdk/pull/13654/files - new: https://git.openjdk.org/jdk/pull/13654/files/4b92aacf..fc5bcaa6 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13654&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13654&range=00-01 Stats: 37 lines in 8 files changed: 1 ins; 0 del; 36 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: 8306836: Remove pinned tag for G1 heap regions [v4]
> Hi all, > > please review this change that removes the pinned tag from `HeapRegion`. > > So that "pinned" tag for G1 heap regions indicates that the region should not > move during (young) gc. This applies to now removed archive regions and > humongous objects/regions. > > With "real" g1 region pinning to deal with gclocker in g1 once and for all > upcoming we need a refcount, a single bit is not sufficient anymore. Further > there will be a naming conflict as this kind of "pinning" is different to g1 > region pinning "pinning". The former indicates "contents can not be moved, > but can be reclaimed", while the latter means "contents can not be moved and > not reclaimed". > > The (current) pinned flag is surprisingly little used, only for policy > decisions. > > The suggestion this change implements is to remove the "pinned" tag as it is, > and reserve it for future g1 region pinning (that needs to store the pinning > attribute differently as a refcount anyway). > > Testing: tier1-3, gha > > Thanks, > Thomas Thomas Schatzl has updated the pull request incrementally with one additional commit since the last revision: remove is_young_gc_movable in full gc code - Changes: - all: https://git.openjdk.org/jdk/pull/13643/files - new: https://git.openjdk.org/jdk/pull/13643/files/eacf54ba..e136c7e4 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13643&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13643&range=02-03 Stats: 3 lines in 3 files changed: 0 ins; 1 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/13643.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13643/head:pull/13643 PR: https://git.openjdk.org/jdk/pull/13643
Re: RFR: 8306851: Move Method access flags
On Thu, 27 Apr 2023 04:28:31 GMT, David Holmes 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.
>
> src/hotspot/share/classfile/classFileParser.cpp line 2741:
>
>> 2739: parsed_annotations.apply_to(methodHandle(THREAD, m));
>> 2740:
>> 2741: if (is_hidden() && !m->is_hidden()) { // Mark methods in hidden
>> classes as 'hidden'.
>
> This seems odd - how would m already be marked hidden? And why do we care?
> The check-and-branch is more expensive than just setting the field.
This hidden flag is set by both the Method attribute during class file parsing
and set to true if the whole class is hidden. My original version of the
assert_is_safe() function only allowed a flag to be set once, but I've changed
it to only set to true. So I can undo this.
> src/hotspot/share/oops/method.cpp line 735:
>
>> 733: case Bytecodes::_jsr:
>> 734: if (bcs.dest() < bcs.next_bci()) {
>> 735: return set_has_loops();
>
> I don't understand the new return logic here. The break gets us out of the
> switch but we are still in the while loop, but the return takes us all the
> way out. ???
Yes, I had a version of this change where we only let has_loops get set once,
and this code set it over and over again, which is unnecessary. Once it's true,
it stays true.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13654#discussion_r1179049959
PR Review Comment: https://git.openjdk.org/jdk/pull/13654#discussion_r117908
Re: RFR: 8306034: add support of virtual threads to JVMTI StopThread [v6]
On Thu, 27 Apr 2023 09:14:29 GMT, Serguei Spitsyn wrote: >> 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: > > install_async_exception: set interrupt status for platform threads only src/hotspot/share/prims/jvmti.xml line 11984: > 11982: > 11983: Information about the frame is not available (e.g. for native > frames), > 11984: or the frame is not suitable for the requested operation. After re-reading the spec changes, I'm wondering if we can improve on "or the frame is not suitable for the requested operation". StopThread doesn't have a frame parameter. ForceEarlyReturn doesn't have a frame parameter either as it's implicit (the current frame). I wonder if wording something like this might be better: "or a function on a thread cannot be performed at the thread's current frame". - PR Review Comment: https://git.openjdk.org/jdk/pull/13546#discussion_r1178984510
Re: RFR: 8306836: Remove pinned tag for G1 heap regions [v3]
On Wed, 26 Apr 2023 09:12:24 GMT, Thomas Schatzl wrote:
>> Hi all,
>>
>> please review this change that removes the pinned tag from `HeapRegion`.
>>
>> So that "pinned" tag for G1 heap regions indicates that the region should
>> not move during (young) gc. This applies to now removed archive regions and
>> humongous objects/regions.
>>
>> With "real" g1 region pinning to deal with gclocker in g1 once and for all
>> upcoming we need a refcount, a single bit is not sufficient anymore. Further
>> there will be a naming conflict as this kind of "pinning" is different to g1
>> region pinning "pinning". The former indicates "contents can not be moved,
>> but can be reclaimed", while the latter means "contents can not be moved and
>> not reclaimed".
>>
>> The (current) pinned flag is surprisingly little used, only for policy
>> decisions.
>>
>> The suggestion this change implements is to remove the "pinned" tag as it
>> is, and reserve it for future g1 region pinning (that needs to store the
>> pinning attribute differently as a refcount anyway).
>>
>> Testing: tier1-3, gha
>>
>> Thanks,
>> Thomas
>
> Thomas Schatzl has updated the pull request incrementally with one additional
> commit since the last revision:
>
> cplummer review
> I think you are right about using is_humongous() directly here: the reason we
> skip compacting of humongous regions during the "main" compaction is
> intentional here
However, I am unable to discern the difference -- why `is_young_gc_movable` is
semantically-correct in one place, but not in the other in this concrete
example.
bool G1CollectionSetChooser::should_add(HeapRegion* hr) {
return !hr->is_young() &&
G1CollectedHeap::heap()->policy()->is_young_gc_movable(hr) &&
vs
void G1FullCollector::before_marking_update_attribute_table(HeapRegion* hr) {
if (hr->is_free()) {
_region_attr_table.set_free(hr->hrm_index());
} else if (hr->is_humongous()) {
Looking at where `G1CollectionSetChooser::should_add` is called, can one use
`hr->is_old()` instead of `!hr->is_young() &&
G1CollectedHeap::heap()->policy()->is_young_gc_movable(hr)`? (In fact, I
believe that inlining `should_add` to the caller would result in a smoother
code flow and prevent some duplicate region-type queries.)
In my opinion, introducing a new `is_young_gc_movable` API in this particular
PR may not be entirely justified. It may make more sense to introduce it in
later PRs where region-pinning is supported and the API is actually utilized.
-
PR Comment: https://git.openjdk.org/jdk/pull/13643#issuecomment-1525434952
Re: RFR: 8305913: com/sun/jdi/JdbLastErrorTest.java failed with "'lastError = 42' missing from stdout/stderr" [v2]
On Sat, 15 Apr 2023 10:15:20 GMT, Kevin Walls wrote: >> This test is failing often since 8304725 added a call to >> Thread::current_in_asgct(). This can end up being called e.g. when >> resolving calls, and then the OS last error value is lost. >> >> The test is reliable with a single warm-up call to getLastError.invoke() >> before the loop. >> >> The test was introduced when in JDK-8292302 a change was undone that had >> made JavaThread::threadObj call Thread::current_or_null_safe, as the use of >> TLS upset this case of accessing last error directly. >> >> This new Thread::current_in_asgct() case shows that the VM will find new >> ways to interfere with the last error value, or at least new VM code keeps >> wanting to call Thread::current. This testcase is kind of niche usage, so >> it not an argument that VM code should not be calling Thread::current. If >> this test is to stay active, it needs to have this warm-up getLastError >> call. (If there are more issues, it might mean removing the test.) > > Kevin Walls has updated the pull request incrementally with one additional > commit since the last revision: > > comment update feedback Thanks for looking at this, and for the Panama pointer to JDK-8294970 - I'm checking if that completely solves it for this testcase... - PR Comment: https://git.openjdk.org/jdk/pull/13481#issuecomment-1525338902
Re: RFR: 8306034: add support of virtual threads to JVMTI StopThread [v6]
> 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: install_async_exception: set interrupt status for platform threads only - Changes: - all: https://git.openjdk.org/jdk/pull/13546/files - new: https://git.openjdk.org/jdk/pull/13546/files/956e8ee8..0113f034 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13546&range=05 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13546&range=04-05 Stats: 14 lines in 2 files changed: 9 ins; 0 del; 5 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: 8291555: Implement alternative fast-locking scheme [v64]
> 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 li
Integrated: 8306858: Remove some remnants of CMS from SA agent
On Tue, 25 Apr 2023 16:25:40 GMT, Thomas Schatzl wrote: > Hi all, > > please review this change that removes some remaining CMS related stuff. > > Testing: tier1-3, gha > > Thanks, > Thomas This pull request has now been integrated. Changeset: d94ce656 Author:Thomas Schatzl URL: https://git.openjdk.org/jdk/commit/d94ce6566d50fc0a6218adbb64d8f90e9eeb844a Stats: 16 lines in 4 files changed: 0 ins; 12 del; 4 mod 8306858: Remove some remnants of CMS from SA agent Reviewed-by: shade, cjplummer, kbarrett, ysr - PR: https://git.openjdk.org/jdk/pull/13646
Re: RFR: 8306858: Remove some remnants of CMS from SA agent [v2]
On Tue, 25 Apr 2023 16:43:27 GMT, Aleksey Shipilev wrote: >> Thomas Schatzl has updated the pull request incrementally with one >> additional commit since the last revision: >> >> cplummer review > > Looks fine to me. > > But the synopsis has a typo, "remnants". Thanks @shipilev @kimbarrett @plummercj @ysr for you reviews. - PR Comment: https://git.openjdk.org/jdk/pull/13646#issuecomment-1524964919
