Re: RFR: 8332042: Move MEMFLAGS to its own include file [v3]
On Tue, 14 May 2024 07:19:32 GMT, Thomas Stuefe wrote: >> MEMFLAGS, as well as its enum constants, should live in its own include. >> >> The constants are used throughout the code base, often without needing the >> allocation APIs exposed through allocation.hpp. >> >> The MEMFLAGS enum def is often needed within NMT itself, again often without >> needing allocation.hpp. >> >> --- >> >> This patch moves the enum to its new file. >> >> It fixes those `allocation.hpp` includes that where only needed to get >> MEMFLAGS. It does not fix other includes. >> >> For backward compatibility, until we straightened out the dependencies >> (e.g., fixing all places where we rely on indirect includes), I added >> memflags.hpp to allocation.hpp. >> >> I tested (built) on: >> - MacOS aarch64, no precompiled headers, fastdebug >> - Linux x64, no precompiled headers, fastdebug, release, fastdebug >> crossbuild to aarch64, fastdebug minimal > > Thomas Stuefe has updated the pull request incrementally with one additional > commit since the last revision: > > Feedback StefanK Looks good. Thanks! - Marked as reviewed by stefank (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19172#pullrequestreview-2055637663
Re: RFR: 8332042: Move MEMFLAGS to its own include file [v2]
On Mon, 13 May 2024 15:36:13 GMT, Stefan Karlsson wrote: >> To quote @robehn - Why write a comment for a rule if you can enforce it with >> code instead... > > I tend to agree with that. My earlier question still stands: is there a > better place to put it? Right now the "enforced with code" in a stand-alone > file doesn't tell me "why" this rule is important. If you want to keep the static_assert it in the .cpp file, then I won't block that. - PR Review Comment: https://git.openjdk.org/jdk/pull/19172#discussion_r1598695748
Re: RFR: 8332042: Move MEMFLAGS to its own include file [v2]
On Mon, 13 May 2024 15:26:18 GMT, Daniel D. Daugherty wrote: >> Could you instead put the static_assert near the code that will break? Right >> now it looks obscure and weird to have this check when it is obviously >> correct as long as no one changes the definition. Would it be enough to >> write a comment in the header that this needs to be 1 byte? > > To quote @robehn - Why write a comment for a rule if you can enforce it with > code instead... I tend to agree with that. My earlier question still stands is there a better place to put it? Right now the "enforced" code in a stand-alone file doesn't tell me "why" this is important. - PR Review Comment: https://git.openjdk.org/jdk/pull/19172#discussion_r1598679057
Re: RFR: 8332042: Move MEMFLAGS to its own include file [v2]
On Mon, 13 May 2024 14:31:22 GMT, Thomas Stuefe wrote: >> src/hotspot/share/nmt/memflags.cpp line 31: >> >>> 29: >>> 30: // Extra insurance that MEMFLAGS truly has the same size as uint8_t. >>> 31: STATIC_ASSERT(sizeof(MEMFLAGS) == sizeof(uint8_t)); >> >> I think you can remove this entire .cpp file. There's no need to check the >> size of an enum with a specified base type. > > I rather have this explicit check. If MEMFLAGS>1byte, things break, and I > would like to make that explicit. > > That said, I can move this static assert to the header. I just wanted to > avoid including debug.hpp. My original intent was for this cpp file to be the > place in the future for any MEMFLAGS related utility functions, e.g. > to-and-from-string conversations. Could you instead put the static_assert near the code that will break? Right now it looks obscure and weird to have this check when it is obviously correct as long as no one changes the definition. Would it be enough to write a comment in the header that this needs to be 1 byte? >> src/hotspot/share/nmt/memflags.hpp line 30: >> >>> 28: #include "utilities/globalDefinitions.hpp" >>> 29: >>> 30: #define MEMORY_TYPES_DO(f) >>> \ >> >> Open-ended comment/question: We call it MEMORY_TYPE and mt, but then we call >> the type MEMFLAGS (with a completely non-standard UPPERCASE style). Maybe it >> is time to rename MEMFLAGS? > > I don't feel like starting that particular bike shedding discussion :) But > sure, sometime in the future we should do this. Here, I want it to be a > simple renaming change. Right. That's why I prefixed this with "Open-ended comment/question", trying to make it super clear that it wasn't intended as a request for this PR, but rather a way to at least plant the seed of an idea that we might want to fix this eyesore. - PR Review Comment: https://git.openjdk.org/jdk/pull/19172#discussion_r1598603277 PR Review Comment: https://git.openjdk.org/jdk/pull/19172#discussion_r1598608110
Re: RFR: 8332042: Move MEMFLAGS to its own include file [v2]
On Mon, 13 May 2024 04:55:24 GMT, Thomas Stuefe wrote: >> MEMFLAGS, as well as its enum constants, should live in its own include. >> >> The constants are used throughout the code base, often without needing the >> allocation APIs exposed through allocation.hpp. >> >> The MEMFLAGS enum def is often needed within NMT itself, again often without >> needing allocation.hpp. >> >> --- >> >> This patch moves the enum to its new file. >> >> It fixes those `allocation.hpp` includes that where only needed to get >> MEMFLAGS. It does not fix other includes. >> >> For backward compatibility, until we straightened out the dependencies >> (e.g., fixing all places where we rely on indirect includes), I added >> memflags.hpp to allocation.hpp. >> >> I tested (built) on: >> - MacOS aarch64, no precompiled headers, fastdebug >> - Linux x64, no precompiled headers, fastdebug, release, fastdebug >> crossbuild to aarch64, fastdebug minimal > > Thomas Stuefe has updated the pull request incrementally with one additional > commit since the last revision: > > Update mallocLimit.hpp Changes requested by stefank (Reviewer). src/hotspot/share/nmt/mallocTracker.hpp line 29: > 27: #define SHARE_NMT_MALLOCTRACKER_HPP > 28: > 29: #include "nmt/memflags.hpp" Should go after mallocHeader.hpp src/hotspot/share/nmt/memflags.cpp line 27: > 25: #include "precompiled.hpp" > 26: > 27: #include "nmt/memflags.hpp" There should be no blankline between precompiled.hpp and the rest of the includes. src/hotspot/share/nmt/memflags.cpp line 31: > 29: > 30: // Extra insurance that MEMFLAGS truly has the same size as uint8_t. > 31: STATIC_ASSERT(sizeof(MEMFLAGS) == sizeof(uint8_t)); I think you can remove this entire .cpp file. There's no need to check the size of an enum with a specified base type. src/hotspot/share/nmt/memflags.hpp line 30: > 28: #include "utilities/globalDefinitions.hpp" > 29: > 30: #define MEMORY_TYPES_DO(f) >\ Open-ended comment/question: We call it MEMORY_TYPE and mt, but then we call the type MEMFLAGS (with a completely non-standard UPPERCASE style). Maybe it is time to rename MEMFLAGS? src/hotspot/share/services/mallocLimit.cpp line 28: > 26: #include "precompiled.hpp" > 27: > 28: #include "nmt/memflags.hpp" While poking around in the includes, could you remove the blankline on 27. This style inconsistency has slowly crept into the code base. - PR Review: https://git.openjdk.org/jdk/pull/19172#pullrequestreview-2052269037 PR Review Comment: https://git.openjdk.org/jdk/pull/19172#discussion_r1598224275 PR Review Comment: https://git.openjdk.org/jdk/pull/19172#discussion_r1598225428 PR Review Comment: https://git.openjdk.org/jdk/pull/19172#discussion_r1598226640 PR Review Comment: https://git.openjdk.org/jdk/pull/19172#discussion_r1598229830 PR Review Comment: https://git.openjdk.org/jdk/pull/19172#discussion_r1598231329
Re: RFR: 8331573: Rename CollectedHeap::is_gc_active to be explicitly about STW GCs
On Thu, 2 May 2024 17:23:21 GMT, Aleksey Shipilev wrote:
>> src/hotspot/share/gc/parallel/psParallelCompact.cpp line 1270:
>>
>>> 1268:
>>> 1269: ParallelScavengeHeap* heap = ParallelScavengeHeap::heap();
>>> 1270: assert(!heap->is_stw_gc_active(), "not reentrant");
>>
>> While reading this I see that all these "not reentrant" asserts seems
>> redundant given that we already do these checks in `IsSTWGCActiveMark`.
>> Brownies points if you get rid of them. ;)
>
> Ah, hm. Indeed! Separate PR? There is some light cleanup in G1 that can be
> associated with it. This PR would keep with just a mechanical rename.
Sounds like a good idea.
>> src/hotspot/share/gc/parallel/psParallelCompact.cpp line 1493:
>>
>>> 1491: PCAddThreadRootsMarkingTaskClosure(uint worker_id) :
>>> _worker_id(worker_id) { }
>>> 1492: void do_thread(Thread* thread) {
>>> 1493: assert(ParallelScavengeHeap::heap()->is_stw_gc_active(), "called
>>> outside gc");
>>
>> Should this be updated to "called outside gc pause" as you did in
>> `G1CollectedHeap::pin_object`? The same comment goes for the other
>> occurrences below.
>
> I deliberately stopped myself from doing this for Parallel GC code, where
> every GC is STW GC :) I can change to "GC pause" if you want.
Ah, I see. I wouldn't mind if it were changed to include "pause", but I'm also
OK with you leaving it as is.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19064#discussion_r1588019866
PR Review Comment: https://git.openjdk.org/jdk/pull/19064#discussion_r1588018382
Re: RFR: 8331573: Rename CollectedHeap::is_gc_active to be explicitly about STW GCs
On Thu, 2 May 2024 14:40:35 GMT, Aleksey Shipilev wrote: > `CollectedHeap::is_gc_active()` is confusing, since its name implies _any_ GC > phase is running, while it actually only covers the STW GCs. It would be good > to rename it for clarity. The freed-up name, `is_gc_active` could then be > repurposed to track any (concurrent or STW) GC phase running. That would be > useful to resolve [JDK-8331572](https://bugs.openjdk.org/browse/JDK-8331572). > > Doing this rename separately guarantees we have caught and renamed all > current uses. > > Additional testing: > - [ ] Linux AArch64 server fastdebug, `all` Marked as reviewed by stefank (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19064#pullrequestreview-2036349526
Re: RFR: 8331573: Rename CollectedHeap::is_gc_active to be explicitly about STW GCs
On Thu, 2 May 2024 14:40:35 GMT, Aleksey Shipilev wrote:
> `CollectedHeap::is_gc_active()` is confusing, since its name implies _any_ GC
> phase is running, while it actually only covers the STW GCs. It would be good
> to rename it for clarity. The freed-up name, `is_gc_active` could then be
> repurposed to track any (concurrent or STW) GC phase running. That would be
> useful to resolve [JDK-8331572](https://bugs.openjdk.org/browse/JDK-8331572).
>
> Doing this rename separately guarantees we have caught and renamed all
> current uses.
>
> Additional testing:
> - [ ] Linux AArch64 server fastdebug, `all`
Seems like a good change.
src/hotspot/share/gc/parallel/psParallelCompact.cpp line 1270:
> 1268:
> 1269: ParallelScavengeHeap* heap = ParallelScavengeHeap::heap();
> 1270: assert(!heap->is_stw_gc_active(), "not reentrant");
While reading this I see that all these "not reentrant" asserts seems redundant
given that we already do these checks in `IsSTWGCActiveMark`. Brownies points
if you get rid of them. ;)
src/hotspot/share/gc/parallel/psParallelCompact.cpp line 1493:
> 1491: PCAddThreadRootsMarkingTaskClosure(uint worker_id) :
> _worker_id(worker_id) { }
> 1492: void do_thread(Thread* thread) {
> 1493: assert(ParallelScavengeHeap::heap()->is_stw_gc_active(), "called
> outside gc");
Should this be updated to "called outside gc pause" as you did in
`G1CollectedHeap::pin_object`? The same comment goes for the other occurrences
below.
-
Changes requested by stefank (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/19064#pullrequestreview-2036315901
PR Review Comment: https://git.openjdk.org/jdk/pull/19064#discussion_r1587988542
PR Review Comment: https://git.openjdk.org/jdk/pull/19064#discussion_r1587974562
Integrated: 8329629: GC interfaces should work directly against nmethod instead of CodeBlob
On Fri, 5 Apr 2024 12:32:30 GMT, Stefan Karlsson wrote: > The GCs scan and handles nmethods and ignores CodeBlobs of other kinds. The I > propose that we stop sending in CodeBlobs to the GCs and make sure to only > give them nmethods. > > I removed `void CodeCache::blobs_do(CodeBlobClosure* f)` since there's no > more usage of that function. Is this OK? > > I also opted to skipped calling the GC verification code from the iterator > code: > > Universe::heap()->verify_nmethod((nmethod*)cb); > > IMHO, I think it is up to the GCs to decide if they want to perform extra > nmethod verification. If someone wants to keep this verification in their > favorite GC I can add calls to this function where we used to call > CodeCache::blobs_do. > > I've only done limited testing and will run extensive testing concurrent with > the review. This pull request has now been integrated. Changeset: 87131fb2 Author:Stefan Karlsson URL: https://git.openjdk.org/jdk/commit/87131fb2f77188a483fd0852da5f9228aafd5336 Stats: 850 lines in 74 files changed: 238 ins; 318 del; 294 mod 8329629: GC interfaces should work directly against nmethod instead of CodeBlob Reviewed-by: ayang, eosterlund - PR: https://git.openjdk.org/jdk/pull/18653
Re: RFR: 8329629: GC interfaces should work directly against nmethod instead of CodeBlob
On Fri, 5 Apr 2024 12:32:30 GMT, Stefan Karlsson wrote: > The GCs scan and handles nmethods and ignores CodeBlobs of other kinds. The I > propose that we stop sending in CodeBlobs to the GCs and make sure to only > give them nmethods. > > I removed `void CodeCache::blobs_do(CodeBlobClosure* f)` since there's no > more usage of that function. Is this OK? > > I also opted to skipped calling the GC verification code from the iterator > code: > > Universe::heap()->verify_nmethod((nmethod*)cb); > > IMHO, I think it is up to the GCs to decide if they want to perform extra > nmethod verification. If someone wants to keep this verification in their > favorite GC I can add calls to this function where we used to call > CodeCache::blobs_do. > > I've only done limited testing and will run extensive testing concurrent with > the review. Thanks for the reviews! I'll let GHA complete before integrating. - PR Comment: https://git.openjdk.org/jdk/pull/18653#issuecomment-2044313453
Re: RFR: 8329629: GC interfaces should work directly against nmethod instead of CodeBlob [v2]
> The GCs scan and handles nmethods and ignores CodeBlobs of other kinds. The I > propose that we stop sending in CodeBlobs to the GCs and make sure to only > give them nmethods. > > I removed `void CodeCache::blobs_do(CodeBlobClosure* f)` since there's no > more usage of that function. Is this OK? > > I also opted to skipped calling the GC verification code from the iterator > code: > > Universe::heap()->verify_nmethod((nmethod*)cb); > > IMHO, I think it is up to the GCs to decide if they want to perform extra > nmethod verification. If someone wants to keep this verification in their > favorite GC I can add calls to this function where we used to call > CodeCache::blobs_do. > > I've only done limited testing and will run extensive testing concurrent with > the review. Stefan Karlsson 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 two additional commits since the last revision: - Merge remote-tracking branch 'upstream/master' into 8329629_do_code_blob - 8329629: GC interfaces should work directly against nmethod instead of CodeBlob - Changes: - all: https://git.openjdk.org/jdk/pull/18653/files - new: https://git.openjdk.org/jdk/pull/18653/files/e10683e2..1c2bdaea Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18653=01 - incr: https://webrevs.openjdk.org/?repo=jdk=18653=00-01 Stats: 5545 lines in 115 files changed: 3743 ins; 1387 del; 415 mod Patch: https://git.openjdk.org/jdk/pull/18653.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18653/head:pull/18653 PR: https://git.openjdk.org/jdk/pull/18653
RFR: 8329629: GC interfaces should work directly against nmethod instead of CodeBlob
The GCs scan and handles nmethods and ignores CodeBlobs of other kinds. The I propose that we stop sending in CodeBlobs to the GCs and make sure to only give them nmethods. I removed `void CodeCache::blobs_do(CodeBlobClosure* f)` since there's no more usage of that function. Is this OK? I also opted to skipped calling the GC verification code from the iterator code: Universe::heap()->verify_nmethod((nmethod*)cb); IMHO, I think it is up to the GCs to decide if they want to perform extra nmethod verification. If someone wants to keep this verification in their favorite GC I can add calls to this function where we used to call CodeCache::blobs_do. I've only done limited testing and will run extensive testing concurrent with the review. - Commit messages: - 8329629: GC interfaces should work directly against nmethod instead of CodeBlob Changes: https://git.openjdk.org/jdk/pull/18653/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18653=00 Issue: https://bugs.openjdk.org/browse/JDK-8329629 Stats: 850 lines in 74 files changed: 238 ins; 318 del; 294 mod Patch: https://git.openjdk.org/jdk/pull/18653.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18653/head:pull/18653 PR: https://git.openjdk.org/jdk/pull/18653
Integrated: 8329655: Cleanup KlassObj and klassOop names after the PermGen removal
On Thu, 4 Apr 2024 09:45:58 GMT, Stefan Karlsson wrote:
> We have a few places that uses the terms `KlassObj` and `klassOop` when
> referring to Klasses. This is old code from before the PermGen removal, when
> Klasses also were Java objects.
>
> These names tripped me up when I was reading the heap heapInspection.cpp and
> first though we were mixing the klass *mirror* objects and klass pointers in
> the hash code calculation:
>
>// An aligned reference address (typically the least
>// address in the perm gen) used for hashing klass
>// objects.
>HeapWord* _ref;
> ...
> _ref = (HeapWord*) Universe::boolArrayKlassObj();
> ...
> uint KlassInfoTable::hash(const Klass* p) {
> return (uint)(((uintptr_t)p - (uintptr_t)_ref) >> 2);
> }
>
>
> I propose that we rename these functions (and stop casting the Klass* to a
> (HeapWord*)).
>
> Tested with serviceability/dcmd/gc/ClassHistogramTest.java but will run this
> through our lower tiers.
This pull request has now been integrated.
Changeset: 71d48bcc
Author:Stefan Karlsson
URL:
https://git.openjdk.org/jdk/commit/71d48bcc3d6313ab4bd031b5e50ae3a16338abc8
Stats: 126 lines in 29 files changed: 0 ins; 2 del; 124 mod
8329655: Cleanup KlassObj and klassOop names after the PermGen removal
Reviewed-by: rkennke, coleenp
-
PR: https://git.openjdk.org/jdk/pull/18618
Re: RFR: 8329655: Cleanup KlassObj and klassOop names after the PermGen removal [v2]
On Thu, 4 Apr 2024 12:18:24 GMT, Stefan Karlsson wrote:
>> We have a few places that uses the terms `KlassObj` and `klassOop` when
>> referring to Klasses. This is old code from before the PermGen removal, when
>> Klasses also were Java objects.
>>
>> These names tripped me up when I was reading the heap heapInspection.cpp and
>> first though we were mixing the klass *mirror* objects and klass pointers in
>> the hash code calculation:
>>
>>// An aligned reference address (typically the least
>>// address in the perm gen) used for hashing klass
>>// objects.
>>HeapWord* _ref;
>> ...
>> _ref = (HeapWord*) Universe::boolArrayKlassObj();
>> ...
>> uint KlassInfoTable::hash(const Klass* p) {
>> return (uint)(((uintptr_t)p - (uintptr_t)_ref) >> 2);
>> }
>>
>>
>> I propose that we rename these functions (and stop casting the Klass* to a
>> (HeapWord*)).
>>
>> Tested with serviceability/dcmd/gc/ClassHistogramTest.java but will run this
>> through our lower tiers.
>
> Stefan Karlsson has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Review Roman
Thanks for the reviews!
-
PR Comment: https://git.openjdk.org/jdk/pull/18618#issuecomment-2039156773
Re: RFR: 8329332: Remove CompiledMethod and CodeBlobLayout classes [v4]
On Thu, 4 Apr 2024 17:04:30 GMT, Vladimir Kozlov wrote: >> Revert [JDK-8152664](https://bugs.openjdk.org/browse/JDK-8152664) RFE >> [changes](https://github.com/openjdk/jdk/commit/b853eb7f5ca24eeeda18acbb14287f706499c365) >> which was used for AOT [JEP 295](https://openjdk.org/jeps/295) >> implementation in JDK 9. The code was left in HotSpot assuming it will help >> in a future. But during work on Leyden we decided to not use it. In Leyden >> cached compiled code will be restored in CodeCache as normal nmethods: no >> need to change VM's runtime and GC code to process them. >> >> I may work on optimizing `CodeBlob` and `nmethod` fields layout to reduce >> header size in separate changes. In these changes I did simple fields >> reordering to keep small (1 byte) fields together. >> >> I do not see (and not expected) performance difference with these changes. >> >> Tested tier1-5, xcomp, stress. Running performance testing. >> >> I need help with testing on platforms which Oracle does not support. > > Vladimir Kozlov has updated the pull request incrementally with one > additional commit since the last revision: > > Address next round of comments Looks good. - Marked as reviewed by stefank (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18554#pullrequestreview-1980693606
Re: RFR: 8329332: Remove CompiledMethod and CodeBlobLayout classes [v3]
On Thu, 4 Apr 2024 15:56:34 GMT, Vladimir Kozlov wrote: >> src/hotspot/share/gc/shared/gcBehaviours.hpp line 31: >> >>> 29: #include "oops/oopsHierarchy.hpp" >>> 30: >>> 31: // This is the behaviour for checking if a nmethod is unloading >> >> Maybe this should be *an* nmethod? > > Quote: "an" goes before words that begin with vowels. I don't think that holds if the 'n' is pronounced the way nmethod is pronounced. - PR Review Comment: https://git.openjdk.org/jdk/pull/18554#discussion_r1552005187
Re: RFR: 8329655: Cleanup KlassObj and klassOop names after the PermGen removal [v2]
On Thu, 4 Apr 2024 12:13:21 GMT, Roman Kennke wrote: >> I think it is an old-style TODO. I'm considering if we shouldn't just remove >> these comments. What do people think about that? > > I'm not even sure what they want to say, really. Should be good to remove, > and if anybody can make sense of it, record an issue in the bug-tracker? OK. I removed the %%%. I'll wait a little bit to see if someone else wants to keep them for some reason, if not, I'll remove them. - PR Review Comment: https://git.openjdk.org/jdk/pull/18618#discussion_r1551568127
Re: RFR: 8329655: Cleanup KlassObj and klassOop names after the PermGen removal [v2]
On Thu, 4 Apr 2024 10:07:11 GMT, Roman Kennke wrote:
>> Stefan Karlsson has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Review Roman
>
> src/hotspot/share/memory/heapInspection.cpp line 173:
>
>> 171: KlassInfoTable::KlassInfoTable(bool add_all_classes) {
>> 172: _size_of_instances_in_words = 0;
>> 173: _ref = (uintptr_t) Universe::boolArrayKlass();
>
> It seems weird (non-obvious) to cast to uintptr_t here. I see it is only used
> in KlassInfoTable::hash(), which is weird, too. I am not sure that this even
> does a useful hashing. Might be worth to get rid of the whole thing and use
> the
> [fastHash](https://github.com/rkennke/jdk/blob/JDK-8305896/src/hotspot/share/utilities/fastHash.hpp)
> stuff that @rose00 proposed for Lilliput. Perhaps in a follow-up. I'd
> probably either cast to void* or Klass*, or cast to uintptr_t as you did and
> remove the unnecessary cast in ::hash().
I agree. I'll start by removing the redundant cast in `::hash()`.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18618#discussion_r1551554904
Re: RFR: 8329655: Cleanup KlassObj and klassOop names after the PermGen removal [v2]
> We have a few places that uses the terms `KlassObj` and `klassOop` when
> referring to Klasses. This is old code from before the PermGen removal, when
> Klasses also were Java objects.
>
> These names tripped me up when I was reading the heap heapInspection.cpp and
> first though we were mixing the klass *mirror* objects and klass pointers in
> the hash code calculation:
>
>// An aligned reference address (typically the least
>// address in the perm gen) used for hashing klass
>// objects.
>HeapWord* _ref;
> ...
> _ref = (HeapWord*) Universe::boolArrayKlassObj();
> ...
> uint KlassInfoTable::hash(const Klass* p) {
> return (uint)(((uintptr_t)p - (uintptr_t)_ref) >> 2);
> }
>
>
> I propose that we rename these functions (and stop casting the Klass* to a
> (HeapWord*)).
>
> Tested with serviceability/dcmd/gc/ClassHistogramTest.java but will run this
> through our lower tiers.
Stefan Karlsson has updated the pull request incrementally with one additional
commit since the last revision:
Review Roman
-
Changes:
- all: https://git.openjdk.org/jdk/pull/18618/files
- new: https://git.openjdk.org/jdk/pull/18618/files/02bcbd89..85f6bbe6
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=18618=01
- incr: https://webrevs.openjdk.org/?repo=jdk=18618=00-01
Stats: 5 lines in 5 files changed: 0 ins; 0 del; 5 mod
Patch: https://git.openjdk.org/jdk/pull/18618.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/18618/head:pull/18618
PR: https://git.openjdk.org/jdk/pull/18618
Re: RFR: 8329655: Cleanup KlassObj and klassOop names after the PermGen removal
On Thu, 4 Apr 2024 09:55:38 GMT, Roman Kennke wrote:
>> We have a few places that uses the terms `KlassObj` and `klassOop` when
>> referring to Klasses. This is old code from before the PermGen removal, when
>> Klasses also were Java objects.
>>
>> These names tripped me up when I was reading the heap heapInspection.cpp and
>> first though we were mixing the klass *mirror* objects and klass pointers in
>> the hash code calculation:
>>
>>// An aligned reference address (typically the least
>>// address in the perm gen) used for hashing klass
>>// objects.
>>HeapWord* _ref;
>> ...
>> _ref = (HeapWord*) Universe::boolArrayKlassObj();
>> ...
>> uint KlassInfoTable::hash(const Klass* p) {
>> return (uint)(((uintptr_t)p - (uintptr_t)_ref) >> 2);
>> }
>>
>>
>> I propose that we rename these functions (and stop casting the Klass* to a
>> (HeapWord*)).
>>
>> Tested with serviceability/dcmd/gc/ClassHistogramTest.java but will run this
>> through our lower tiers.
>
> src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp line 1202:
>
>> 1200: ldrw(scan_temp, Address(recv_klass, Klass::vtable_length_offset()));
>> 1201:
>> 1202: // %%% Could store the aligned, prescaled offset in the klassoop.
>
> Unrelated, but what's the point of the %%% in all those comments? Might want
> to remove that, while you're there.
I think it is an old-style TODO. I'm considering if we shouldn't just remove
these comments. What do people think about that?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18618#discussion_r1551548890
RFR: 8329655: Cleanup KlassObj and klassOop names after the PermGen removal
We have a few places that uses the terms `KlassObj` and `klassOop` when
referring to Klasses. This is old code from before the PermGen removal, when
Klasses also were Java objects.
These names tripped me up when I was reading the heap heapInspection.cpp and
first though we were mixing the klass *mirror* objects and klass pointers in
the hash code calculation:
// An aligned reference address (typically the least
// address in the perm gen) used for hashing klass
// objects.
HeapWord* _ref;
...
_ref = (HeapWord*) Universe::boolArrayKlassObj();
...
uint KlassInfoTable::hash(const Klass* p) {
return (uint)(((uintptr_t)p - (uintptr_t)_ref) >> 2);
}
I propose that we rename these functions (and stop casting the Klass* to a
(HeapWord*)).
Tested with serviceability/dcmd/gc/ClassHistogramTest.java but will run this
through our lower tiers.
-
Commit messages:
- 8329655: Cleanup KlassObj and klassOop names after the PermGen removal
Changes: https://git.openjdk.org/jdk/pull/18618/files
Webrev: https://webrevs.openjdk.org/?repo=jdk=18618=00
Issue: https://bugs.openjdk.org/browse/JDK-8329655
Stats: 125 lines in 29 files changed: 0 ins; 2 del; 123 mod
Patch: https://git.openjdk.org/jdk/pull/18618.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/18618/head:pull/18618
PR: https://git.openjdk.org/jdk/pull/18618
Re: RFR: 8329332: Remove CompiledMethod and CodeBlobLayout classes [v3]
On Thu, 4 Apr 2024 00:05:20 GMT, Vladimir Kozlov wrote:
>> Revert [JDK-8152664](https://bugs.openjdk.org/browse/JDK-8152664) RFE
>> [changes](https://github.com/openjdk/jdk/commit/b853eb7f5ca24eeeda18acbb14287f706499c365)
>> which was used for AOT [JEP 295](https://openjdk.org/jeps/295)
>> implementation in JDK 9. The code was left in HotSpot assuming it will help
>> in a future. But during work on Leyden we decided to not use it. In Leyden
>> cached compiled code will be restored in CodeCache as normal nmethods: no
>> need to change VM's runtime and GC code to process them.
>>
>> I may work on optimizing `CodeBlob` and `nmethod` fields layout to reduce
>> header size in separate changes. In these changes I did simple fields
>> reordering to keep small (1 byte) fields together.
>>
>> I do not see (and not expected) performance difference with these changes.
>>
>> Tested tier1-5, xcomp, stress. Running performance testing.
>>
>> I need help with testing on platforms which Oracle does not support.
>
> Vladimir Kozlov 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 five additional
> commits since the last revision:
>
> - Address comments
> - Merge branch 'master' into 8329332
> - Removed not_used state of nmethod
> - remove trailing whitespace
> - 8329332: Remove CompiledMethod and CodeBlobLayout classes
I took a second pass over the changes. I've given a few suggestions below. None
of them should require respinning of tests (except for making sure that this
still builds).
src/hotspot/share/code/codeBlob.hpp line 168:
> 166: bool is_vtable_blob() const { return _kind ==
> CodeBlobKind::Blob_Vtable; }
> 167: bool is_method_handles_adapter_blob() const { return _kind ==
> CodeBlobKind::Blob_MH_Adapter; }
> 168: bool is_upcall_stub() const { return _kind ==
> CodeBlobKind::Blob_Upcall; }
The `Blob_` prefix is now redundant since we always have to prefix with
CodeBlobKind::. Just a suggestion if you want to shorten these.
src/hotspot/share/gc/shared/gcBehaviours.hpp line 31:
> 29: #include "oops/oopsHierarchy.hpp"
> 30:
> 31: // This is the behaviour for checking if a nmethod is unloading
Maybe this should be *an* nmethod?
src/hotspot/share/gc/shenandoah/shenandoahUnload.cpp line 81:
> 79: class ShenandoahIsUnloadingBehaviour : public IsUnloadingBehaviour {
> 80: public:
> 81: virtual bool has_dead_oop(nmethod* const nm) const {
Is there a reason why this was changed to `nmethod* const nm` instead of
`nmethod* nm`? IsUnloadingBehviour::has_dead_oop uses `nmethod* nm`. This
question applies to the other changes in this file as well.
src/hotspot/share/gc/x/xUnload.cpp line 78:
> 76: class XIsUnloadingBehaviour : public IsUnloadingBehaviour {
> 77: public:
> 78: virtual bool has_dead_oop(nmethod* const nm) const {
`nmethod* const nm` => `nmethod* nm`. (ZGC's style is to use const for local
variables, but not for variables in the parameter list). The same applies to
the rest of the changes to this file.
src/hotspot/share/gc/z/zUnload.cpp line 77:
> 75: class ZIsUnloadingBehaviour : public IsUnloadingBehaviour {
> 76: public:
> 77: virtual bool has_dead_oop(nmethod* const nm) const {
`nmethod* const nm` => `nmethod* nm`. (ZGC's style is to use const for local
variables, but not for variables in the parameter list). The same applies to
the rest of the changes to this file.
src/hotspot/share/runtime/javaThread.hpp line 123:
> 121: DeoptResourceMark* _deopt_mark; // Holds special
> ResourceMark for deoptimization
> 122:
> 123: nmethod* _deopt_nmethod; // nmethod that is
> currently being deoptimized
The alignment is (and was) weird here.
-
Marked as reviewed by stefank (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/18554#pullrequestreview-1978954058
PR Review Comment: https://git.openjdk.org/jdk/pull/18554#discussion_r1551107567
PR Review Comment: https://git.openjdk.org/jdk/pull/18554#discussion_r1551073461
PR Review Comment: https://git.openjdk.org/jdk/pull/18554#discussion_r1551077362
PR Review Comment: https://git.openjdk.org/jdk/pull/18554#discussion_r1551080101
PR Review Comment: https://git.openjdk.org/jdk/pull/18554#discussion_r1551080280
PR Review Comment: https://git.openjdk.org/jdk/pull/18554#discussion_r1551100400
Re: RFR: 8329332: Remove CompiledMethod and CodeBlobLayout classes [v2]
On Wed, 3 Apr 2024 16:38:13 GMT, Vladimir Kozlov wrote:
>> src/hotspot/share/code/codeCache.cpp line 1009:
>>
>>> 1007: int CodeCache::nmethod_count() {
>>> 1008: int count = 0;
>>> 1009: for (GrowableArrayIterator heap =
>>> _nmethod_heaps->begin(); heap != _nmethod_heaps->end(); ++heap) {
>>
>> Is there a reason why FOR_ALL_NMETHOD_HEAPS wasn't good fit here? I'm
>> wondering since the similar `CodeCache::blob_count()` still uses one of
>> these macros.
>
> No, `CodeCache::blob_count()` uses different macro `FOR_ALL_HEAPS(heap)`
> because it looks for all code blobs, not only nmethods.
>
> `CodeCache::nmethod_count()` is the only place where `FOR_ALL_NMETHOD_HEAPS `
> was used. So I decided to remove the macro.
I didn't say that blob_count used `FOR_ALL_NMETHODS_HEAP`. I wrote "one of
these macros". I still think this adds an inconsistency to the code that I
don't think is beneficial.
With that said, can't this be written as:
for (CodeHeap* heap : *_nmethod_heaps) {
Maybe yet another opportunity for cleanups.
>> src/hotspot/share/code/nmethod.cpp line 812:
>>
>>> 810: // By calling this nmethod entry barrier, it plays along and acts
>>> 811: // like any other nmethod found on the stack of a thread (fewer
>>> surprises).
>>> 812: nmethod* nm = as_nmethod_or_null();
>>
>> Calling as_nmethod_or_null() from within functions in the nmethod class is
>> suspicious. Shouldn't all such usages be removed? (I'm fine with doing that
>> as a separate change)
>
> Good catch! The code was moved from CompiledMethod where it made sense but
> now it is not needed. Here the change I will make:
>
> // like any other nmethod found on the stack of a thread (fewer
> surprises).
> -nmethod* nm = as_nmethod_or_null();
> -if (nm != nullptr && bs_nm->is_armed(nm)) {
> +nmethod* nm = this;
> +if (bs_nm->is_armed(nm)) {
>bool alive = bs_nm->nmethod_entry_barrier(nm);
Sounds good.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18554#discussion_r1550216628
PR Review Comment: https://git.openjdk.org/jdk/pull/18554#discussion_r1550217712
Re: RFR: 8329332: Remove CompiledMethod and CodeBlobLayout classes [v2]
On Wed, 3 Apr 2024 16:29:03 GMT, Vladimir Kozlov wrote:
>> src/hotspot/share/code/codeBlob.hpp line 409:
>>
>>> 407:
>>> 408: // GC/Verification support
>>> 409: virtual void preserve_callee_argument_oops(frame fr, const
>>> RegisterMap *reg_map, OopClosure* f) override { /* nothing to do */ }
>>
>> In the GC code we usually have either virtual OR override, but not both.
>> Could we skip `virtual` here? Or does the compiler code usually use both?
>
> No special rules here. I simply want to see all `virtual` methods explicitly
> and `override` is required by C++.
> I would like to keep it this way in these changes. I am investigating
> possibility to convert all these virtual methods to normal one to remove
> virtual table and virtual pointer (8 bytes) from CodeBlob class.
`override` is not required by C++. You do however mark all virtual methods with
`override` if any of the functions are marked with `override`. I think it would
be good to have a HotSpot code style discussion about this (but not in this
RFE).
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18554#discussion_r1550206804
Re: RFR: 8329332: Remove CompiledMethod and CodeBlobLayout classes [v2]
On Mon, 1 Apr 2024 21:07:31 GMT, Vladimir Kozlov wrote:
>> Revert [JDK-8152664](https://bugs.openjdk.org/browse/JDK-8152664) RFE
>> [changes](https://github.com/openjdk/jdk/commit/b853eb7f5ca24eeeda18acbb14287f706499c365)
>> which was used for AOT [JEP 295](https://openjdk.org/jeps/295)
>> implementation in JDK 9. The code was left in HotSpot assuming it will help
>> in a future. But during work on Leyden we decided to not use it. In Leyden
>> cached compiled code will be restored in CodeCache as normal nmethods: no
>> need to change VM's runtime and GC code to process them.
>>
>> I may work on optimizing `CodeBlob` and `nmethod` fields layout to reduce
>> header size in separate changes. In these changes I did simple fields
>> reordering to keep small (1 byte) fields together.
>>
>> I do not see (and not expected) performance difference with these changes.
>>
>> Tested tier1-5, xcomp, stress. Running performance testing.
>>
>> I need help with testing on platforms which Oracle does not support.
>
> Vladimir Kozlov has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Removed not_used state of nmethod
Nice!
We've wanted to clean up some interfaces between the CodeCache and the GC code
by using nmethod closures instead of CodeBlob closures. This change (and the
Sweeper removal) makes it possible to do those cleanups.
I've made a superficial pass over the patch to and left a few comments. Most of
those comments are things that would be nice to fix, but could also be left as
follow-up RFEs (if they are deemed to be worthy ideas to pursue).
src/hotspot/os/posix/signals_posix.cpp line 27:
> 25: #include "precompiled.hpp"
> 26: #include "code/codeCache.hpp"
> 27: #include "code/nmethod.hpp"
The include line needs to move down.
src/hotspot/share/code/codeBlob.hpp line 77:
> 75: // - data space
> 76:
> 77: enum CodeBlobKind : u1 {
It will probably be safer to change this to an enum class, so that the compiler
will help us if we mess up with the argument order when this is used in
function calls. I see that this patch switches the parameter order of some
functions, so I think it could be worth trying out.
src/hotspot/share/code/codeBlob.hpp line 409:
> 407:
> 408: // GC/Verification support
> 409: virtual void preserve_callee_argument_oops(frame fr, const RegisterMap
> *reg_map, OopClosure* f) override { /* nothing to do */ }
In the GC code we usually have either virtual OR override, but not both. Could
we skip `virtual` here? Or does the compiler code usually use both?
src/hotspot/share/code/codeBlob.hpp line 429:
> 427:SingletonBlob(
> 428: const char* name,
> 429: CodeBlobKind kind,
There's an alignment issue after this change.
src/hotspot/share/code/codeCache.cpp line 1009:
> 1007: int CodeCache::nmethod_count() {
> 1008: int count = 0;
> 1009: for (GrowableArrayIterator heap = _nmethod_heaps->begin();
> heap != _nmethod_heaps->end(); ++heap) {
Is there a reason why FOR_ALL_NMETHOD_HEAPS wasn't good fit here? I'm wondering
since the similar `CodeCache::blob_count()` still uses one of these macros.
src/hotspot/share/code/nmethod.cpp line 812:
> 810: // By calling this nmethod entry barrier, it plays along and acts
> 811: // like any other nmethod found on the stack of a thread (fewer
> surprises).
> 812: nmethod* nm = as_nmethod_or_null();
Calling as_nmethod_or_null() from within functions in the nmethod class is
suspicious. Shouldn't all such usages be removed? (I'm fine with doing that as
a separate change)
src/hotspot/share/code/nmethod.cpp line 1009:
> 1007: // Fill in default values for various flag fields
> 1008: void nmethod::init_defaults() {
> 1009: { // avoid uninitialized fields, even for short time periods
Should these curly braces be removed?
src/hotspot/share/code/nmethod.cpp line 2164:
> 2162: DTRACE_METHOD_UNLOAD_PROBE(method());
> 2163:
> 2164: // If a JVMTI agent has enabled the nmethod Unload event then
I think the event is still called CompiledMethodUnload, so this line should
probably be reverted.
src/hotspot/share/code/nmethod.hpp line 50:
> 48: class ScopeDesc;
> 49: class CompiledIC;
> 50: class MetadataClosure;
Maybe merge (and sort) this together with the other forward declarations?
src/hotspot/share/code/nmethod.hpp line 905:
> 903:
> 904: // printing support
> 905: void print() const override;
Here and a few other places you only use override and not also virtual. This is
inconsistent with other functions in this class. (FWIW, I prefer this style
with only the override qualifier).
src/hotspot/share/code/nmethod.inline.hpp line 60:
> 58: // (b) it is a deopt PC
> 59:
> 60: inline address nmethod::get_deopt_original_pc(const frame* fr) {
While reading this PR I wonder if this really belongs in the `nmethod` class or
if it would make more sense to have it as a member function in the `frame`
Re: RFR: 8236736: Change notproduct JVM flags to develop flags [v2]
On Tue, 2 Apr 2024 16:24:19 GMT, Coleen Phillimore wrote:
>> Remove the notproduct distinction for command line options, rather than
>> trying to wrestle the macros to fix the bug that they've been treated as
>> develop options for some time now. This simplifies the command line option
>> macros.
>>
>> Tested with tier1-4, tier1 on Oracle platforms. Also built shenandoah.
>
> Coleen Phillimore has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Clean up notproduct from tests.
src/hotspot/share/runtime/arguments.cpp line 3420:
> 3418: static void apply_debugger_ergo() {
> 3419: #ifndef PRODUCT
> 3420: // UseDebuggerErgo is notproduct
Now that the flag has been changed to a develop flag, it seems wrong that these
are guarded by "#ifndef PRODUCT". Shouldn't this be changed to check for ASSERT
instead?
src/hotspot/share/runtime/flags/jvmFlag.hpp line 118:
> 116: EXPERIMENTAL_FLAG_BUT_LOCKED,
> 117: DEVELOPER_FLAG_BUT_PRODUCT_BUILD,
> 118: NOTPRODUCT_FLAG_BUT_PRODUCT_BUILD
Should the ',' on the previous line be removed?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18541#discussion_r1548236362
PR Review Comment: https://git.openjdk.org/jdk/pull/18541#discussion_r1548239130
Re: RFR: JDK-8327769: jcmd GC.heap_dump without options should write to location given by -XX:HeapDumpPath, if set
On Tue, 12 Mar 2024 06:15:20 GMT, David Holmes wrote: > GC folk should be reviewing this not runtime. I don't fully agree with that. How these serviceability tools work, and their interfaces, are usually not something that we GC devs are directly responsible for. This change could be reviewed by any HotSpot developer that has a stake in the hprof tooling. - PR Comment: https://git.openjdk.org/jdk/pull/18190#issuecomment-1991019456
Re: RFR: 8252136: Several methods in hotspot are missing "static" [v2]
On Tue, 13 Feb 2024 11:05:30 GMT, Magnus Ihse Bursie wrote: >> There are several places in hotspot where an internal function should have >> been declared static, but isn't. >> >> These were discovered by trying to use the gcc option >> `-Wmissing-declarations` and the corresponding clang option >> `-Wmissing-prototypes`. These warnings check that a function either: >> a) is declared static, or >> b) has a declaration before its definition. >> >> The rationale of this is that functions are either internal to a compilation >> unit, or exported to be linked by some other compilation unit. In the former >> case, it should be marked static. In the latter case, it should be declared >> in a header file, which should be included by the implementation as well. If >> there is a discrepancy between the exported prototype and the implemented >> prototype, this will be discovered during compilation (instead of as a >> runtime error). Additionally, marking internal methods as static allows the >> compiler to make better optimization, like inlining. >> >> This seem to be to be a sane assumption, and I think Hotspot (and the entire >> JDK) would increase code quality by turning on these warnings. The absolute >> majority of the code already adheres to these rules, but there are still >> some places that needs to be fixed. >> >> This is the first part of addressing these issues, where all places that are >> trivially missing static are fixed. >> >> I have discovered these by running with the warnings mentioned above turned >> on. I have filtered out those places were an export was obviously missing. >> The remaining warnings I have manually inspected. About 1/4 of them were >> *_init() functions (which are directly declared in `init.cpp`) and another >> 1/4 were documented as "use in debugger"; these I have not touched. I also >> ignored functions with names suggesting it might be used in the debugger, >> even if not documented as such, or any places that even seemed remotely >> non-trivial. Finally I also reverted a few changes after it turned out that >> gcc complained about unused functions. These places are actually dead code, >> but it is not clear if they should be removed or if there is actually a call >> missing somewhere (I believe it is a mix of both). In any case, I did not >> want any such complexities in this PR. >> >> When the functions where marked static, gcc started complaining if they were >> not used, since it consider it dead code. To address this, I had to add or >> fix some `#ifdef`s. Since this code were not actually used unless these >> criteria were fulfilled before either (it was just not disc... > > Magnus Ihse Bursie has updated the pull request incrementally with two > additional commits since the last revision: > > - Revert spurious changes in non-modified file > - Fix indentation issues Marked as reviewed by stefank (Reviewer). Yes, the GC code looks good. Thanks! - PR Review: https://git.openjdk.org/jdk/pull/17806#pullrequestreview-1879939402 PR Comment: https://git.openjdk.org/jdk/pull/17806#issuecomment-1943501692
Re: RFR: 8252136: Several methods in hotspot are missing "static"
On Mon, 12 Feb 2024 12:43:09 GMT, Magnus Ihse Bursie wrote: > There are several places in hotspot where an internal function should have > been declared static, but isn't. > > These were discovered by trying to use the gcc option > `-Wmissing-declarations` and the corresponding clang option > `-Wmissing-prototypes`. These warnings check that a function either: > a) is declared static, or > b) has a declaration before its definition. > > The rationale of this is that functions are either internal to a compilation > unit, or exported to be linked by some other compilation unit. In the former > case, it should be marked static. In the latter case, it should be declared > in a header file, which should be included by the implementation as well. If > there is a discrepancy between the exported prototype and the implemented > prototype, this will be discovered during compilation (instead of as a > runtime error). Additionally, marking internal methods as static allows the > compiler to make better optimization, like inlining. > > This seem to be to be a sane assumption, and I think Hotspot (and the entire > JDK) would increase code quality by turning on these warnings. The absolute > majority of the code already adheres to these rules, but there are still some > places that needs to be fixed. > > This is the first part of addressing these issues, where all places that are > trivially missing static are fixed. > > I have discovered these by running with the warnings mentioned above turned > on. I have filtered out those places were an export was obviously missing. > The remaining warnings I have manually inspected. About 1/4 of them were > *_init() functions (which are directly declared in `init.cpp`) and another > 1/4 were documented as "use in debugger"; these I have not touched. I also > ignored functions with names suggesting it might be used in the debugger, > even if not documented as such, or any places that even seemed remotely > non-trivial. Finally I also reverted a few changes after it turned out that > gcc complained about unused functions. These places are actually dead code, > but it is not clear if they should be removed or if there is actually a call > missing somewhere (I believe it is a mix of both). In any case, I did not > want any such complexities in this PR. > > When the functions where marked static, gcc started complaining if they were > not used, since it consider it dead code. To address this, I had to add or > fix some `#ifdef`s. Since this code were not actually used unless these > criteria were fulfilled before either (it was just not discovered by the > compiler), I thi... The argument alignment is wonky after this patch. Could you go over the patch and fix that? - Changes requested by stefank (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17806#pullrequestreview-1875713907
Re: RFR: 8325464: GCCause.java out of sync with gcCause.hpp [v2]
On Fri, 9 Feb 2024 05:31:17 GMT, Yifeng Jin wrote: >> These two files (`GCCause.java` and `gcCause.hpp`) should be in sync by >> design, see comments in these two files. However, some recent changes (e.g. >> [JDK-8240239](https://bugs.openjdk.org/browse/JDK-8240239)) to `gcCause.hpp` >> were not simultaneously reflected in `GCCause.java`. This patch updates >> `GCCause.java` to keep sync with latest `gcCause.hpp`. > > Yifeng Jin has updated the pull request incrementally with one additional > commit since the last revision: > > swap zgc and shenandoah block Looks good. - Marked as reviewed by stefank (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17766#pullrequestreview-1871801118
Re: RFR: 8325464: GCCause.java out of sync with gcCause.hpp
On Thu, 8 Feb 2024 06:19:16 GMT, Yifeng Jin wrote:
> These two files (`GCCause.java` and `gcCause.hpp`) should be in sync by
> design, see comments in these two files. However, some recent changes (e.g.
> [JDK-8240239](https://bugs.openjdk.org/browse/JDK-8240239)) to `gcCause.hpp`
> were not simultaneously reflected in `GCCause.java`. This patch updates
> `GCCause.java` to keep sync with latest `gcCause.hpp`.
Changes requested by stefank (Reviewer).
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/gc/shared/GCCause.java line
65:
> 63: _z_allocation_stall ("Allocation Stall"),
> 64: _z_proactive ("Proactive"),
> 65: _z_high_usage ("High Usage"),
gcCause.hpp has the Shenandoah and ZGC blocks swapped. Could you fix it in this
change as well?
-
PR Review: https://git.openjdk.org/jdk/pull/17766#pullrequestreview-1869553109
PR Review Comment: https://git.openjdk.org/jdk/pull/17766#discussion_r1482599594
Re: RFR: 8324512: Serial: Remove Generation::Name
On Tue, 23 Jan 2024 10:20:46 GMT, Albert Mingkun Yang wrote: > Trivial removing redundant code. Marked as reviewed by stefank (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17530#pullrequestreview-1838769192
Re: RFR: 8322989: New test serviceability/HeapDump/FullGCHeapDumpLimitTest.java fails [v5]
On Fri, 5 Jan 2024 02:22:45 GMT, Denghui Dong wrote: >> Hi, >> >> Please help review this patch that fixes the failures of >> FullGCHeapDumpLimitTest.java caused by passing other gc flags. >> >> Thanks. > > Denghui Dong has updated the pull request incrementally with one additional > commit since the last revision: > > update copyright Marked as reviewed by stefank (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17263#pullrequestreview-1805569410
Re: RFR: 8322989: New test serviceability/HeapDump/FullGCHeapDumpLimitTest.java fails [v3]
On Thu, 4 Jan 2024 11:56:45 GMT, David Holmes wrote: > > For this test I think we can just add @requires vm.gc.Serial > > @stefank but it doesn't require that, it explicitly sets that. The test > requires that no specific GC has been requested. @dholmes-ora `@requires vm.gc.Serial` doesn't mean that it requires Serial to be set. It's more subtle than that. It means that either Serial is set (and available) or it can be set (because no other GC has been selected). It is the correct requires line to use when you explicitly set the GC in the test. Look at our other GC tests. For example: test/hotspot/jtreg/gc/arguments/TestNewSizeThreadIncrease.java Without selecting a GC, the test is run: $ make -C ../build/fastdebug test TEST=test/hotspot/jtreg/gc/arguments/TestNewSizeThreadIncrease.java JTREG="JAVA_OPTIONS=-Xmx128m" ... TEST TOTAL PASS FAIL ERROR jtreg:open/test/hotspot/jtreg/gc/arguments/TestNewSizeThreadIncrease.java 1 1 0 0 With Serial as the selected GC, the test is run: $ make -C ../build/fastdebug test TEST=test/hotspot/jtreg/gc/arguments/TestNewSizeThreadIncrease.java JTREG="JAVA_OPTIONS=-Xmx128m -XX:+UseSerialGC" ... TEST TOTAL PASS FAIL ERROR jtreg:open/test/hotspot/jtreg/gc/arguments/TestNewSizeThreadIncrease.java 1 1 0 0 With G1 as the selected GC, the test is excluded: $ make -C ../build/fastdebug test TEST=test/hotspot/jtreg/gc/arguments/TestNewSizeThreadIncrease.java JTREG="JAVA_OPTIONS=-Xmx128m -XX:+UseG1GC" ... TEST TOTAL PASS FAIL ERROR jtreg:open/test/hotspot/jtreg/gc/arguments/TestNewSizeThreadIncrease.java 0 0 0 0 - PR Comment: https://git.openjdk.org/jdk/pull/17263#issuecomment-1877333642
Re: RFR: 8322989: New test serviceability/HeapDump/FullGCHeapDumpLimitTest.java fails
On Thu, 4 Jan 2024 08:01:57 GMT, Denghui Dong wrote: > Hi, > > Please help review this patch that fixes the failures of > FullGCHeapDumpLimitTest.java caused by passing other gc flags. > > Thanks. I prefer if we stay away from using `@requires vm.flagless` if possible. For this test I think we can just add `@requires vm.gc.Serial`? Could you try that instead? - Changes requested by stefank (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17263#pullrequestreview-1803844916
Integrated: 8321713: Harmonize executeTestJvm with create[Limited]TestJavaProcessBuilder
On Mon, 11 Dec 2023 09:15:50 GMT, Stefan Karlsson wrote: > [JDK-8315097](https://bugs.openjdk.org/browse/JDK-8315097): 'Rename > createJavaProcessBuilder' changed the name of the ProcessTools helper > functions used to create `ProcessBuilder`s used to spawn new java test > processes. > > We now have `createTestJavaProcessBuilder` and > `createLimitedTestJavaProcess`. The former prepends jvm options from jtreg, > while the latter doesn't. > > With these functions it is common to see the following pattern in tests: > > ProcessBuilder pb = ProcessTools.createTestJavaProcessBuilder(...); > OutputAnalyzer output = executeProcess(pb); > > > We have a couple of thin wrapper in `ProcessTools` that does exactly this, so > that the code can be written as a one-liner: > > OutputAnalyzer output = ProcessTools.executeTestJvm(); > > > I propose that we name this functions using the same naming scheme we used > for `createTestJavaProcessBuilder` and `createLimitedTestJavaProcessBuilder`. > That is, we change `executeTestJvm` to `executeTestJava` and add a new > `executeLimitedTestJava` function. This pull request has now been integrated. Changeset: cbe329b9 Author:Stefan Karlsson URL: https://git.openjdk.org/jdk/commit/cbe329b90ac1488836d4852fead79aa26c082114 Stats: 262 lines in 89 files changed: 73 ins; 1 del; 188 mod 8321713: Harmonize executeTestJvm with create[Limited]TestJavaProcessBuilder Reviewed-by: lkorinth, lmesnik - PR: https://git.openjdk.org/jdk/pull/17049
Re: RFR: 8321713: Harmonize executeTestJvm with create[Limited]TestJavaProcessBuilder [v5]
> [JDK-8315097](https://bugs.openjdk.org/browse/JDK-8315097): 'Rename > createJavaProcessBuilder' changed the name of the ProcessTools helper > functions used to create `ProcessBuilder`s used to spawn new java test > processes. > > We now have `createTestJavaProcessBuilder` and > `createLimitedTestJavaProcess`. The former prepends jvm options from jtreg, > while the latter doesn't. > > With these functions it is common to see the following pattern in tests: > > ProcessBuilder pb = ProcessTools.createTestJavaProcessBuilder(...); > OutputAnalyzer output = executeProcess(pb); > > > We have a couple of thin wrapper in `ProcessTools` that does exactly this, so > that the code can be written as a one-liner: > > OutputAnalyzer output = ProcessTools.executeTestJvm(); > > > I propose that we name this functions using the same naming scheme we used > for `createTestJavaProcessBuilder` and `createLimitedTestJavaProcessBuilder`. > That is, we change `executeTestJvm` to `executeTestJava` and add a new > `executeLimitedTestJava` function. Stefan Karlsson 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 five additional commits since the last revision: - Merge remote-tracking branch 'upstream/master' into rename_executeTestJvm - Merge remote-tracking branch 'upstream/master' into rename_executeTestJvm - Test cleanup - Fix impl and add test - 8321713: Harmonize executeTestJvm with create[Limited]TestJavaProcessBuilder - Changes: - all: https://git.openjdk.org/jdk/pull/17049/files - new: https://git.openjdk.org/jdk/pull/17049/files/486dc6d5..755d925d Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17049=04 - incr: https://webrevs.openjdk.org/?repo=jdk=17049=03-04 Stats: 875 lines in 70 files changed: 577 ins; 58 del; 240 mod Patch: https://git.openjdk.org/jdk/pull/17049.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17049/head:pull/17049 PR: https://git.openjdk.org/jdk/pull/17049
Re: RFR: 8321713: Harmonize executeTestJvm with create[Limited]TestJavaProcessBuilder [v3]
On Mon, 11 Dec 2023 14:06:43 GMT, Stefan Karlsson wrote: >> [JDK-8315097](https://bugs.openjdk.org/browse/JDK-8315097): 'Rename >> createJavaProcessBuilder' changed the name of the ProcessTools helper >> functions used to create `ProcessBuilder`s used to spawn new java test >> processes. >> >> We now have `createTestJavaProcessBuilder` and >> `createLimitedTestJavaProcess`. The former prepends jvm options from jtreg, >> while the latter doesn't. >> >> With these functions it is common to see the following pattern in tests: >> >> ProcessBuilder pb = ProcessTools.createTestJavaProcessBuilder(...); >> OutputAnalyzer output = executeProcess(pb); >> >> >> We have a couple of thin wrapper in `ProcessTools` that does exactly this, >> so that the code can be written as a one-liner: >> >> OutputAnalyzer output = ProcessTools.executeTestJvm(); >> >> >> I propose that we name this functions using the same naming scheme we used >> for `createTestJavaProcessBuilder` and >> `createLimitedTestJavaProcessBuilder`. That is, we change `executeTestJvm` >> to `executeTestJava` and add a new `executeLimitedTestJava` function. > > Stefan Karlsson has updated the pull request incrementally with one > additional commit since the last revision: > > Test cleanup Thanks for the reviews! - PR Comment: https://git.openjdk.org/jdk/pull/17049#issuecomment-1874176578
Re: RFR: 8321713: Harmonize executeTestJvm with create[Limited]TestJavaProcessBuilder [v4]
> [JDK-8315097](https://bugs.openjdk.org/browse/JDK-8315097): 'Rename > createJavaProcessBuilder' changed the name of the ProcessTools helper > functions used to create `ProcessBuilder`s used to spawn new java test > processes. > > We now have `createTestJavaProcessBuilder` and > `createLimitedTestJavaProcess`. The former prepends jvm options from jtreg, > while the latter doesn't. > > With these functions it is common to see the following pattern in tests: > > ProcessBuilder pb = ProcessTools.createTestJavaProcessBuilder(...); > OutputAnalyzer output = executeProcess(pb); > > > We have a couple of thin wrapper in `ProcessTools` that does exactly this, so > that the code can be written as a one-liner: > > OutputAnalyzer output = ProcessTools.executeTestJvm(); > > > I propose that we name this functions using the same naming scheme we used > for `createTestJavaProcessBuilder` and `createLimitedTestJavaProcessBuilder`. > That is, we change `executeTestJvm` to `executeTestJava` and add a new > `executeLimitedTestJava` function. Stefan Karlsson 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 four additional commits since the last revision: - Merge remote-tracking branch 'upstream/master' into rename_executeTestJvm - Test cleanup - Fix impl and add test - 8321713: Harmonize executeTestJvm with create[Limited]TestJavaProcessBuilder - Changes: - all: https://git.openjdk.org/jdk/pull/17049/files - new: https://git.openjdk.org/jdk/pull/17049/files/5d488f42..486dc6d5 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17049=03 - incr: https://webrevs.openjdk.org/?repo=jdk=17049=02-03 Stats: 5249 lines in 348 files changed: 3069 ins; 973 del; 1207 mod Patch: https://git.openjdk.org/jdk/pull/17049.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17049/head:pull/17049 PR: https://git.openjdk.org/jdk/pull/17049
Re: RFR: 8322476: Remove GrowableArray C-Heap version, replace usages with GrowableArrayCHeap
On Wed, 20 Dec 2023 21:11:09 GMT, Kim Barrett wrote: > I'm not a fan of the additional clutter in APIs that the static memory types > add. If we had a variant of GrowableArrayCHeap that was not itself > dynamically allocatable and took a memory type to use internally as a > constructor argument, then I think a lot of that clutter could be eliminated. > It could be used for ordinary data members that are direct GAs rather than > pointers to GAs. I think there is a way to do something similar for static > data members that are pointers that are dynamically allocated later, though > that probably requires more work. FWIW, I added the GrowableArrayCHeap and the static memory type. I did that because there was a perceived need to minimize the memory usage, because we were going to use an extreme amount of these arrays for one of our subsystems in ZGC. It later turned out that we really didn't need to squeeze out the last bit of memory for that use-case. I would really like to get rid of the the static memory type from GrowableArrayCHeap, and just add it as an instance member. - PR Comment: https://git.openjdk.org/jdk/pull/17160#issuecomment-1867683570
Re: RFR: 8321713: Harmonize executeTestJvm with create[Limited]TestJavaProcessBuilder [v3]
> [JDK-8315097](https://bugs.openjdk.org/browse/JDK-8315097): 'Rename > createJavaProcessBuilder' changed the name of the ProcessTools helper > functions used to create `ProcessBuilder`s used to spawn new java test > processes. > > We now have `createTestJavaProcessBuilder` and > `createLimitedTestJavaProcess`. The former prepends jvm options from jtreg, > while the latter doesn't. > > With these functions it is common to see the following pattern in tests: > > ProcessBuilder pb = ProcessTools.createTestJavaProcessBuilder(...); > OutputAnalyzer output = executeProcess(pb); > > > We have a couple of thin wrapper in `ProcessTools` that does exactly this, so > that the code can be written as a one-liner: > > OutputAnalyzer output = ProcessTools.executeTestJvm(); > > > I propose that we name this functions using the same naming scheme we used > for `createTestJavaProcessBuilder` and `createLimitedTestJavaProcessBuilder`. > That is, we change `executeTestJvm` to `executeTestJava` and add a new > `executeLimitedTestJava` function. Stefan Karlsson has updated the pull request incrementally with one additional commit since the last revision: Test cleanup - Changes: - all: https://git.openjdk.org/jdk/pull/17049/files - new: https://git.openjdk.org/jdk/pull/17049/files/ad072e06..5d488f42 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17049=02 - incr: https://webrevs.openjdk.org/?repo=jdk=17049=01-02 Stats: 10 lines in 1 file changed: 1 ins; 8 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/17049.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17049/head:pull/17049 PR: https://git.openjdk.org/jdk/pull/17049
Re: RFR: 8321713: Harmonize executeTestJvm with create[Limited]TestJavaProcessBuilder [v2]
> [JDK-8315097](https://bugs.openjdk.org/browse/JDK-8315097): 'Rename > createJavaProcessBuilder' changed the name of the ProcessTools helper > functions used to create `ProcessBuilder`s used to spawn new java test > processes. > > We now have `createTestJavaProcessBuilder` and > `createLimitedTestJavaProcess`. The former prepends jvm options from jtreg, > while the latter doesn't. > > With these functions it is common to see the following pattern in tests: > > ProcessBuilder pb = ProcessTools.createTestJavaProcessBuilder(...); > OutputAnalyzer output = executeProcess(pb); > > > We have a couple of thin wrapper in `ProcessTools` that does exactly this, so > that the code can be written as a one-liner: > > OutputAnalyzer output = ProcessTools.executeTestJvm(); > > > I propose that we name this functions using the same naming scheme we used > for `createTestJavaProcessBuilder` and `createLimitedTestJavaProcessBuilder`. > That is, we change `executeTestJvm` to `executeTestJava` and add a new > `executeLimitedTestJava` function. Stefan Karlsson has updated the pull request incrementally with one additional commit since the last revision: Fix impl and add test - Changes: - all: https://git.openjdk.org/jdk/pull/17049/files - new: https://git.openjdk.org/jdk/pull/17049/files/080caef5..ad072e06 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17049=01 - incr: https://webrevs.openjdk.org/?repo=jdk=17049=00-01 Stats: 54 lines in 2 files changed: 52 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/17049.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17049/head:pull/17049 PR: https://git.openjdk.org/jdk/pull/17049
RFR: 8321713: Harmonize executeTestJvm with create[Limited]TestJavaProcessBuilder
[JDK-8315097](https://bugs.openjdk.org/browse/JDK-8315097): 'Rename createJavaProcessBuilder' changed the name of the ProcessTools helper functions used to create `ProcessBuilder`s used to spawn new java test processes. We now have `createTestJavaProcessBuilder` and `createLimitedTestJavaProcess`. The former prepends jvm options from jtreg, while the latter doesn't. With these functions it is common to see the following pattern in tests: ProcessBuilder pb = ProcessTools.createTestJavaProcessBuilder(...); OutputAnalyzer output = executeProcess(pb); We have a couple of thin wrapper in `ProcessTools` that does exactly this, so that the code can be written as a one-liner: OutputAnalyzer output = ProcessTools.executeTestJvm(); I propose that we name this functions using the same naming scheme we used for `createTestJavaProcessBuilder` and `createLimitedTestJavaProcessBuilder`. That is, we change `executeTestJvm` to `executeTestJava` and add a new `executeLimitedTestJava` function. - Commit messages: - 8321713: Harmonize executeTestJvm with create[Limited]TestJavaProcessBuilder Changes: https://git.openjdk.org/jdk/pull/17049/files Webrev: https://webrevs.openjdk.org/?repo=jdk=17049=00 Issue: https://bugs.openjdk.org/browse/JDK-8321713 Stats: 217 lines in 88 files changed: 28 ins; 1 del; 188 mod Patch: https://git.openjdk.org/jdk/pull/17049.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17049/head:pull/17049 PR: https://git.openjdk.org/jdk/pull/17049
Integrated: 8320515: assert(monitor->object_peek() != nullptr) failed: Owned monitors should not have a dead object
On Wed, 22 Nov 2023 15:00:29 GMT, Stefan Karlsson wrote: > In the rewrites made for: > [JDK-8318757](https://bugs.openjdk.org/browse/JDK-8318757) `VM_ThreadDump > asserts in interleaved ObjectMonitor::deflate_monitor calls` > > I removed the filtering of *owned ObjectMonitors with dead objects*. The > reasoning was that you should never have an owned ObjectMonitor with a dead > object. I added an assert to check this assumption. It turns out that the > assumption was wrong *if* you use JNI to call MonitorEnter and then remove > all references to the locked object. > > The provided tests provoke this assert form: > * the JNI thread detach code > * thread dumping with locked monitors, and > * the JVMTI GetOwnedMonitorInfo API. > > While investigating this we've found that the thread detach code becomes more > correct when this filter was removed. Previously, the locked monitors never > got unlocked because the ObjectMonitor iterator never exposed these monitors > to the JNI detach code that unlocks the thread's monitors. That bug caused an > ObjectMonitor leak. So, for this case I'm leaving these ObjectMonitors > unfiltered so that we don't reintroduce the leak. > > The thread dumping case doesn't tolerate ObjectMonitor with dead objects, so > I'm filtering those in the closure that collects ObjectMonitor. Side note: We > have discussions about ways to completely rewrite this by letting each thread > have thread-local information about JNI held locks. If we have this we could > probably throw away the entire ObjectMonitorDump hashtable, and its walk of > the `_in_use_list.`. > > For GetOwnedMonitorInfo it is unclear if we should expose these weird > ObjectMonitor. If we do, then the users can detect that a thread holds a lock > with a dead object, and the code will return NULL as one of the "owned > monitors" returned. I don't think that's a good idea, so I'm filtering out > these ObjectMonitor for those calls. > > Test: the written tests with and without the fix. Tier1-Tier3, so far. This pull request has now been integrated. Changeset: 0d146361 Author:Stefan Karlsson URL: https://git.openjdk.org/jdk/commit/0d146361f27e1415fab9272de1cdde84c074c718 Stats: 326 lines in 8 files changed: 319 ins; 1 del; 6 mod 8320515: assert(monitor->object_peek() != nullptr) failed: Owned monitors should not have a dead object Reviewed-by: dholmes, ihse, sspitsyn, dcubed - PR: https://git.openjdk.org/jdk/pull/16783
Re: RFR: 8320515: assert(monitor->object_peek() != nullptr) failed: Owned monitors should not have a dead object [v9]
On Wed, 29 Nov 2023 06:38:51 GMT, Stefan Karlsson wrote: >> In the rewrites made for: >> [JDK-8318757](https://bugs.openjdk.org/browse/JDK-8318757) `VM_ThreadDump >> asserts in interleaved ObjectMonitor::deflate_monitor calls` >> >> I removed the filtering of *owned ObjectMonitors with dead objects*. The >> reasoning was that you should never have an owned ObjectMonitor with a dead >> object. I added an assert to check this assumption. It turns out that the >> assumption was wrong *if* you use JNI to call MonitorEnter and then remove >> all references to the locked object. >> >> The provided tests provoke this assert form: >> * the JNI thread detach code >> * thread dumping with locked monitors, and >> * the JVMTI GetOwnedMonitorInfo API. >> >> While investigating this we've found that the thread detach code becomes >> more correct when this filter was removed. Previously, the locked monitors >> never got unlocked because the ObjectMonitor iterator never exposed these >> monitors to the JNI detach code that unlocks the thread's monitors. That bug >> caused an ObjectMonitor leak. So, for this case I'm leaving these >> ObjectMonitors unfiltered so that we don't reintroduce the leak. >> >> The thread dumping case doesn't tolerate ObjectMonitor with dead objects, so >> I'm filtering those in the closure that collects ObjectMonitor. Side note: >> We have discussions about ways to completely rewrite this by letting each >> thread have thread-local information about JNI held locks. If we have this >> we could probably throw away the entire ObjectMonitorDump hashtable, and its >> walk of the `_in_use_list.`. >> >> For GetOwnedMonitorInfo it is unclear if we should expose these weird >> ObjectMonitor. If we do, then the users can detect that a thread holds a >> lock with a dead object, and the code will return NULL as one of the "owned >> monitors" returned. I don't think that's a good idea, so I'm filtering out >> these ObjectMonitor for those calls. >> >> Test: the written tests with and without the fix. Tier1-Tier3, so far. > > Stefan Karlsson has updated the pull request incrementally with one > additional commit since the last revision: > > Review comments Thanks all for reviewing! - PR Comment: https://git.openjdk.org/jdk/pull/16783#issuecomment-1833418484
Re: RFR: 8320515: assert(monitor->object_peek() != nullptr) failed: Owned monitors should not have a dead object [v8]
On Tue, 28 Nov 2023 18:38:07 GMT, Daniel D. Daugherty wrote: >> Stefan Karlsson has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Fix indentation > > I'll re-review again once the last set of comments are addressed. Thanks @dcubed-ojdk! - PR Comment: https://git.openjdk.org/jdk/pull/16783#issuecomment-1832357089
Re: RFR: 8320515: assert(monitor->object_peek() != nullptr) failed: Owned monitors should not have a dead object [v7]
On Mon, 27 Nov 2023 19:33:09 GMT, Serguei Spitsyn wrote: > It will be completely safe to run mach5 tiers 1-4, tier5-svc and 6. In this > particular case, the tier6 can be not necessary. It has some important > -Xcomp/interp_only_mode related testing. I've run the suggested testing and it passes (except a few unrelated issues) - PR Comment: https://git.openjdk.org/jdk/pull/16783#issuecomment-1831485088
Re: RFR: 8320515: assert(monitor->object_peek() != nullptr) failed: Owned monitors should not have a dead object [v9]
On Wed, 29 Nov 2023 07:00:06 GMT, David Holmes wrote: > Updates look good. I think that is all from me. Thanks for your patience on > the test issues. Thanks for the in-depth review! - PR Comment: https://git.openjdk.org/jdk/pull/16783#issuecomment-1831327248
Re: RFR: 8320515: assert(monitor->object_peek() != nullptr) failed: Owned monitors should not have a dead object [v9]
> In the rewrites made for: > [JDK-8318757](https://bugs.openjdk.org/browse/JDK-8318757) `VM_ThreadDump > asserts in interleaved ObjectMonitor::deflate_monitor calls` > > I removed the filtering of *owned ObjectMonitors with dead objects*. The > reasoning was that you should never have an owned ObjectMonitor with a dead > object. I added an assert to check this assumption. It turns out that the > assumption was wrong *if* you use JNI to call MonitorEnter and then remove > all references to the locked object. > > The provided tests provoke this assert form: > * the JNI thread detach code > * thread dumping with locked monitors, and > * the JVMTI GetOwnedMonitorInfo API. > > While investigating this we've found that the thread detach code becomes more > correct when this filter was removed. Previously, the locked monitors never > got unlocked because the ObjectMonitor iterator never exposed these monitors > to the JNI detach code that unlocks the thread's monitors. That bug caused an > ObjectMonitor leak. So, for this case I'm leaving these ObjectMonitors > unfiltered so that we don't reintroduce the leak. > > The thread dumping case doesn't tolerate ObjectMonitor with dead objects, so > I'm filtering those in the closure that collects ObjectMonitor. Side note: We > have discussions about ways to completely rewrite this by letting each thread > have thread-local information about JNI held locks. If we have this we could > probably throw away the entire ObjectMonitorDump hashtable, and its walk of > the `_in_use_list.`. > > For GetOwnedMonitorInfo it is unclear if we should expose these weird > ObjectMonitor. If we do, then the users can detect that a thread holds a lock > with a dead object, and the code will return NULL as one of the "owned > monitors" returned. I don't think that's a good idea, so I'm filtering out > these ObjectMonitor for those calls. > > Test: the written tests with and without the fix. Tier1-Tier3, so far. Stefan Karlsson has updated the pull request incrementally with one additional commit since the last revision: Review comments - Changes: - all: https://git.openjdk.org/jdk/pull/16783/files - new: https://git.openjdk.org/jdk/pull/16783/files/0e68fb68..ca6a7828 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16783=08 - incr: https://webrevs.openjdk.org/?repo=jdk=16783=07-08 Stats: 19 lines in 3 files changed: 3 ins; 3 del; 13 mod Patch: https://git.openjdk.org/jdk/pull/16783.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16783/head:pull/16783 PR: https://git.openjdk.org/jdk/pull/16783
Re: RFR: 8320515: assert(monitor->object_peek() != nullptr) failed: Owned monitors should not have a dead object [v5]
On Tue, 28 Nov 2023 05:38:33 GMT, David Holmes wrote: >> test/hotspot/jtreg/serviceability/jvmti/GetOwnedMonitorInfo/GetOwnedMonitorInfoTest.java >> line 1: >> >>> 1: /* >> >> Please update the `@bug` line and update the summary. > > This is still outstanding - thanks. Done. - PR Review Comment: https://git.openjdk.org/jdk/pull/16783#discussion_r1407367471
Re: RFR: 8320515: assert(monitor->object_peek() != nullptr) failed: Owned monitors should not have a dead object [v8]
On Mon, 27 Nov 2023 22:42:41 GMT, Daniel D. Daugherty
wrote:
>> Stefan Karlsson has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Fix indentation
>
> test/hotspot/jtreg/runtime/Monitor/MonitorWithDeadObjectTest.java line 2:
>
>> 1: /*
>> 2: * Copyright (c) 2022, 2023, Oracle and/or its affiliates. All rights
>> reserved.
>
> nit: why include 2022 in the copyright header?
See earlier comment.
> test/hotspot/jtreg/runtime/Monitor/libMonitorWithDeadObjectTest.c line 149:
>
>> 147: if ((*jvm)->DetachCurrentThread(jvm) != JNI_OK)
>> die("DetachCurrentThread");
>> 148:
>> 149: return NULL;
>
> Why is this function return type "void*" when it only returns NULL?
See earlier comment.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16783#discussion_r1407365519
PR Review Comment: https://git.openjdk.org/jdk/pull/16783#discussion_r1407366093
Re: RFR: 8320515: assert(monitor->object_peek() != nullptr) failed: Owned monitors should not have a dead object [v8]
On Tue, 28 Nov 2023 05:34:39 GMT, David Holmes wrote: >> Stefan Karlsson has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Fix indentation > > test/hotspot/jtreg/runtime/Monitor/MonitorWithDeadObjectTest.java line 88: > >> 86: // monitor with a dead object. The thread dumping code used to >> not >> 87: // tolerate such a monitor and would assert. Run a thread dump >> and make >> 88: // sure that it doesn't crash/assert. > > The comment is not correct, the monitor will be unlocked by the time > `createMonitorWithDeadObject` has returned. This post-dump is really just a > sanity test. All of these are sanity checks now that this PR fixes the bug. This specific test was added because of the combinations of bugs I've seen and provoked by temporarily reinstating various combinations of the bugs. Specifically, if the detach code skips visiting monitors with dead objects, but the thread dumping code does. That is, the opposite of the currently proposed patch. I'll update the comment. - PR Review Comment: https://git.openjdk.org/jdk/pull/16783#discussion_r1407339116
Re: RFR: 8320515: assert(monitor->object_peek() != nullptr) failed: Owned monitors should not have a dead object [v8]
On Tue, 28 Nov 2023 05:26:32 GMT, David Holmes wrote: >> Stefan Karlsson has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Fix indentation > > test/hotspot/jtreg/runtime/Monitor/MonitorWithDeadObjectTest.java line 35: > >> 33: >> 34: /* >> 35: * @requires os.family != "windows" & os.family != "aix" > > I guess we can fix the AIX usage later. This test tests a very specific platform-independent bug. Making it work for other platforms than Linux was done when it was easy to do so. I can't test AIX and don't know all the quirks of that OS, so I don't think it is worth for me to spend the time trying to make this test work for AIX. - PR Review Comment: https://git.openjdk.org/jdk/pull/16783#discussion_r1407327378
Re: RFR: 8320515: assert(monitor->object_peek() != nullptr) failed: Owned monitors should not have a dead object [v8]
On Tue, 28 Nov 2023 05:33:08 GMT, David Holmes wrote: >> @dcubed-ojdk These are the thread routines passed to `pthread_create` and >> must be typed as `void *(*start_routine)(void*)`. > > Though I just noticed the parameter is missing. As @stefank has pointed out > this was copied from another test so all of that other test's issues are/were > also present here unfortunately. I've updated the functions to use the correct signature. - PR Review Comment: https://git.openjdk.org/jdk/pull/16783#discussion_r1407321590
Re: RFR: 8320515: assert(monitor->object_peek() != nullptr) failed: Owned monitors should not have a dead object [v8]
On Tue, 28 Nov 2023 05:31:11 GMT, David Holmes wrote: >> test/hotspot/jtreg/runtime/Monitor/libMonitorWithDeadObjectTest.c line 110: >> >>> 108: >>> 109: // Let the GC clear the weak reference to the object. >>> 110: system_gc(env); >> >> A single GC may not be enough... > > @dcubed-ojdk there's some earlier discussion on this. Apparently a single GC > is sufficient to clear an oopStorage WeakHandle, even though it may not be > enough for Java level reference processing actions to be observed. One is enough for this test. - PR Review Comment: https://git.openjdk.org/jdk/pull/16783#discussion_r1407319349
Re: RFR: 8320515: assert(monitor->object_peek() != nullptr) failed: Owned monitors should not have a dead object [v8]
On Mon, 27 Nov 2023 22:46:15 GMT, Daniel D. Daugherty wrote: >> Stefan Karlsson has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Fix indentation > > test/hotspot/jtreg/runtime/Monitor/libMonitorWithDeadObjectTest.c line 2: > >> 1: /* >> 2: * Copyright (c) 2022, 2023, Oracle and/or its affiliates. All rights >> reserved. > > nit: why include 2022 in the copyright header? Because the test was copied and built upon a test that was created in 2022. > test/hotspot/jtreg/runtime/Monitor/libMonitorWithDeadObjectTest.c line 28: > >> 26: #include >> 27: #include >> 28: #include > > Should these be in sort order? Yes > test/hotspot/jtreg/runtime/Monitor/libMonitorWithDeadObjectTest.c line 121: > >> 119: create_monitor_with_dead_object(env); >> 120: >> 121: // DetachCurrenThread will try to unlock held monitors. This has been >> a > > nit typo: s/DetachCurrenThread/DetachCurrentThread/ Done - PR Review Comment: https://git.openjdk.org/jdk/pull/16783#discussion_r1407317548 PR Review Comment: https://git.openjdk.org/jdk/pull/16783#discussion_r1407318161 PR Review Comment: https://git.openjdk.org/jdk/pull/16783#discussion_r1407319756
Re: RFR: 8320515: assert(monitor->object_peek() != nullptr) failed: Owned monitors should not have a dead object [v8]
> In the rewrites made for: > [JDK-8318757](https://bugs.openjdk.org/browse/JDK-8318757) `VM_ThreadDump > asserts in interleaved ObjectMonitor::deflate_monitor calls` > > I removed the filtering of *owned ObjectMonitors with dead objects*. The > reasoning was that you should never have an owned ObjectMonitor with a dead > object. I added an assert to check this assumption. It turns out that the > assumption was wrong *if* you use JNI to call MonitorEnter and then remove > all references to the locked object. > > The provided tests provoke this assert form: > * the JNI thread detach code > * thread dumping with locked monitors, and > * the JVMTI GetOwnedMonitorInfo API. > > While investigating this we've found that the thread detach code becomes more > correct when this filter was removed. Previously, the locked monitors never > got unlocked because the ObjectMonitor iterator never exposed these monitors > to the JNI detach code that unlocks the thread's monitors. That bug caused an > ObjectMonitor leak. So, for this case I'm leaving these ObjectMonitors > unfiltered so that we don't reintroduce the leak. > > The thread dumping case doesn't tolerate ObjectMonitor with dead objects, so > I'm filtering those in the closure that collects ObjectMonitor. Side note: We > have discussions about ways to completely rewrite this by letting each thread > have thread-local information about JNI held locks. If we have this we could > probably throw away the entire ObjectMonitorDump hashtable, and its walk of > the `_in_use_list.`. > > For GetOwnedMonitorInfo it is unclear if we should expose these weird > ObjectMonitor. If we do, then the users can detect that a thread holds a lock > with a dead object, and the code will return NULL as one of the "owned > monitors" returned. I don't think that's a good idea, so I'm filtering out > these ObjectMonitor for those calls. > > Test: the written tests with and without the fix. Tier1-Tier3, so far. Stefan Karlsson has updated the pull request incrementally with one additional commit since the last revision: Fix indentation - Changes: - all: https://git.openjdk.org/jdk/pull/16783/files - new: https://git.openjdk.org/jdk/pull/16783/files/234175d9..0e68fb68 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16783=07 - incr: https://webrevs.openjdk.org/?repo=jdk=16783=06-07 Stats: 8 lines in 1 file changed: 0 ins; 0 del; 8 mod Patch: https://git.openjdk.org/jdk/pull/16783.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16783/head:pull/16783 PR: https://git.openjdk.org/jdk/pull/16783
Re: RFR: 8320515: assert(monitor->object_peek() != nullptr) failed: Owned monitors should not have a dead object [v7]
On Mon, 27 Nov 2023 19:04:42 GMT, Serguei Spitsyn wrote:
> The fix looks good to me. How was this tested?
Thanks. It was tested with the added and updated tests in GHA. Do you have any
suggestions for more tests to run?
> test/hotspot/jtreg/runtime/Monitor/libMonitorWithDeadObjectTest.c line 168:
>
>> 166: if (pthread_create(, NULL,
>> create_monitor_with_dead_object_and_dump_threads_in_thread, NULL) != 0)
>> die("pthread_create");
>> 167: if (pthread_join(attacher, ) != 0) die("pthread_join");
>> 168: }
>
> The lines 153-167 have an inconsistent indent.
Fixed. Thanks.
-
PR Comment: https://git.openjdk.org/jdk/pull/16783#issuecomment-182846
PR Review Comment: https://git.openjdk.org/jdk/pull/16783#discussion_r1406636914
Re: RFR: 8320515: assert(monitor->object_peek() != nullptr) failed: Owned monitors should not have a dead object [v5]
On Mon, 27 Nov 2023 18:58:14 GMT, Serguei Spitsyn wrote: >> Yeah, I was thinking the same. Maybe @sspitsyn or @plummercj could give >> guidance here? > > This looks okay. I see no problem with it. OK. Thanks. - PR Review Comment: https://git.openjdk.org/jdk/pull/16783#discussion_r1406637379
Re: RFR: 8320515: assert(monitor->object_peek() != nullptr) failed: Owned monitors should not have a dead object [v7]
> In the rewrites made for: > [JDK-8318757](https://bugs.openjdk.org/browse/JDK-8318757) `VM_ThreadDump > asserts in interleaved ObjectMonitor::deflate_monitor calls` > > I removed the filtering of *owned ObjectMonitors with dead objects*. The > reasoning was that you should never have an owned ObjectMonitor with a dead > object. I added an assert to check this assumption. It turns out that the > assumption was wrong *if* you use JNI to call MonitorEnter and then remove > all references to the locked object. > > The provided tests provoke this assert form: > * the JNI thread detach code > * thread dumping with locked monitors, and > * the JVMTI GetOwnedMonitorInfo API. > > While investigating this we've found that the thread detach code becomes more > correct when this filter was removed. Previously, the locked monitors never > got unlocked because the ObjectMonitor iterator never exposed these monitors > to the JNI detach code that unlocks the thread's monitors. That bug caused an > ObjectMonitor leak. So, for this case I'm leaving these ObjectMonitors > unfiltered so that we don't reintroduce the leak. > > The thread dumping case doesn't tolerate ObjectMonitor with dead objects, so > I'm filtering those in the closure that collects ObjectMonitor. Side note: We > have discussions about ways to completely rewrite this by letting each thread > have thread-local information about JNI held locks. If we have this we could > probably throw away the entire ObjectMonitorDump hashtable, and its walk of > the `_in_use_list.`. > > For GetOwnedMonitorInfo it is unclear if we should expose these weird > ObjectMonitor. If we do, then the users can detect that a thread holds a lock > with a dead object, and the code will return NULL as one of the "owned > monitors" returned. I don't think that's a good idea, so I'm filtering out > these ObjectMonitor for those calls. > > Test: the written tests with and without the fix. Tier1-Tier3, so far. Stefan Karlsson has updated the pull request incrementally with one additional commit since the last revision: Tweaks to jtreg run comments - Changes: - all: https://git.openjdk.org/jdk/pull/16783/files - new: https://git.openjdk.org/jdk/pull/16783/files/d08a930e..234175d9 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16783=06 - incr: https://webrevs.openjdk.org/?repo=jdk=16783=05-06 Stats: 6 lines in 1 file changed: 3 ins; 1 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/16783.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16783/head:pull/16783 PR: https://git.openjdk.org/jdk/pull/16783
Re: RFR: 8320515: assert(monitor->object_peek() != nullptr) failed: Owned monitors should not have a dead object [v6]
> In the rewrites made for: > [JDK-8318757](https://bugs.openjdk.org/browse/JDK-8318757) `VM_ThreadDump > asserts in interleaved ObjectMonitor::deflate_monitor calls` > > I removed the filtering of *owned ObjectMonitors with dead objects*. The > reasoning was that you should never have an owned ObjectMonitor with a dead > object. I added an assert to check this assumption. It turns out that the > assumption was wrong *if* you use JNI to call MonitorEnter and then remove > all references to the locked object. > > The provided tests provoke this assert form: > * the JNI thread detach code > * thread dumping with locked monitors, and > * the JVMTI GetOwnedMonitorInfo API. > > While investigating this we've found that the thread detach code becomes more > correct when this filter was removed. Previously, the locked monitors never > got unlocked because the ObjectMonitor iterator never exposed these monitors > to the JNI detach code that unlocks the thread's monitors. That bug caused an > ObjectMonitor leak. So, for this case I'm leaving these ObjectMonitors > unfiltered so that we don't reintroduce the leak. > > The thread dumping case doesn't tolerate ObjectMonitor with dead objects, so > I'm filtering those in the closure that collects ObjectMonitor. Side note: We > have discussions about ways to completely rewrite this by letting each thread > have thread-local information about JNI held locks. If we have this we could > probably throw away the entire ObjectMonitorDump hashtable, and its walk of > the `_in_use_list.`. > > For GetOwnedMonitorInfo it is unclear if we should expose these weird > ObjectMonitor. If we do, then the users can detect that a thread holds a lock > with a dead object, and the code will return NULL as one of the "owned > monitors" returned. I don't think that's a good idea, so I'm filtering out > these ObjectMonitor for those calls. > > Test: the written tests with and without the fix. Tier1-Tier3, so far. Stefan Karlsson has updated the pull request incrementally with one additional commit since the last revision: Review comments - Changes: - all: https://git.openjdk.org/jdk/pull/16783/files - new: https://git.openjdk.org/jdk/pull/16783/files/bad51926..d08a930e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16783=05 - incr: https://webrevs.openjdk.org/?repo=jdk=16783=04-05 Stats: 62 lines in 4 files changed: 6 ins; 43 del; 13 mod Patch: https://git.openjdk.org/jdk/pull/16783.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16783/head:pull/16783 PR: https://git.openjdk.org/jdk/pull/16783
Re: RFR: 8320515: assert(monitor->object_peek() != nullptr) failed: Owned monitors should not have a dead object [v5]
On Mon, 27 Nov 2023 02:17:27 GMT, David Holmes wrote:
>> Stefan Karlsson has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Split test and use othervm
>
> test/hotspot/jtreg/serviceability/jvmti/GetOwnedMonitorInfo/GetOwnedMonitorInfoTest.java
> line 78:
>
>> 76: final GetOwnedMonitorInfoTest lock = new
>> GetOwnedMonitorInfoTest();
>> 77:
>> 78: Thread t1 = threadFactory.newThread(() -> {
>
> Pre-existing nit: by default virtual threads have no name, so the output in
> the virtual thread case looks a little odd. Can you add:
>
> Thread.currentThread().setName("Worker-Thread");
>
> please.
Sure.
> test/hotspot/jtreg/serviceability/jvmti/GetOwnedMonitorInfo/libGetOwnedMonitorInfoTest.c
> line 270:
>
>> 268: Java_GetOwnedMonitorInfoTest_jniMonitorEnter(JNIEnv* env, jclass cls,
>> jobject obj) {
>> 269: if ((*env)->MonitorEnter(env, obj) != 0) {
>> 270: fprintf(stderr, "MonitorEnter failed");
>
> Should this be a fatal error?
I added a call to exit.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16783#discussion_r1405909361
PR Review Comment: https://git.openjdk.org/jdk/pull/16783#discussion_r1405906191
Re: RFR: 8320515: assert(monitor->object_peek() != nullptr) failed: Owned monitors should not have a dead object [v5]
On Mon, 27 Nov 2023 02:14:05 GMT, David Holmes wrote:
>> Stefan Karlsson has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Split test and use othervm
>
> test/hotspot/jtreg/runtime/Monitor/libMonitorWithDeadObjectTest.c line 130:
>
>> 128: // test provokes that situation and that asserts.
>> 129: if ((*jvm)->DetachCurrentThread(jvm) != JNI_OK)
>> die("DetachCurrentThread");
>> 130: pthread_exit(NULL);
>
> You don't need to call `pthread_exit` - the thread's entry function can
> simply return.
This is more code copied from CompleteExit.c.
> test/hotspot/jtreg/serviceability/jvmti/GetOwnedMonitorInfo/GetOwnedMonitorInfoTest.java
> line 53:
>
>> 51: private static native boolean hasEventPosted();
>> 52:
>> 53: private static void jniMonitorEnterAndLetObjectDie() {
>
> I can see it is convenient to just inject this test case in an existing test,
> but I'm not sure it is necessarily the right thing to do. Serviceability folk
> may have a stronger opinion.
Yeah, I was thinking the same. Maybe @sspitsyn or @plummercj could give
guidance here?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16783#discussion_r1405897185
PR Review Comment: https://git.openjdk.org/jdk/pull/16783#discussion_r1405894786
Re: RFR: 8320515: assert(monitor->object_peek() != nullptr) failed: Owned monitors should not have a dead object [v5]
On Mon, 27 Nov 2023 02:09:56 GMT, David Holmes wrote:
>> Stefan Karlsson has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Split test and use othervm
>
> test/hotspot/jtreg/serviceability/jvmti/GetOwnedMonitorInfo/GetOwnedMonitorInfoTest.java
> line 58:
>
>> 56: // Inject this situation into this test that performs other
>> 57: // GetOwnedMonitorInfo testing.
>> 58: Object obj = new Object() { public String toString() {return
>> "";} };
>
> Nit: the `toString` definition is not needed. This could just be `new
> Object();`, or `new Object() {};` if you want to introduce a nested class.
Thanks. Done.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16783#discussion_r1405891539
Re: RFR: 8320515: assert(monitor->object_peek() != nullptr) failed: Owned monitors should not have a dead object [v5]
On Mon, 27 Nov 2023 02:03:30 GMT, David Holmes wrote:
>> Stefan Karlsson has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Split test and use othervm
>
> test/hotspot/jtreg/runtime/Monitor/MonitorWithDeadObjectTest.java line 86:
>
>> 84: }
>> 85:
>> 86: private static void testDumpThreadsAfterDetachBeforeJoin() {
>
> The `AfterDetach` in the name is not accurate. If you don't join the new
> native thread then you are racing with its execution and you don't know when
> it will detach.
Thanks. I'll remove the "before join" test case.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16783#discussion_r1405887536
Re: RFR: 8320515: assert(monitor->object_peek() != nullptr) failed: Owned monitors should not have a dead object [v5]
On Mon, 27 Nov 2023 01:56:01 GMT, David Holmes wrote:
>> Stefan Karlsson has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Split test and use othervm
>
> test/hotspot/jtreg/runtime/Monitor/libMonitorWithDeadObjectTest.c line 161:
>
>> 159:
>> 160: if (pthread_attr_init() != 0) die("pthread_attr_init");
>> 161: if (pthread_create(, ,
>> create_monitor_with_dead_object_in_thread, NULL) != 0) die("pthread_create");
>
> You are not actually using the attr object to change anything. On AIX you may
> need to explicitly set the stack size.
OK. I copied this CommpleteExit.c. Should that be changed as well?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16783#discussion_r1405883743
Re: RFR: 8320515: assert(monitor->object_peek() != nullptr) failed: Owned monitors should not have a dead object [v5]
On Mon, 27 Nov 2023 01:52:29 GMT, David Holmes wrote: >> Stefan Karlsson has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Split test and use othervm > > test/hotspot/jtreg/runtime/Monitor/libMonitorWithDeadObjectTest.c line 47: > >> 45: >> 46: #define check(env, what, msg) \ >> 47: check_exception((env), (msg)); \ > > I'm not understanding why you have `check` and `check_exception` here nor why > you choose to use one versus the other. ?? Some JNI calls return something, for those I can use `check` which combines a null-check and an exception check. Some tests don't return anything, they can't null-check and can only perform an exception check. - PR Review Comment: https://git.openjdk.org/jdk/pull/16783#discussion_r1405874373
Re: RFR: 8320515: assert(monitor->object_peek() != nullptr) failed: Owned monitors should not have a dead object [v5]
On Mon, 27 Nov 2023 02:02:08 GMT, David Holmes wrote:
>> test/hotspot/jtreg/runtime/Monitor/MonitorWithDeadObjectTest.java line 75:
>>
>>> 73: private static void testDetachThread() {
>>> 74: // Create an ObjectMonitor with a dead object from an
>>> 75: // attached thread.
>>
>> Unclear what the "Detach" in the method name has to do with anything. ??
>
> And why add these wrapper methods that simply call one other method?
> Unclear what the "Detach" in the method name has to do with anything. ??
This test case provokes the assert we hit when the monitor is visited inside
DetachCurrentThread. I updated the comment to state that.
> And why add these wrapper methods that simply call one other method?
Because I find this structure more cohesive and better structured. I have four
functions representing the four tests. The fact that two of them in turn only
call one function is an implementation detail. I don't want to push the call to
`createMonitorWithDeadObject` down into the main function, because then I also
have to move the comment there, and suddenly the main function becomes more
then just a super simple dispatch function.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16783#discussion_r1405871913
Re: RFR: 8320515: assert(monitor->object_peek() != nullptr) failed: Owned monitors should not have a dead object [v5]
On Mon, 27 Nov 2023 01:43:07 GMT, David Holmes wrote: >> Stefan Karlsson has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Split test and use othervm > > test/hotspot/jtreg/runtime/Monitor/MonitorWithDeadObjectTest.java line 26: > >> 24: >> 25: /* >> 26: * @summary This test checks that ObjectMonitors with dead objects don't > > Please add `@bug` line Done - PR Review Comment: https://git.openjdk.org/jdk/pull/16783#discussion_r1405858379
Re: RFR: 8320515: assert(monitor->object_peek() != nullptr) failed: Owned monitors should not have a dead object [v5]
> In the rewrites made for: > [JDK-8318757](https://bugs.openjdk.org/browse/JDK-8318757) `VM_ThreadDump > asserts in interleaved ObjectMonitor::deflate_monitor calls` > > I removed the filtering of *owned ObjectMonitors with dead objects*. The > reasoning was that you should never have an owned ObjectMonitor with a dead > object. I added an assert to check this assumption. It turns out that the > assumption was wrong *if* you use JNI to call MonitorEnter and then remove > all references to the locked object. > > The provided tests provoke this assert form: > * the JNI thread detach code > * thread dumping with locked monitors, and > * the JVMTI GetOwnedMonitorInfo API. > > While investigating this we've found that the thread detach code becomes more > correct when this filter was removed. Previously, the locked monitors never > got unlocked because the ObjectMonitor iterator never exposed these monitors > to the JNI detach code that unlocks the thread's monitors. That bug caused an > ObjectMonitor leak. So, for this case I'm leaving these ObjectMonitors > unfiltered so that we don't reintroduce the leak. > > The thread dumping case doesn't tolerate ObjectMonitor with dead objects, so > I'm filtering those in the closure that collects ObjectMonitor. Side note: We > have discussions about ways to completely rewrite this by letting each thread > have thread-local information about JNI held locks. If we have this we could > probably throw away the entire ObjectMonitorDump hashtable, and its walk of > the `_in_use_list.`. > > For GetOwnedMonitorInfo it is unclear if we should expose these weird > ObjectMonitor. If we do, then the users can detect that a thread holds a lock > with a dead object, and the code will return NULL as one of the "owned > monitors" returned. I don't think that's a good idea, so I'm filtering out > these ObjectMonitor for those calls. > > Test: the written tests with and without the fix. Tier1-Tier3, so far. Stefan Karlsson has updated the pull request incrementally with one additional commit since the last revision: Split test and use othervm - Changes: - all: https://git.openjdk.org/jdk/pull/16783/files - new: https://git.openjdk.org/jdk/pull/16783/files/9521b26d..bad51926 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16783=04 - incr: https://webrevs.openjdk.org/?repo=jdk=16783=03-04 Stats: 29 lines in 1 file changed: 23 ins; 1 del; 5 mod Patch: https://git.openjdk.org/jdk/pull/16783.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16783/head:pull/16783 PR: https://git.openjdk.org/jdk/pull/16783
Re: RFR: 8314502: Change the comparator taking version of GrowableArray::find to be a template method [v3]
On Fri, 24 Nov 2023 07:59:51 GMT, Afshin Zafari wrote: > > @afshin-zafari I'm happy to hear the code is now cleaner and nicer, but > > AFAICS this new version of the code has only had a single review. It should > > really have been re-reviewed by at least one of the earlier reviewers. > > Thanks. > > Oh, really sorry. That's obviously my mistake. Your are right, I should have > waited for others. What to do now? I think the other reviewers can give their review feedback then you create a new RFE to fix whatever they'd like to get fixed. - PR Comment: https://git.openjdk.org/jdk/pull/15418#issuecomment-1825313436
Re: RFR: 8320515: assert(monitor->object_peek() != nullptr) failed: Owned monitors should not have a dead object [v4]
On Fri, 24 Nov 2023 06:43:35 GMT, David Holmes wrote: >> I provoked test failures for all paths I filtered. If I remove this check >> and run: >> >> make -C ../build/fastdebug test >> TEST=test/hotspot/jtreg/runtime/Monitor/IterateMonitorWithDeadObjectTest.java >> JTREG="JAVA_OPTIONS=-XX:+UseZGC" >> >> >> I hit this assert: >> >> # Internal Error >> (/home/stefank/git/jdk/open/src/hotspot/share/services/management.cpp:1274), >> pid=1546709, tid=1546754 >> # assert(object != nullptr) failed: must be a Java object >> ... >> V [libjvm.so+0x1330ce8] jmm_DumpThreads+0x1a48 (management.cpp:1274) >> j >> sun.management.ThreadImpl.dumpThreads0([JZZI)[Ljava/lang/management/ThreadInfo;+0 >> java.management@22-internal >> j >> sun.management.ThreadImpl.dumpAllThreads(ZZI)[Ljava/lang/management/ThreadInfo;+28 >> java.management@22-internal >> j >> sun.management.ThreadImpl.dumpAllThreads(ZZ)[Ljava/lang/management/ThreadInfo;+5 >> java.management@22-internal >> j IterateMonitorWithDeadObjectTest.dumpThreadsWithLockedMonitors()V+7 >> j IterateMonitorWithDeadObjectTest.main([Ljava/lang/String;)V+11 >> >> >> If I remove that assert I hit an NPE in the Java layer: >> >> java.lang.NullPointerException: Cannot invoke "Object.getClass()" because >> "lock" is null >> at >> java.management/java.lang.management.ThreadInfo.(ThreadInfo.java:172) >> at java.management/sun.management.ThreadImpl.dumpThreads0(Native Method) >> at >> java.management/sun.management.ThreadImpl.dumpAllThreads(ThreadImpl.java:518) >> at >> java.management/sun.management.ThreadImpl.dumpAllThreads(ThreadImpl.java:506) >> at >> IterateMonitorWithDeadObjectTest.dumpThreadsWithLockedMonitors(IterateMonitorWithDeadObjectTest.java:44) >> at >> IterateMonitorWithDeadObjectTest.main(IterateMonitorWithDeadObjectTest.java:66) >> at >> java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103) >> at java.base/java.lang.reflect.Method.invoke(Method.java:580) >> at >> com.sun.javatest.regtest.agent.MainActionHelper$AgentVMRunnable.run(MainActionHelper.java:333) >> at java.base/java.lang.Thread.run(Thread.java:1570) > > Thanks for that. Looks like JMM thread dump is different to VM Thread dump. > Okay we definitely need RFEs to look into how to handle this. Will you create the RFE? I'm not as convinced that this is something that needs to be fixed, so it would be better if you create the RFE with the proper motivation. - PR Review Comment: https://git.openjdk.org/jdk/pull/16783#discussion_r1404046328
Re: RFR: 8314502: Change the comparator taking version of GrowableArray::find to be a template method [v12]
On Thu, 23 Nov 2023 16:11:32 GMT, Afshin Zafari wrote:
>> The `find` method now is
>> ```C++
>> template
>> int find(T* token, bool f(T*, E)) const {
>> ...
>>
>> Any other functions which use this are also changed.
>> Local linux-x64-debug hotspot:tier1 passed. Mach5 tier1 build on linux and
>> Windows passed.
>
> Afshin Zafari has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - Merge remote-tracking branch 'upstream/pr/15418' into pr_15418
> - Suggested cleanups and tests
Marked as reviewed by stefank (Reviewer).
-
PR Review: https://git.openjdk.org/jdk/pull/15418#pullrequestreview-1746766388
Re: RFR: 8320515: assert(monitor->object_peek() != nullptr) failed: Owned monitors should not have a dead object [v3]
On Thu, 23 Nov 2023 11:52:38 GMT, Stefan Karlsson wrote: >> In the rewrites made for: >> [JDK-8318757](https://bugs.openjdk.org/browse/JDK-8318757) `VM_ThreadDump >> asserts in interleaved ObjectMonitor::deflate_monitor calls` >> >> I removed the filtering of *owned ObjectMonitors with dead objects*. The >> reasoning was that you should never have an owned ObjectMonitor with a dead >> object. I added an assert to check this assumption. It turns out that the >> assumption was wrong *if* you use JNI to call MonitorEnter and then remove >> all references to the locked object. >> >> The provided tests provoke this assert form: >> * the JNI thread detach code >> * thread dumping with locked monitors, and >> * the JVMTI GetOwnedMonitorInfo API. >> >> While investigating this we've found that the thread detach code becomes >> more correct when this filter was removed. Previously, the locked monitors >> never got unlocked because the ObjectMonitor iterator never exposed these >> monitors to the JNI detach code that unlocks the thread's monitors. That bug >> caused an ObjectMonitor leak. So, for this case I'm leaving these >> ObjectMonitors unfiltered so that we don't reintroduce the leak. >> >> The thread dumping case doesn't tolerate ObjectMonitor with dead objects, so >> I'm filtering those in the closure that collects ObjectMonitor. Side note: >> We have discussions about ways to completely rewrite this by letting each >> thread have thread-local information about JNI held locks. If we have this >> we could probably throw away the entire ObjectMonitorDump hashtable, and its >> walk of the `_in_use_list.`. >> >> For GetOwnedMonitorInfo it is unclear if we should expose these weird >> ObjectMonitor. If we do, then the users can detect that a thread holds a >> lock with a dead object, and the code will return NULL as one of the "owned >> monitors" returned. I don't think that's a good idea, so I'm filtering out >> these ObjectMonitor for those calls. >> >> Test: the written tests with and without the fix. Tier1-Tier3, so far. > > Stefan Karlsson has updated the pull request incrementally with one > additional commit since the last revision: > > Tweak test comments I reworked the MonitorWithDeadObject tests so that they all run in one class loader. Jtreg loads the tests in different class loaders, which causes problems because it's only allowed to load a library from *one* class loader. - PR Comment: https://git.openjdk.org/jdk/pull/16783#issuecomment-1824679121
Re: RFR: 8320515: assert(monitor->object_peek() != nullptr) failed: Owned monitors should not have a dead object [v3]
On Thu, 23 Nov 2023 14:17:18 GMT, Magnus Ihse Bursie wrote: >> Stefan Karlsson has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Tweak test comments > > Build changes are trivially fine. Thanks @magicus. I'm removing the build label now. - PR Comment: https://git.openjdk.org/jdk/pull/16783#issuecomment-1824675404
Re: RFR: 8320515: assert(monitor->object_peek() != nullptr) failed: Owned monitors should not have a dead object [v4]
> In the rewrites made for: > [JDK-8318757](https://bugs.openjdk.org/browse/JDK-8318757) `VM_ThreadDump > asserts in interleaved ObjectMonitor::deflate_monitor calls` > > I removed the filtering of *owned ObjectMonitors with dead objects*. The > reasoning was that you should never have an owned ObjectMonitor with a dead > object. I added an assert to check this assumption. It turns out that the > assumption was wrong *if* you use JNI to call MonitorEnter and then remove > all references to the locked object. > > The provided tests provoke this assert form: > * the JNI thread detach code > * thread dumping with locked monitors, and > * the JVMTI GetOwnedMonitorInfo API. > > While investigating this we've found that the thread detach code becomes more > correct when this filter was removed. Previously, the locked monitors never > got unlocked because the ObjectMonitor iterator never exposed these monitors > to the JNI detach code that unlocks the thread's monitors. That bug caused an > ObjectMonitor leak. So, for this case I'm leaving these ObjectMonitors > unfiltered so that we don't reintroduce the leak. > > The thread dumping case doesn't tolerate ObjectMonitor with dead objects, so > I'm filtering those in the closure that collects ObjectMonitor. Side note: We > have discussions about ways to completely rewrite this by letting each thread > have thread-local information about JNI held locks. If we have this we could > probably throw away the entire ObjectMonitorDump hashtable, and its walk of > the `_in_use_list.`. > > For GetOwnedMonitorInfo it is unclear if we should expose these weird > ObjectMonitor. If we do, then the users can detect that a thread holds a lock > with a dead object, and the code will return NULL as one of the "owned > monitors" returned. I don't think that's a good idea, so I'm filtering out > these ObjectMonitor for those calls. > > Test: the written tests with and without the fix. Tier1-Tier3, so far. Stefan Karlsson has updated the pull request incrementally with one additional commit since the last revision: Rewrite tests to prevent problem with native libs - Changes: - all: https://git.openjdk.org/jdk/pull/16783/files - new: https://git.openjdk.org/jdk/pull/16783/files/3239b822..9521b26d Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16783=03 - incr: https://webrevs.openjdk.org/?repo=jdk=16783=02-03 Stats: 299 lines in 7 files changed: 99 ins; 194 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/16783.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16783/head:pull/16783 PR: https://git.openjdk.org/jdk/pull/16783
Re: RFR: 8320515: assert(monitor->object_peek() != nullptr) failed: Owned monitors should not have a dead object [v3]
On Thu, 23 Nov 2023 11:52:38 GMT, Stefan Karlsson wrote: >> In the rewrites made for: >> [JDK-8318757](https://bugs.openjdk.org/browse/JDK-8318757) `VM_ThreadDump >> asserts in interleaved ObjectMonitor::deflate_monitor calls` >> >> I removed the filtering of *owned ObjectMonitors with dead objects*. The >> reasoning was that you should never have an owned ObjectMonitor with a dead >> object. I added an assert to check this assumption. It turns out that the >> assumption was wrong *if* you use JNI to call MonitorEnter and then remove >> all references to the locked object. >> >> The provided tests provoke this assert form: >> * the JNI thread detach code >> * thread dumping with locked monitors, and >> * the JVMTI GetOwnedMonitorInfo API. >> >> While investigating this we've found that the thread detach code becomes >> more correct when this filter was removed. Previously, the locked monitors >> never got unlocked because the ObjectMonitor iterator never exposed these >> monitors to the JNI detach code that unlocks the thread's monitors. That bug >> caused an ObjectMonitor leak. So, for this case I'm leaving these >> ObjectMonitors unfiltered so that we don't reintroduce the leak. >> >> The thread dumping case doesn't tolerate ObjectMonitor with dead objects, so >> I'm filtering those in the closure that collects ObjectMonitor. Side note: >> We have discussions about ways to completely rewrite this by letting each >> thread have thread-local information about JNI held locks. If we have this >> we could probably throw away the entire ObjectMonitorDump hashtable, and its >> walk of the `_in_use_list.`. >> >> For GetOwnedMonitorInfo it is unclear if we should expose these weird >> ObjectMonitor. If we do, then the users can detect that a thread holds a >> lock with a dead object, and the code will return NULL as one of the "owned >> monitors" returned. I don't think that's a good idea, so I'm filtering out >> these ObjectMonitor for those calls. >> >> Test: the written tests with and without the fix. Tier1-Tier3, so far. > > Stefan Karlsson has updated the pull request incrementally with one > additional commit since the last revision: > > Tweak test comments GHA complains that the lib gets loaded into multiple class loaders. I need to figure out how to share code between the tests without so that I don't have to duplicate the code. - PR Comment: https://git.openjdk.org/jdk/pull/16783#issuecomment-1824494844
Re: RFR: 8314502: Change the comparator taking version of GrowableArray::find to be a template method [v10]
On Thu, 23 Nov 2023 13:32:23 GMT, Afshin Zafari wrote: >>> > Thanks for making this change. >>> > I'd like to suggest the following cleanups, some documentation, and a few >>> > tests: >>> > [20d4502](https://github.com/openjdk/jdk/commit/20d4502471ba396ae395512cfa3dab3f87555421) >>> > I think it might be easier to review by looking at the final diff: >>> > [master...stefank:jdk:pr_15418](https://github.com/openjdk/jdk/compare/master...stefank:jdk:pr_15418) >>> >>> One question: the `private static bool by_name(const char* name, PerfData* >>> pd);` is added to `PerfDataList` class but is never used. Is something >>> missing? >> >> That should have been removed when I added name_equals. > > @stefank, the changes are applied as suggested and ready for review. @afshin-zafari you toke my changes and then you made a few small whitespace changes that messed up the patch. I merged our two branches to correct those issues. Could you make sure to pull the changes from my branch (don't copy the changes): https://github.com/stefank/jdk/tree/pr_15418 - PR Comment: https://git.openjdk.org/jdk/pull/15418#issuecomment-1824474455
Re: RFR: 8314502: Change the comparator taking version of GrowableArray::find to be a template method [v10]
On Thu, 23 Nov 2023 11:00:32 GMT, Afshin Zafari wrote: > > Thanks for making this change. > > I'd like to suggest the following cleanups, some documentation, and a few > > tests: > > [20d4502](https://github.com/openjdk/jdk/commit/20d4502471ba396ae395512cfa3dab3f87555421) > > I think it might be easier to review by looking at the final diff: > > [master...stefank:jdk:pr_15418](https://github.com/openjdk/jdk/compare/master...stefank:jdk:pr_15418) > > One question: the `private static bool by_name(const char* name, PerfData* > pd);` is added to `PerfDataList` class but is never used. Is something > missing? That should have been removed when I added name_equals. - PR Comment: https://git.openjdk.org/jdk/pull/15418#issuecomment-1824302304
Re: RFR: 8320515: assert(monitor->object_peek() != nullptr) failed: Owned monitors should not have a dead object [v3]
> In the rewrites made for: > [JDK-8318757](https://bugs.openjdk.org/browse/JDK-8318757) `VM_ThreadDump > asserts in interleaved ObjectMonitor::deflate_monitor calls` > > I removed the filtering of *owned ObjectMonitors with dead objects*. The > reasoning was that you should never have an owned ObjectMonitor with a dead > object. I added an assert to check this assumption. It turns out that the > assumption was wrong *if* you use JNI to call MonitorEnter and then remove > all references to the locked object. > > The provided tests provoke this assert form: > * the JNI thread detach code > * thread dumping with locked monitors, and > * the JVMTI GetOwnedMonitorInfo API. > > While investigating this we've found that the thread detach code becomes more > correct when this filter was removed. Previously, the locked monitors never > got unlocked because the ObjectMonitor iterator never exposed these monitors > to the JNI detach code that unlocks the thread's monitors. That bug caused an > ObjectMonitor leak. So, for this case I'm leaving these ObjectMonitors > unfiltered so that we don't reintroduce the leak. > > The thread dumping case doesn't tolerate ObjectMonitor with dead objects, so > I'm filtering those in the closure that collects ObjectMonitor. Side note: We > have discussions about ways to completely rewrite this by letting each thread > have thread-local information about JNI held locks. If we have this we could > probably throw away the entire ObjectMonitorDump hashtable, and its walk of > the `_in_use_list.`. > > For GetOwnedMonitorInfo it is unclear if we should expose these weird > ObjectMonitor. If we do, then the users can detect that a thread holds a lock > with a dead object, and the code will return NULL as one of the "owned > monitors" returned. I don't think that's a good idea, so I'm filtering out > these ObjectMonitor for those calls. > > Test: the written tests with and without the fix. Tier1-Tier3, so far. Stefan Karlsson has updated the pull request incrementally with one additional commit since the last revision: Tweak test comments - Changes: - all: https://git.openjdk.org/jdk/pull/16783/files - new: https://git.openjdk.org/jdk/pull/16783/files/4b0976a8..3239b822 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16783=02 - incr: https://webrevs.openjdk.org/?repo=jdk=16783=01-02 Stats: 6 lines in 1 file changed: 0 ins; 0 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/16783.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16783/head:pull/16783 PR: https://git.openjdk.org/jdk/pull/16783
Re: RFR: 8320515: assert(monitor->object_peek() != nullptr) failed: Owned monitors should not have a dead object
On Thu, 23 Nov 2023 00:48:19 GMT, David Holmes wrote: > > Previously, the locked monitors never got unlocked because the > > ObjectMonitor iterator never exposed these monitors to the JNI detach code > > I had not realized that. It explains some confusion in a separate issue I had > been looking into! It is important that these monitors are exposed and > unlocked at detach time, otherwise it also messes up the `held_monitor_count`. > I think I managed to reproduce that while reworking the tests and temporarily reintroducing the bug. > > The thread dumping case doesn't tolerate ObjectMonitor with dead objects, > > so I'm filtering those in the closure > > I think we may need to make that code tolerate the absence of an object. IDK. That's another thing that would be good to discuss in a separate RFE. > > > For GetOwnedMonitorInfo it is unclear if we should expose these weird > > ObjectMonitor. > > I think we probably should expose this to be accurate, but I think this needs > investigation on the JVMTI side to ensure that the null entry is tolerated > okay. So a separate RFE to handle this would be fine. This will lead to NPE in the management code, but it even if we fixed that, it could potentially causing NPEs in user code. So, yes, I wouldn't mind if someone wanted to investigate this as a separate RFE. > > Thanks - PR Comment: https://git.openjdk.org/jdk/pull/16783#issuecomment-1824286615
Re: RFR: 8320515: assert(monitor->object_peek() != nullptr) failed: Owned monitors should not have a dead object [v2]
On Thu, 23 Nov 2023 02:10:41 GMT, David Holmes wrote:
>> Stefan Karlsson has updated the pull request incrementally with two
>> additional commits since the last revision:
>>
>> - Tweaked comment in test
>> - Rewrite tests
>
> test/hotspot/jtreg/runtime/Monitor/libIterateMonitorWithDeadObjectTest.c line
> 43:
>
>> 41: static jobject create_object(JNIEnv* env) {
>> 42: jclass clazz = (*env)->FindClass(env, "java/lang/Object");
>> 43: if (clazz == 0) die("No class");
>
> The `die` method is for errors with system calls. It won't show useful
> information for JNI calls that leave exceptions pending.
I've added better checks.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16783#discussion_r1403263549
Re: RFR: 8320515: assert(monitor->object_peek() != nullptr) failed: Owned monitors should not have a dead object [v2]
> In the rewrites made for: > [JDK-8318757](https://bugs.openjdk.org/browse/JDK-8318757) `VM_ThreadDump > asserts in interleaved ObjectMonitor::deflate_monitor calls` > > I removed the filtering of *owned ObjectMonitors with dead objects*. The > reasoning was that you should never have an owned ObjectMonitor with a dead > object. I added an assert to check this assumption. It turns out that the > assumption was wrong *if* you use JNI to call MonitorEnter and then remove > all references to the locked object. > > The provided tests provoke this assert form: > * the JNI thread detach code > * thread dumping with locked monitors, and > * the JVMTI GetOwnedMonitorInfo API. > > While investigating this we've found that the thread detach code becomes more > correct when this filter was removed. Previously, the locked monitors never > got unlocked because the ObjectMonitor iterator never exposed these monitors > to the JNI detach code that unlocks the thread's monitors. That bug caused an > ObjectMonitor leak. So, for this case I'm leaving these ObjectMonitors > unfiltered so that we don't reintroduce the leak. > > The thread dumping case doesn't tolerate ObjectMonitor with dead objects, so > I'm filtering those in the closure that collects ObjectMonitor. Side note: We > have discussions about ways to completely rewrite this by letting each thread > have thread-local information about JNI held locks. If we have this we could > probably throw away the entire ObjectMonitorDump hashtable, and its walk of > the `_in_use_list.`. > > For GetOwnedMonitorInfo it is unclear if we should expose these weird > ObjectMonitor. If we do, then the users can detect that a thread holds a lock > with a dead object, and the code will return NULL as one of the "owned > monitors" returned. I don't think that's a good idea, so I'm filtering out > these ObjectMonitor for those calls. > > Test: the written tests with and without the fix. Tier1-Tier3, so far. Stefan Karlsson has updated the pull request incrementally with two additional commits since the last revision: - Tweaked comment in test - Rewrite tests - Changes: - all: https://git.openjdk.org/jdk/pull/16783/files - new: https://git.openjdk.org/jdk/pull/16783/files/b1dd4cf8..4b0976a8 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16783=01 - incr: https://webrevs.openjdk.org/?repo=jdk=16783=00-01 Stats: 605 lines in 9 files changed: 388 ins; 214 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/16783.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16783/head:pull/16783 PR: https://git.openjdk.org/jdk/pull/16783
Re: RFR: 8320515: assert(monitor->object_peek() != nullptr) failed: Owned monitors should not have a dead object
On Thu, 23 Nov 2023 02:07:59 GMT, David Holmes wrote:
>> In the rewrites made for:
>> [JDK-8318757](https://bugs.openjdk.org/browse/JDK-8318757) `VM_ThreadDump
>> asserts in interleaved ObjectMonitor::deflate_monitor calls`
>>
>> I removed the filtering of *owned ObjectMonitors with dead objects*. The
>> reasoning was that you should never have an owned ObjectMonitor with a dead
>> object. I added an assert to check this assumption. It turns out that the
>> assumption was wrong *if* you use JNI to call MonitorEnter and then remove
>> all references to the locked object.
>>
>> The provided tests provoke this assert form:
>> * the JNI thread detach code
>> * thread dumping with locked monitors, and
>> * the JVMTI GetOwnedMonitorInfo API.
>>
>> While investigating this we've found that the thread detach code becomes
>> more correct when this filter was removed. Previously, the locked monitors
>> never got unlocked because the ObjectMonitor iterator never exposed these
>> monitors to the JNI detach code that unlocks the thread's monitors. That bug
>> caused an ObjectMonitor leak. So, for this case I'm leaving these
>> ObjectMonitors unfiltered so that we don't reintroduce the leak.
>>
>> The thread dumping case doesn't tolerate ObjectMonitor with dead objects, so
>> I'm filtering those in the closure that collects ObjectMonitor. Side note:
>> We have discussions about ways to completely rewrite this by letting each
>> thread have thread-local information about JNI held locks. If we have this
>> we could probably throw away the entire ObjectMonitorDump hashtable, and its
>> walk of the `_in_use_list.`.
>>
>> For GetOwnedMonitorInfo it is unclear if we should expose these weird
>> ObjectMonitor. If we do, then the users can detect that a thread holds a
>> lock with a dead object, and the code will return NULL as one of the "owned
>> monitors" returned. I don't think that's a good idea, so I'm filtering out
>> these ObjectMonitor for those calls.
>>
>> Test: the written tests with and without the fix. Tier1-Tier3, so far.
>
> test/hotspot/jtreg/serviceability/jvmti/GetOwnedMonitorInfo/GetOwnedMonitorInfoTest.java
> line 59:
>
>> 57: // GetOwnedMonitorInfo testing.
>> 58: Object obj = new Object() { public String toString() {return
>> "";} };
>> 59: jniMonitorEnter(obj);
>
> I would add a check for `Thread.holdsLock(obj);` after this just to be sure
> it worked.
Done
> test/hotspot/jtreg/serviceability/jvmti/GetOwnedMonitorInfo/GetOwnedMonitorInfoTest.java
> line 61:
>
>> 59: jniMonitorEnter(obj);
>> 60: obj = null;
>> 61: System.gc();
>
> Again one gc() is generally not sufficient.
>
> How can this test tell that the object in the monitor was actually cleared? I
> think `monitorinflation` logging may be the only way to tell.
Yes, probably. I've been looking at the `monitorinflation` logging to very that
it gets cleared. I think it would be messy to try to get this test to also
start to parse logs.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16783#discussion_r1403245976
PR Review Comment: https://git.openjdk.org/jdk/pull/16783#discussion_r1403244666
Re: RFR: 8320515: assert(monitor->object_peek() != nullptr) failed: Owned monitors should not have a dead object
On Thu, 23 Nov 2023 00:48:19 GMT, David Holmes wrote: >> In the rewrites made for: >> [JDK-8318757](https://bugs.openjdk.org/browse/JDK-8318757) `VM_ThreadDump >> asserts in interleaved ObjectMonitor::deflate_monitor calls` >> >> I removed the filtering of *owned ObjectMonitors with dead objects*. The >> reasoning was that you should never have an owned ObjectMonitor with a dead >> object. I added an assert to check this assumption. It turns out that the >> assumption was wrong *if* you use JNI to call MonitorEnter and then remove >> all references to the locked object. >> >> The provided tests provoke this assert form: >> * the JNI thread detach code >> * thread dumping with locked monitors, and >> * the JVMTI GetOwnedMonitorInfo API. >> >> While investigating this we've found that the thread detach code becomes >> more correct when this filter was removed. Previously, the locked monitors >> never got unlocked because the ObjectMonitor iterator never exposed these >> monitors to the JNI detach code that unlocks the thread's monitors. That bug >> caused an ObjectMonitor leak. So, for this case I'm leaving these >> ObjectMonitors unfiltered so that we don't reintroduce the leak. >> >> The thread dumping case doesn't tolerate ObjectMonitor with dead objects, so >> I'm filtering those in the closure that collects ObjectMonitor. Side note: >> We have discussions about ways to completely rewrite this by letting each >> thread have thread-local information about JNI held locks. If we have this >> we could probably throw away the entire ObjectMonitorDump hashtable, and its >> walk of the `_in_use_list.`. >> >> For GetOwnedMonitorInfo it is unclear if we should expose these weird >> ObjectMonitor. If we do, then the users can detect that a thread holds a >> lock with a dead object, and the code will return NULL as one of the "owned >> monitors" returned. I don't think that's a good idea, so I'm filtering out >> these ObjectMonitor for those calls. >> >> Test: the written tests with and without the fix. Tier1-Tier3, so far. > >> Previously, the locked monitors never got unlocked because the >> ObjectMonitor iterator never exposed these monitors to the JNI detach code > > I had not realized that. It explains some confusion in a separate issue I > had been looking into! It is important that these monitors are exposed and > unlocked at detach time, otherwise it also messes up the `held_monitor_count`. > >> The thread dumping case doesn't tolerate ObjectMonitor with dead objects, so >> I'm filtering those in the closure > > I think we may need to make that code tolerate the absence of an object. > >> For GetOwnedMonitorInfo it is unclear if we should expose these weird >> ObjectMonitor. > > I think we probably should expose this to be accurate, but I think this needs > investigation on the JVMTI side to ensure that the null entry is tolerated > okay. So a separate RFE to handle this would be fine. > > Thanks While addressing @dholmes-ora's comments about the tests I found that the JNI implementation was incorrect and caused a failure, that seems to prevent the thread detach, and then the Java layer thread dumping caught the thread dumping bug. I'm going to see if I can split the various test cases so that they can be tested individually. Don't look too closely at the current test until it has been updated. > test/hotspot/jtreg/runtime/Monitor/libIterateMonitorWithDeadObjectTest.c line > 94: > >> 92: >> 93: // Let the GC clear the weak reference to the object. >> 94: system_gc(env); > > AFAIK there is no guarantee that one call to `System.gc()` will suffice to > clear the weakRef. We tend use a loop with a few iterations in other tests, > or use a WhiteBox method to achieve it. In my testing I used the finalizer to > observe that the objects had been finalized but even then, and with a loop, I > did not always see them collected with G1. ZGC will clear the weak reference. G1 clears the weak reference, except for in a few specific situations. I've verified that G1 clears this weak reference, so I don't think it would be worth making this test longer or more complicated. > test/hotspot/jtreg/runtime/Monitor/libIterateMonitorWithDeadObjectTest.c line > 104: > >> 102: // source of at least two bugs: >> 103: // - When the object reference in the monitor was made weak, the code >> 104: // didn't unlock the monitor, leaving it lingering in the system. > > Suggestion: > > // - When the object reference in the monitor was cleared, the monitor > // iterator code would skip it, preventing it from being unlocked when > // the owner thread detached, leaving it lingering in the system. > > the original made it sound to me like the code that cleared the reference > (i.e. the GC) was expected to do the unlocking. Thanks. - PR Comment: https://git.openjdk.org/jdk/pull/16783#issuecomment-1824116115 PR Review Comment:
Re: RFR: 8320515: assert(monitor->object_peek() != nullptr) failed: Owned monitors should not have a dead object
On Thu, 23 Nov 2023 01:38:57 GMT, David Holmes wrote:
>> In the rewrites made for:
>> [JDK-8318757](https://bugs.openjdk.org/browse/JDK-8318757) `VM_ThreadDump
>> asserts in interleaved ObjectMonitor::deflate_monitor calls`
>>
>> I removed the filtering of *owned ObjectMonitors with dead objects*. The
>> reasoning was that you should never have an owned ObjectMonitor with a dead
>> object. I added an assert to check this assumption. It turns out that the
>> assumption was wrong *if* you use JNI to call MonitorEnter and then remove
>> all references to the locked object.
>>
>> The provided tests provoke this assert form:
>> * the JNI thread detach code
>> * thread dumping with locked monitors, and
>> * the JVMTI GetOwnedMonitorInfo API.
>>
>> While investigating this we've found that the thread detach code becomes
>> more correct when this filter was removed. Previously, the locked monitors
>> never got unlocked because the ObjectMonitor iterator never exposed these
>> monitors to the JNI detach code that unlocks the thread's monitors. That bug
>> caused an ObjectMonitor leak. So, for this case I'm leaving these
>> ObjectMonitors unfiltered so that we don't reintroduce the leak.
>>
>> The thread dumping case doesn't tolerate ObjectMonitor with dead objects, so
>> I'm filtering those in the closure that collects ObjectMonitor. Side note:
>> We have discussions about ways to completely rewrite this by letting each
>> thread have thread-local information about JNI held locks. If we have this
>> we could probably throw away the entire ObjectMonitorDump hashtable, and its
>> walk of the `_in_use_list.`.
>>
>> For GetOwnedMonitorInfo it is unclear if we should expose these weird
>> ObjectMonitor. If we do, then the users can detect that a thread holds a
>> lock with a dead object, and the code will return NULL as one of the "owned
>> monitors" returned. I don't think that's a good idea, so I'm filtering out
>> these ObjectMonitor for those calls.
>>
>> Test: the written tests with and without the fix. Tier1-Tier3, so far.
>
> test/hotspot/jtreg/runtime/Monitor/IterateMonitorWithDeadObjectTest.java line
> 40:
>
>> 38: public class IterateMonitorWithDeadObjectTest {
>> 39: public static native void runTestAndDetachThread();
>> 40: public static native void joinTestThread();
>
> I don't think this form of the test needs to separate out the
> `pthread_join()`, it can just be done in `runTestAndDetachThread` AFAICS. I
> originally split it out to allow the Java code to do the GC while the native
> thread was sleeping prior to detaching.
All this is left-overs that I thought I had removed. I'm removing this.
> test/hotspot/jtreg/runtime/Monitor/IterateMonitorWithDeadObjectTest.java line
> 57:
>
>> 55: // - Drop the last reference to the object
>> 56: // - GC to clear the weak reference to the object in the monitor
>> 57: // - Detach the thread - provoke previous bug
>
> It also does a thread dump while the lock is held
Updated
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16783#discussion_r1403082283
PR Review Comment: https://git.openjdk.org/jdk/pull/16783#discussion_r1403082528
Re: RFR: 8320515: assert(monitor->object_peek() != nullptr) failed: Owned monitors should not have a dead object
On Thu, 23 Nov 2023 01:29:24 GMT, David Holmes wrote: >> In the rewrites made for: >> [JDK-8318757](https://bugs.openjdk.org/browse/JDK-8318757) `VM_ThreadDump >> asserts in interleaved ObjectMonitor::deflate_monitor calls` >> >> I removed the filtering of *owned ObjectMonitors with dead objects*. The >> reasoning was that you should never have an owned ObjectMonitor with a dead >> object. I added an assert to check this assumption. It turns out that the >> assumption was wrong *if* you use JNI to call MonitorEnter and then remove >> all references to the locked object. >> >> The provided tests provoke this assert form: >> * the JNI thread detach code >> * thread dumping with locked monitors, and >> * the JVMTI GetOwnedMonitorInfo API. >> >> While investigating this we've found that the thread detach code becomes >> more correct when this filter was removed. Previously, the locked monitors >> never got unlocked because the ObjectMonitor iterator never exposed these >> monitors to the JNI detach code that unlocks the thread's monitors. That bug >> caused an ObjectMonitor leak. So, for this case I'm leaving these >> ObjectMonitors unfiltered so that we don't reintroduce the leak. >> >> The thread dumping case doesn't tolerate ObjectMonitor with dead objects, so >> I'm filtering those in the closure that collects ObjectMonitor. Side note: >> We have discussions about ways to completely rewrite this by letting each >> thread have thread-local information about JNI held locks. If we have this >> we could probably throw away the entire ObjectMonitorDump hashtable, and its >> walk of the `_in_use_list.`. >> >> For GetOwnedMonitorInfo it is unclear if we should expose these weird >> ObjectMonitor. If we do, then the users can detect that a thread holds a >> lock with a dead object, and the code will return NULL as one of the "owned >> monitors" returned. I don't think that's a good idea, so I'm filtering out >> these ObjectMonitor for those calls. >> >> Test: the written tests with and without the fix. Tier1-Tier3, so far. > > test/hotspot/jtreg/runtime/Monitor/IterateMonitorWithDeadObjectTest.java line > 29: > >> 27: * @summary This locks a monitor, GCs the object, and iterate and perform >> 28: * various iteration and operations over this monitor. >> 29: * @requires os.family == "linux" > > I know the test this was copied from had this but I'm not sure it is actually > a necessary restriction - any Posix platform should work. Though maybe perror > is linux only ... I couldn't find any test filtering for posix, but some tests have: @requires os.family != "windows" That seems to work on my Mac. > test/hotspot/jtreg/runtime/Monitor/IterateMonitorWithDeadObjectTest.java line > 31: > >> 29: * @requires os.family == "linux" >> 30: * @library /testlibrary /test/lib >> 31: * @build IterateMonitorWithDeadObjectTest > > You don't need an explicit `@build` step Sure. This was just copied brought over from your original reproducer. > test/hotspot/jtreg/runtime/Monitor/IterateMonitorWithDeadObjectTest.java line > 66: > >> 64: // dead object. The thread dumping code didn't tolerate such a >> monitor, >> 65: // so run a thread dump and make sure that it doesn't >> crash/assert. >> 66: dumpThreadsWithLockedMonitors(); > > But you've already detached the thread so there is no locked monitor any > longer. And `runTestAndDetachThread()` also did a thread dump. Right. This is left-overs from my earlier attempt. I'll remove this. - PR Review Comment: https://git.openjdk.org/jdk/pull/16783#discussion_r1403077397 PR Review Comment: https://git.openjdk.org/jdk/pull/16783#discussion_r1403077988 PR Review Comment: https://git.openjdk.org/jdk/pull/16783#discussion_r1403078851
Re: RFR: 8320515: assert(monitor->object_peek() != nullptr) failed: Owned monitors should not have a dead object
On Thu, 23 Nov 2023 01:28:32 GMT, David Holmes wrote: >> In the rewrites made for: >> [JDK-8318757](https://bugs.openjdk.org/browse/JDK-8318757) `VM_ThreadDump >> asserts in interleaved ObjectMonitor::deflate_monitor calls` >> >> I removed the filtering of *owned ObjectMonitors with dead objects*. The >> reasoning was that you should never have an owned ObjectMonitor with a dead >> object. I added an assert to check this assumption. It turns out that the >> assumption was wrong *if* you use JNI to call MonitorEnter and then remove >> all references to the locked object. >> >> The provided tests provoke this assert form: >> * the JNI thread detach code >> * thread dumping with locked monitors, and >> * the JVMTI GetOwnedMonitorInfo API. >> >> While investigating this we've found that the thread detach code becomes >> more correct when this filter was removed. Previously, the locked monitors >> never got unlocked because the ObjectMonitor iterator never exposed these >> monitors to the JNI detach code that unlocks the thread's monitors. That bug >> caused an ObjectMonitor leak. So, for this case I'm leaving these >> ObjectMonitors unfiltered so that we don't reintroduce the leak. >> >> The thread dumping case doesn't tolerate ObjectMonitor with dead objects, so >> I'm filtering those in the closure that collects ObjectMonitor. Side note: >> We have discussions about ways to completely rewrite this by letting each >> thread have thread-local information about JNI held locks. If we have this >> we could probably throw away the entire ObjectMonitorDump hashtable, and its >> walk of the `_in_use_list.`. >> >> For GetOwnedMonitorInfo it is unclear if we should expose these weird >> ObjectMonitor. If we do, then the users can detect that a thread holds a >> lock with a dead object, and the code will return NULL as one of the "owned >> monitors" returned. I don't think that's a good idea, so I'm filtering out >> these ObjectMonitor for those calls. >> >> Test: the written tests with and without the fix. Tier1-Tier3, so far. > > test/hotspot/jtreg/runtime/Monitor/IterateMonitorWithDeadObjectTest.java line > 28: > >> 26: * @test IterateMonitorWithDeadObjectTest >> 27: * @summary This locks a monitor, GCs the object, and iterate and perform >> 28: * various iteration and operations over this monitor. > > This doesn't read right with "iterate" and "iteration". Not sure exactly what > you were trying to say. I agree. I'll reword that. - PR Review Comment: https://git.openjdk.org/jdk/pull/16783#discussion_r1403073457
Re: RFR: 8320515: assert(monitor->object_peek() != nullptr) failed: Owned monitors should not have a dead object
On Thu, 23 Nov 2023 01:27:24 GMT, David Holmes wrote: >> In the rewrites made for: >> [JDK-8318757](https://bugs.openjdk.org/browse/JDK-8318757) `VM_ThreadDump >> asserts in interleaved ObjectMonitor::deflate_monitor calls` >> >> I removed the filtering of *owned ObjectMonitors with dead objects*. The >> reasoning was that you should never have an owned ObjectMonitor with a dead >> object. I added an assert to check this assumption. It turns out that the >> assumption was wrong *if* you use JNI to call MonitorEnter and then remove >> all references to the locked object. >> >> The provided tests provoke this assert form: >> * the JNI thread detach code >> * thread dumping with locked monitors, and >> * the JVMTI GetOwnedMonitorInfo API. >> >> While investigating this we've found that the thread detach code becomes >> more correct when this filter was removed. Previously, the locked monitors >> never got unlocked because the ObjectMonitor iterator never exposed these >> monitors to the JNI detach code that unlocks the thread's monitors. That bug >> caused an ObjectMonitor leak. So, for this case I'm leaving these >> ObjectMonitors unfiltered so that we don't reintroduce the leak. >> >> The thread dumping case doesn't tolerate ObjectMonitor with dead objects, so >> I'm filtering those in the closure that collects ObjectMonitor. Side note: >> We have discussions about ways to completely rewrite this by letting each >> thread have thread-local information about JNI held locks. If we have this >> we could probably throw away the entire ObjectMonitorDump hashtable, and its >> walk of the `_in_use_list.`. >> >> For GetOwnedMonitorInfo it is unclear if we should expose these weird >> ObjectMonitor. If we do, then the users can detect that a thread holds a >> lock with a dead object, and the code will return NULL as one of the "owned >> monitors" returned. I don't think that's a good idea, so I'm filtering out >> these ObjectMonitor for those calls. >> >> Test: the written tests with and without the fix. Tier1-Tier3, so far. > > src/hotspot/share/runtime/vmOperations.cpp line 354: > >> 352: // alive. Filter out monitors with dead objects. >> 353: return; >> 354: } > > I don't think we need to do this, but even without this filtering I ran a > number of tests and was unable to demonstrate any problem. The JNI locked > monitor seems to be "invisible" to the frame that locked it and so the thread > dump never encounters it. Were you able to provoke a failure here or is this > defensive programming? I provoked test failures for all paths I filtered. If I remove this check and run: make -C ../build/fastdebug test TEST=test/hotspot/jtreg/runtime/Monitor/IterateMonitorWithDeadObjectTest.java JTREG="JAVA_OPTIONS=-XX:+UseZGC" I hit this assert: # Internal Error (/home/stefank/git/jdk/open/src/hotspot/share/services/management.cpp:1274), pid=1546709, tid=1546754 # assert(object != nullptr) failed: must be a Java object ... V [libjvm.so+0x1330ce8] jmm_DumpThreads+0x1a48 (management.cpp:1274) j sun.management.ThreadImpl.dumpThreads0([JZZI)[Ljava/lang/management/ThreadInfo;+0 java.management@22-internal j sun.management.ThreadImpl.dumpAllThreads(ZZI)[Ljava/lang/management/ThreadInfo;+28 java.management@22-internal j sun.management.ThreadImpl.dumpAllThreads(ZZ)[Ljava/lang/management/ThreadInfo;+5 java.management@22-internal j IterateMonitorWithDeadObjectTest.dumpThreadsWithLockedMonitors()V+7 j IterateMonitorWithDeadObjectTest.main([Ljava/lang/String;)V+11 If I remove that assert I hit an NPE in the Java layer: java.lang.NullPointerException: Cannot invoke "Object.getClass()" because "lock" is null at java.management/java.lang.management.ThreadInfo.(ThreadInfo.java:172) at java.management/sun.management.ThreadImpl.dumpThreads0(Native Method) at java.management/sun.management.ThreadImpl.dumpAllThreads(ThreadImpl.java:518) at java.management/sun.management.ThreadImpl.dumpAllThreads(ThreadImpl.java:506) at IterateMonitorWithDeadObjectTest.dumpThreadsWithLockedMonitors(IterateMonitorWithDeadObjectTest.java:44) at IterateMonitorWithDeadObjectTest.main(IterateMonitorWithDeadObjectTest.java:66) at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103) at java.base/java.lang.reflect.Method.invoke(Method.java:580) at com.sun.javatest.regtest.agent.MainActionHelper$AgentVMRunnable.run(MainActionHelper.java:333) at java.base/java.lang.Thread.run(Thread.java:1570) - PR Review Comment: https://git.openjdk.org/jdk/pull/16783#discussion_r1403052907
RFR: 8320515: assert(monitor->object_peek() != nullptr) failed: Owned monitors should not have a dead object
In the rewrites made for: [JDK-8318757](https://bugs.openjdk.org/browse/JDK-8318757) `VM_ThreadDump asserts in interleaved ObjectMonitor::deflate_monitor calls` I removed the filtering of *owned ObjectMonitors with dead objects*. The reasoning was that you should never have an owned ObjectMonitor with a dead object. I added an assert to check this assumption. It turns out that the assumption was wrong *if* you use JNI to call MonitorEnter and then remove all references to the locked object. The provided tests provoke this assert form: * the JNI thread detach code * thread dumping with locked monitors, and * the JVMTI GetOwnedMonitorInfo API. While investigating this we've found that the thread detach code becomes more correct when this filter was removed. Previously, the locked monitors never got unlocked because the ObjectMonitor iterator never exposed these monitors to the JNI detach code that unlocks the thread's monitors. That bug caused an ObjectMonitor leak. So, for this case I'm leaving these ObjectMonitors unfiltered so that we don't reintroduce the leak. The thread dumping case doesn't tolerate ObjectMonitor with dead objects, so I'm filtering those in the closure that collects ObjectMonitor. Side note: We have discussions about ways to completely rewrite this by letting each thread have thread-local information about JNI held locks. If we have this we could probably throw away the entire ObjectMonitorDump hashtable, and its walk of the `_in_use_list.`. For GetOwnedMonitorInfo it is unclear if we should expose these weird ObjectMonitor. If we do, then the users can detect that a thread holds a lock with a dead object, and the code will return NULL as one of the "owned monitors" returned. I don't think that's a good idea, so I'm filtering out these ObjectMonitor for those calls. Test: the written tests with and without the fix. Tier1-Tier3, so far. - Commit messages: - 8320515: assert(monitor->object_peek() != nullptr) failed: Owned monitors should not have a dead object Changes: https://git.openjdk.org/jdk/pull/16783/files Webrev: https://webrevs.openjdk.org/?repo=jdk=16783=00 Issue: https://bugs.openjdk.org/browse/JDK-8320515 Stats: 258 lines in 8 files changed: 253 ins; 1 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/16783.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16783/head:pull/16783 PR: https://git.openjdk.org/jdk/pull/16783
Re: RFR: 8314502: Change the comparator taking version of GrowableArray::find to be a template method [v9]
On Wed, 22 Nov 2023 08:41:35 GMT, Afshin Zafari wrote:
>> The `find` method now is
>> ```C++
>> template
>> int find(T* token, bool f(T*, E)) const {
>> ...
>>
>> Any other functions which use this are also changed.
>> Local linux-x64-debug hotspot:tier1 passed. Mach5 tier1 build on linux and
>> Windows passed.
>
> Afshin Zafari has updated the pull request incrementally with one additional
> commit since the last revision:
>
> find methods accepts Function and callers provide lambda.
Thanks for making this change.
I'd like to suggest the following cleanups, some documentation, and a few tests:
https://github.com/openjdk/jdk/commit/20d4502471ba396ae395512cfa3dab3f87555421
I think it might be easier to review by looking at the final diff:
https://github.com/openjdk/jdk/compare/master...stefank:jdk:pr_15418
-
PR Comment: https://git.openjdk.org/jdk/pull/15418#issuecomment-1822887111
Re: RFR: 8314502: Change the comparator taking version of GrowableArray::find to be a template method [v3]
On Fri, 17 Nov 2023 13:20:22 GMT, Afshin Zafari wrote: >> @kimbarrett , @dholmes-ora , @merykitty >> Is there any comment on this PR? > >> @afshin-zafari I will leave it to other to (re-) review the latest changes. >> I don't grok this template stuff enough to pass judgement. > > Thank you very much @dholmes-ora, for your comments and discussions. @afshin-zafari I think you misunderstand the feedback given. The suggestions from many of us has been that you should change the functions to accept lambdas / template-typed functions instead. https://github.com/openjdk/jdk/pull/15418#discussion_r1305389552 https://github.com/openjdk/jdk/pull/15418#discussion_r1375386257 https://github.com/openjdk/jdk/pull/15418#discussion_r1376940244 https://github.com/openjdk/jdk/pull/15418#discussion_r1395219935 - PR Comment: https://git.openjdk.org/jdk/pull/15418#issuecomment-1816438451
Re: RFR: 8319876: Reduce memory consumption of VM_ThreadDump::doit [v3]
On Mon, 13 Nov 2023 10:11:26 GMT, Long Yang wrote: >> I would like to fix this. >> >> Create 4096 threads, and the stack depth of each thread is 256. >> After running jmx.dumpAllThreads(true, true), the RSS reaches 5.3GiB. >> After optimization, the RSS is 250MiB. >> >> I would appreciate it if anyone could review this. >> >> - >> update >> >> If the number of `threads` and `stack depth` are relatively large, we need >> to apply for more space in `ResourceArea` during the execution of >> `jmx.dumpAllThreads(true, true)`. >> >> The reason is that `VM_ThreadDump::doit` creates `vframe` for each `frame` >> of each `thread`. >> https://github.com/openjdk/jdk/blob/fe0ccdf5f8a5559a608d2e2cd2b6aecbe245c5ec/src/hotspot/share/services/threadService.cpp#L704 >> sizeof `vframe` is 4808 (bytes), and sizeof `compiledVFrame` is 4824 >> (bytes), mainly because the `xmm registers` in `RegisterMap` are relatively >> large. Assuming there are 4096 `threads` and each `thread` has 256 `frames`, >> the memory required is 4096 * 256 * 4824 = 4.7GiB。 >> >> These memories of all threads are released once by the the initial >> `ResourceMark` of `VM_ThreadDump::doit`. >> https://github.com/openjdk/jdk/blob/fe0ccdf5f8a5559a608d2e2cd2b6aecbe245c5ec/src/hotspot/share/runtime/vmOperations.cpp#L269 >> My solution is to add a `ResourceMark` for each thread. > > Long Yang has updated the pull request incrementally with one additional > commit since the last revision: > > Use VMThread::vm_thread() to avoid the need to call Thread::current() Marked as reviewed by stefank (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/16598#pullrequestreview-1736277454
Re: RFR: 8319876: Reduce memory consumption of VM_ThreadDump::doit [v2]
On Mon, 13 Nov 2023 02:01:09 GMT, David Holmes wrote: >> Long Yang has updated the pull request incrementally with one additional >> commit since the last revision: >> >> move ResourceMark to before start_vf > > src/hotspot/share/services/threadService.cpp line 698: > >> 696: RegisterMap::ProcessFrames::include, >> 697: RegisterMap::WalkContinuation::skip); >> 698: ResourceMark rm; > > Nit: Use `rm(VMThread::vm_thread());` to avoid the need to call > `Thread::current()`. FWIW, I don't see the appeal to micro optimize away one call to Thread::current() in a function that performs a massive amount of work. - PR Review Comment: https://git.openjdk.org/jdk/pull/16598#discussion_r1396841591
Re: RFR: 8318447: Move NMT source code to own subdirectory [v8]
On Thu, 26 Oct 2023 09:51:15 GMT, Johan Sjölen wrote: >> I think that NMT is deserving of its own subdirectory. Can we do a review of >> the changes before I fix the merge conflicts? >> >> 1. Moved all the nmt source code from services/ to nmt/ >> 2. Renamed all the include statements and sorted them >> 3. Fixed the include guards > > Johan Sjölen has updated the pull request incrementally with one additional > commit since the last revision: > > Stefan's suggestions Marked as reviewed by stefank (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/16276#pullrequestreview-1699205834
Re: RFR: 8318447: Move NMT source code to own subdirectory [v7]
On Tue, 24 Oct 2023 11:51:45 GMT, Johan Sjölen wrote: >> I think that NMT is deserving of its own subdirectory. Can we do a review of >> the changes before I fix the merge conflicts? >> >> 1. Moved all the nmt source code from services/ to nmt/ >> 2. Renamed all the include statements and sorted them >> 3. Fixed the include guards > > Johan Sjölen has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains eight commits: > > - Merge remote-tracking branch 'upstream/master' into move-nmt > - Fix stefank suggestions > - Merge remote-tracking branch 'origin/master' into move-nmt > - Fix messed up include > - Missed this include > - Merge remote-tracking branch 'origin/master' into move-nmt > - Fixed reviewed changes > - Move NMT to its own subdirectory I've approved the patch, but I think you should revert the two gtest changes I mention below. I have given suggestions for two cleanups that you might want to do. src/hotspot/share/memory/resourceArea.inline.hpp line 30: > 28: #include "memory/resourceArea.hpp" > 29: > 30: #include "nmt/memTracker.hpp" Another new line that should not be here. Bonus points if it gets removed. src/hotspot/share/nmt/mallocHeader.cpp line 29: > 27: #include "nmt/mallocHeader.inline.hpp" > 28: > 29: #include "nmt/mallocSiteTable.hpp" There should be no newline between these two includes. If you make another round of changes I think it would be good to get rid of it. test/hotspot/gtest/nmt/test_nmt_buffer_overflow_detection.cpp line 33: > 31: #include "testutils.hpp" > 32: #include "unittest.hpp" > 33: It is inclear to me if there is an ordering requirement between testutils and unittest. I'd prefer if you didn't change those unit test header files in this patch and make these cleanups separately. test/hotspot/gtest/nmt/test_nmt_locationprinting.cpp line 35: > 33: // Uncomment to get test output > 34: //#define LOG_PLEASE > 35: There seems to be an ordering requirement between LOG_PLEASE and the inclusion of LOG_PLEASE. I think you break it with this change. - Marked as reviewed by stefank (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16276#pullrequestreview-1697824750 PR Review Comment: https://git.openjdk.org/jdk/pull/16276#discussion_r1372058081 PR Review Comment: https://git.openjdk.org/jdk/pull/16276#discussion_r1372057335 PR Review Comment: https://git.openjdk.org/jdk/pull/16276#discussion_r1372053129 PR Review Comment: https://git.openjdk.org/jdk/pull/16276#discussion_r1372051716
Re: RFR: 8315097: Rename createJavaProcessBuilder [v3]
On Tue, 5 Sep 2023 18:05:34 GMT, Roger Riggs wrote: >> I have created an alternative that uses enums to force the user to make a >> decision: >> https://github.com/openjdk/jdk/compare/master...lkorinth:jdk:+process_tools >> . Another alternative is to do the same but instead using an enum (I think >> it is not as good). A third alternative is to use the current pull request >> with a better name. >> >> What do you prefer? Do you have a better alternative? Do someone still think >> the current code is good? I think what we have today is inferior to all >> these improvements, and I would like to make it harder to develop bad test >> cases. > >> What do you prefer? Do you have a better alternative? Do someone still think >> the current code is good? I think what we have today is inferior to all >> these improvements, and I would like to make it harder to develop bad test ca > > The current API (name) is fine and fit for purpose; it does not promise or > hide extra functionality under a simple name. > > There needs to be an explicit intention in the test(s) to support after the > fact that arbitrary flags can be added. > @AlanBateman's proposal for naming > [above](https://github.com/openjdk/jdk/pull/15452#issuecomment-1700459277) > (or similar) would capture more clearly that test options are propagated to > the child process. > Every test writer should be aware that additional command line options may be > mixed in. > > There are many cases in which the ProcessTools APIs are not used to create > child processes and do not need to be used in writing tests. They provide > some convenience but also add a dependency and another API layer to work > through in the case of failures. > > As far as I'm aware, there is no general guidance or design pattern outside > of hotspot tests to propagate flags or use ProcessTools. Adding that as a > requirement will need a different level of communication and change. @RogerRiggs You seem to know what you want w.r.t. the extra java doc comments. Could you help write those? Could we also do that as a separate RFE? I think that would make it easier to get this PR and the javadoc update through the door. - PR Comment: https://git.openjdk.org/jdk/pull/15452#issuecomment-1778669353
Re: RFR: 8315097: Rename createJavaProcessBuilder [v6]
On Tue, 24 Oct 2023 07:49:30 GMT, Leo Korinth wrote: >> Rename createJavaProcessBuilder so that it is not used by mistake instead of >> createTestJvm. >> >> I have used the following sed script: `find -name "*.java" | xargs -n 1 sed >> -i -e >> "s/createJavaProcessBuilder(/createJavaProcessBuilderIgnoreTestJavaOpts(/g"` >> >> Then I have manually modified ProcessTools.java. In that file I have moved >> one version of createJavaProcessBuilder so that it is close to the other >> version. Then I have added a javadoc comment in bold telling: >> >>/** >> * Create ProcessBuilder using the java launcher from the jdk to >> * be tested. >> * >> * Please observe that you likely should use >> * createTestJvm() instead of this method because createTestJvm() >> * will add JVM options from "test.vm.opts" and "test.java.opts" >> * and this method will not do that. >> * >> * @param command Arguments to pass to the java command. >> * @return The ProcessBuilder instance representing the java command. >> */ >> >> >> I have used the name createJavaProcessBuilderIgnoreTestJavaOpts because of >> the name of Utils.prependTestJavaOpts that adds those VM flags. If you have >> a better name I could do a rename of the method. I kind of like that it is >> long and clumsy, that makes it harder to use... >> >> I have run tier 1 testing, and I have started more exhaustive testing. > > Leo Korinth has updated the pull request incrementally with six additional > commits since the last revision: > > - static import > - copyright > - whitespace > - whitespace > - sed > - fix test/lib/jdk/test/lib/process/ProcessTools.java Marked as reviewed by stefank (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/15452#pullrequestreview-1694437335
Re: RFR: 8318447: Move NMT source code to own subdirectory
On Fri, 20 Oct 2023 11:31:11 GMT, Johan Sjölen wrote: > For the gtest source files I separated the includes in a consistent manner, > they all look like this pattern now: That's not what I see in the latest patch. Could you revert that separation and then we can consider that style change in a separate RFE? - PR Comment: https://git.openjdk.org/jdk/pull/16276#issuecomment-1774703929
Re: RFR: 8318447: Move NMT source code to own subdirectory [v6]
On Mon, 23 Oct 2023 08:34:59 GMT, Johan Sjölen wrote: >> I think that NMT is deserving of its own subdirectory. Can we do a review of >> the changes before I fix the merge conflicts? >> >> 1. Moved all the nmt source code from services/ to nmt/ >> 2. Renamed all the include statements and sorted them >> 3. Fixed the include guards > > Johan Sjölen has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains six commits: > > - Merge remote-tracking branch 'origin/master' into move-nmt > - Fix messed up include > - Missed this include > - Merge remote-tracking branch 'origin/master' into move-nmt > - Fixed reviewed changes > - Move NMT to its own subdirectory Changes requested by stefank (Reviewer). src/hotspot/share/services/mallocLimit.cpp line 32: > 30: #include "runtime/globals.hpp" > 31: #include "services/mallocLimit.hpp" > 32: #include "nmt/nmtCommon.hpp" Sort order test/hotspot/gtest/nmt/test_nmt_cornercases.cpp line 30: > 28: #include "nmt/mallocTracker.hpp" > 29: #include "runtime/os.hpp" > 30: #include "nmt/memTracker.hpp" Sort order - PR Review: https://git.openjdk.org/jdk/pull/16276#pullrequestreview-1692002562 PR Review Comment: https://git.openjdk.org/jdk/pull/16276#discussion_r1368319264 PR Review Comment: https://git.openjdk.org/jdk/pull/16276#discussion_r1368318877
