Re: [jdk23] RFR: 8326820: Metadata artificially kept alive
On Thu, 27 Jun 2024 14:30:43 GMT, Axel Boldt-Christmas wrote: > Hi all, > > This pull request contains a backport of commit > [5909d541](https://github.com/openjdk/jdk/commit/5909d54147355dd7da5786ff39ead4c15816705c) > from the [openjdk/jdk](https://git.openjdk.org/jdk) repository. > > The commit being backported was authored by Axel Boldt-Christmas on 27 Jun > 2024 and was reviewed by Erik Österlund, Stefan Karlsson and Coleen > Phillimore. > > Thanks! Marked as reviewed by stefank (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19929#pullrequestreview-2145723893
Re: RFR: 8326820: Metadata artificially kept alive [v3]
On Sat, 22 Jun 2024 00:18:43 GMT, Coleen Phillimore wrote: > The 'resolve' for the CLDG iterator was to temporarily keep that CLD from > being unloaded, in the short time that we're iterating on that particular CLD. This is the crux of the problem. For concurrent GCs, if the 'resolve' is called during a concurrent marking the CLD will be "marked" alive, and will be considered live all the way until we start a new concurrent mark cycle. At that point, we'll try again to figure out if the CLD is dead. If you then again use the iterator during that concurrent marking, the cycle repeats. So, if you tend to use these iterators a lot, we'll never get the chance to unload the classes. This patch tries to combat that, by changing the iterators. With the patch the iterators hands out objects that are not dead, but they are not considered part of the live object graph. You can use the oops (and its transitive closure) in the CLD as long as you block out safepoints. However, if you try to use them after blocking for a safepoint things will break because the objects are not guaranteed to be a part of the live object graph so nothing kept is keeping them alive. That's what the no-keepalive is intended to refer to. It would be great if we could figure out a name that hints that the iterators are unsafe to use unless you have understood the above. - PR Comment: https://git.openjdk.org/jdk/pull/19769#issuecomment-2185885243
Re: RFR: 8326820: Metadata artificially kept alive [v3]
On Thu, 20 Jun 2024 16:30:30 GMT, Coleen Phillimore wrote: > If the default is to not keep the CLD alive, I don't like that we need the > details of the side effect in the name. Just call it classes_do, etc. I don't > care about no-keepalive in all these callers, if that's the right answer for > most of these callers. Put the side effect in the name of the exceptional > cases. I disagree and probably urged/influenced Axel to name these functions this way. Very few people understand that they need to be careful around liveness of CLDs and klasses when they use these iterators/visitors. We keep seeing bugs in this area. Either because the devs fail to keep the klasses alive, or as in this bug, all klasses become kept alive. We want a name that strongly suggests that these functions are not as innocent as one might think, and that if you are going to use these functions you need to go an look at the comments (or ask someone) what we mean with no-keepalive. > > We had the iterator keep things alive for safety in the cases where we could > have the CLD unload while we were looking at it. It turns out that that safety causes significant problems for seldomly run, concurrent marking GCs. If you repeatedly use any of these APIs you in effect turn off class unloading when running wit ZGC. > In all these cases, this wasn't needed? We don't think they are. It would be great if you could take an extra close look at the class redefinition code. > Still have to work through that. Thanks! - PR Comment: https://git.openjdk.org/jdk/pull/19769#issuecomment-2181139233
Re: RFR: 8326820: Metadata artificially kept alive [v3]
On Wed, 19 Jun 2024 15:06:25 GMT, Axel Boldt-Christmas wrote: >> ClassLoaderDataGraph provides APIs for walking different metadata. All the >> iterators which are not designed to be used by the GC also keep the holder >> of the CLDs alive and by extensions keeps all metadata alive. This is >> problematic for concurrent GC as it keeps otherwise unreachable classes from >> being unloaded and the respective metadata freed. >> >> This patch changes the default iteration behaviour to not keep the holder >> alive, with the exception of `loaded_classes_do` (renamed >> `loaded_classes_do_keepalive`) and `modules_do` (renamed >> `modules_do_keepalive`) which is used by jvmti APIs that requires that the >> holder is kept alive. >> >> All other uses consumes all the metadata it queries during its safepoint or >> before releasing the `ClassLoaderDataGraph_lock`. >> >> Before this change some jcmd, new jfr chunks and some jfr events, all of >> which consumed these APIs, could cause class unloading to not occur. >> >> Been running our internal stress test in an even more stressful mode which >> without this patch reproduces the metaspace OOME >> [JDK-8326005](https://bugs.openjdk.org/browse/JDK-8326005) consistently >> within a few hours. And after this patch it does not. >> >> Currently running tier1-tier8 testing. > > Axel Boldt-Christmas has updated the pull request incrementally with one > additional commit since the last revision: > > Rename and comment SystemDictionary::methods_do Marked as reviewed by stefank (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19769#pullrequestreview-2128591105
Re: RFR: 8326820: Metadata artificially kept alive [v2]
On Wed, 19 Jun 2024 06:14:21 GMT, Axel Boldt-Christmas
wrote:
>> ClassLoaderDataGraph provides APIs for walking different metadata. All the
>> iterators which are not designed to be used by the GC also keep the holder
>> of the CLDs alive and by extensions keeps all metadata alive. This is
>> problematic for concurrent GC as it keeps otherwise unreachable classes from
>> being unloaded and the respective metadata freed.
>>
>> This patch changes the default iteration behaviour to not keep the holder
>> alive, with the exception of `loaded_classes_do` (renamed
>> `loaded_classes_do_keepalive`) and `modules_do` (renamed
>> `modules_do_keepalive`) which is used by jvmti APIs that requires that the
>> holder is kept alive.
>>
>> All other uses consumes all the metadata it queries during its safepoint or
>> before releasing the `ClassLoaderDataGraph_lock`.
>>
>> Before this change some jcmd, new jfr chunks and some jfr events, all of
>> which consumed these APIs, could cause class unloading to not occur.
>>
>> Been running our internal stress test in an even more stressful mode which
>> without this patch reproduces the metaspace OOME
>> [JDK-8326005](https://bugs.openjdk.org/browse/JDK-8326005) consistently
>> within a few hours. And after this patch it does not.
>>
>> Currently running tier1-tier8 testing.
>
> Axel Boldt-Christmas has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Document the iterator and functions
Looks good. One question below.
src/hotspot/share/classfile/systemDictionary.cpp line 1588:
> 1586: {
> 1587: MutexLocker ml(ClassLoaderDataGraph_lock);
> 1588: ClassLoaderDataGraph::methods_do_no_keepalive(f);
What about the call (SystemDictionary::methods_do), should we leave that name
as-is or does it also need to be suffixed with no_keepalive?
-
Marked as reviewed by stefank (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/19769#pullrequestreview-2127563591
PR Review Comment: https://git.openjdk.org/jdk/pull/19769#discussion_r1645724854
Re: RFR: 8332252: Clean up vmTestbase/vm/share [v3]
On Sat, 15 Jun 2024 16:28:05 GMT, Leonid Mesnik wrote: >> The vmTestbase/vm/share is a shared test library for vmTestbase tests. This >> library contains a lot of code that is used by only by small number of tests >> or not used at all. There are no plans to actively develop new tests in >> vmTestsbase and improve this shared library. >> The final goal of this and the following PRs is to reduce the maintenance >> cost of vmTestbase by eliminating this library. >> >> Also, this PR moves test-specific code into corresponding test directories >> to increase code locality. This allows later easier move tests from >> vmTestbase. >> >> The few remaining classes include >> InMemoryJavaCompiler.java >> that is very similar to same class from the standard testlibrary and could >> be merge with it and >> ProcessUtils.java >> which is used by >> test/hotspot/jtreg/runtime/Thread/TestBreakSignalThreadDump.java >> and thus should be moved into the standard testlibrary. >> The stack and options might be merged in nsk/share test library. > > Leonid Mesnik has updated the pull request incrementally with one additional > commit since the last revision: > > removed unused import Some of the moves/renames messed up the sort-order of the imports. Could you take a pass over the patch and clean that up? - PR Review: https://git.openjdk.org/jdk/pull/19727#pullrequestreview-2122317400
Re: RFR: 8332494: java/util/zip/EntryCount64k.java failing with java.lang.RuntimeException: '\\A\\Z' missing from stderr [v2]
On Mon, 20 May 2024 07:24:16 GMT, Axel Boldt-Christmas wrote: >> Improve `java/util/zip/EntryCount64k.java` stderr parsing by ignoring >> deprecated warnings. Testing non-generational ZGC requires the use of a >> deprecated option. > > Axel Boldt-Christmas has updated the pull request incrementally with one > additional commit since the last revision: > > Update copyright year Marked as reviewed by stefank (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19297#pullrequestreview-2065538663
Re: RFR: 8332495: java/util/logging/LoggingDeadlock2.java fails with AssertionError: Some tests failed [v2]
On Mon, 20 May 2024 07:22:49 GMT, Axel Boldt-Christmas wrote: >> Improve ` java/util/logging/LoggingDeadlock2.java` stderr parsing by >> ignoring deprecated warnings. Testing non-generational ZGC requires the use >> of a deprecated option. > > Axel Boldt-Christmas has updated the pull request incrementally with one > additional commit since the last revision: > > Update copyright year Marked as reviewed by stefank (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19298#pullrequestreview-2065533348
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: 8327180: Failed: java/io/ObjectStreamClass/ObjectStreamClassCaching.java#G1 [v3]
On Wed, 13 Mar 2024 20:10:25 GMT, Roger Riggs wrote: >> The intermittent failure of ObjectStreamClassCaching is due to an incorrect >> assumption about GC behavior and a race condition. >> >> Removed test based on incorrect assumptions about simultaneous clearing of >> WeakReferences. >> Added a run of the test using ZGC, (previously omitted) > > Roger Riggs has updated the pull request incrementally with one additional > commit since the last revision: > > Use the correct ZGenerational collector Looks good. Make sure that the test has run after the -XX:+ZGenerational flag was added. - Marked as reviewed by stefank (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18284#pullrequestreview-1935146818
Re: RFR: 8327180: Failed: java/io/ObjectStreamClass/ObjectStreamClassCaching.java#G1 [v2]
On Wed, 13 Mar 2024 19:41:25 GMT, Roger Riggs wrote: >> The intermittent failure of ObjectStreamClassCaching is due to an incorrect >> assumption about GC behavior and a race condition. >> >> Removed test based on incorrect assumptions about simultaneous clearing of >> WeakReferences. >> Added a run of the test using ZGC, (previously omitted) > > Roger Riggs has updated the pull request incrementally with one additional > commit since the last revision: > > Remove obsolete note; no longer disabled Changes requested by stefank (Reviewer). test/jdk/java/io/ObjectStreamClass/ObjectStreamClassCaching.java line 57: > 55: * @library /test/lib/ > 56: * @summary ObjectStreamClass caches keep ClassLoaders alive (ZGC) > 57: * @run testng/othervm -Xmx64m -XX:+UseZGC ObjectStreamClassCaching This test is meant to run with Generational ZGC (according to the requires line). It needs to run with `-XX:+UseZGC -XX:+ZGenerational` to enable Generational ZGC. If you run with `-XX:+UseZGC` you get the older, non-generational ZGC. - PR Review: https://git.openjdk.org/jdk/pull/18284#pullrequestreview-1935079550 PR Review Comment: https://git.openjdk.org/jdk/pull/18284#discussion_r1523833917
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: 8320699: Add parameter to skip progress logging of OutputAnalyzer [v4]
> Tests using ProcessTools.executeProcess gets the following output written to > stdout: > [2023-11-24T09:58:16.797540608Z] Gathering output for process 2517117 > [2023-11-24T09:58:23.070781345Z] Waiting for completion for process 2517117 > [2023-11-24T09:58:23.071045055Z] Waiting for completion finished for process > 2517117 > > This might be good for some use cases and debugging, but some tests spawns a > large number of processes and for those this output fills up the log files. > > I propose that we add a way to turn of this output for tests where we find > this output to be too noisy. Stefan Karlsson has updated the pull request incrementally with one additional commit since the last revision: Update copyright years - Changes: - all: https://git.openjdk.org/jdk/pull/16807/files - new: https://git.openjdk.org/jdk/pull/16807/files/9a43b2c9..4fb2fc21 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16807=03 - incr: https://webrevs.openjdk.org/?repo=jdk=16807=02-03 Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/16807.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16807/head:pull/16807 PR: https://git.openjdk.org/jdk/pull/16807
Re: RFR: 8320699: Add parameter to skip progress logging of OutputAnalyzer [v3]
On Fri, 12 Jan 2024 01:10:49 GMT, Leonid Mesnik wrote: > Needed to update copyrights now. Even when the code was written in 2023? - PR Comment: https://git.openjdk.org/jdk/pull/16807#issuecomment-1888699636
Re: RFR: 8323508: Remove TestGCLockerWithShenandoah.java line from TEST.groups
On Wed, 10 Jan 2024 12:09:07 GMT, Stefan Karlsson wrote: > TestGCLockerWithShenandoah.java was recently removed, but the TEST.groups > file still has a reference to it. This causes problems in our CI pipeline. Thanks for the reviews! - PR Comment: https://git.openjdk.org/jdk/pull/17344#issuecomment-1884845496
Integrated: 8323508: Remove TestGCLockerWithShenandoah.java line from TEST.groups
On Wed, 10 Jan 2024 12:09:07 GMT, Stefan Karlsson wrote: > TestGCLockerWithShenandoah.java was recently removed, but the TEST.groups > file still has a reference to it. This causes problems in our CI pipeline. This pull request has now been integrated. Changeset: ec385057 Author:Stefan Karlsson URL: https://git.openjdk.org/jdk/commit/ec38505720251ceefc8e838bd68b740d166c83c1 Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod 8323508: Remove TestGCLockerWithShenandoah.java line from TEST.groups Reviewed-by: dholmes, shade, tschatzl - PR: https://git.openjdk.org/jdk/pull/17344
RFR: 8323508: Remove TestGCLockerWithShenandoah.java line from TEST.groups
TestGCLockerWithShenandoah.java was recently removed, but the TEST.groups file still has a reference to it. This causes problems in our CI pipeline. - Commit messages: - Revert unrelated changes - 8323508: Remove TestGCLockerWithShenandoah.java line from TEST.groups Changes: https://git.openjdk.org/jdk/pull/17344/files Webrev: https://webrevs.openjdk.org/?repo=jdk=17344=00 Issue: https://bugs.openjdk.org/browse/JDK-8323508 Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/17344.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17344/head:pull/17344 PR: https://git.openjdk.org/jdk/pull/17344
Re: RFR: 8323508: Remove TestGCLockerWithShenandoah.java line from TEST.groups
On Wed, 10 Jan 2024 12:09:07 GMT, Stefan Karlsson wrote: > TestGCLockerWithShenandoah.java was recently removed, but the TEST.groups > file still has a reference to it. This causes problems in our CI pipeline. Thanks. The unrelated change has been reverted. - PR Comment: https://git.openjdk.org/jdk/pull/17344#issuecomment-1884749086
Re: RFR: 8322920: Some ProcessTools.execute* functions are declared to throw Throwable [v2]
On Fri, 5 Jan 2024 08:22:41 GMT, Stefan Karlsson wrote: >> Most functions in ProcessTools are declared to throw Exceptions, or one of >> its subclasses. However, there are a small number of functions that are >> unnecessarily declared to throw Throwable instead of Exception. I propose >> that we change them to also be declared to throw Exception. >> >> This is a trivial patch to make it easier to refactor tests to use the >> updated functions. >> >> Tested manually, but will wait for GHA to verify that the change is OK. > > Stefan Karlsson has updated the pull request incrementally with one > additional commit since the last revision: > > Copyright year Thanks for the reviews. Testing with Tier1-3 passes. - PR Comment: https://git.openjdk.org/jdk/pull/17240#issuecomment-1878341513
Integrated: 8322920: Some ProcessTools.execute* functions are declared to throw Throwable
On Wed, 3 Jan 2024 09:51:24 GMT, Stefan Karlsson wrote: > Most functions in ProcessTools are declared to throw Exceptions, or one of > its subclasses. However, there are a small number of functions that are > unnecessarily declared to throw Throwable instead of Exception. I propose > that we change them to also be declared to throw Exception. > > This is a trivial patch to make it easier to refactor tests to use the > updated functions. > > Tested manually, but will wait for GHA to verify that the change is OK. This pull request has now been integrated. Changeset: 868f8745 Author:Stefan Karlsson URL: https://git.openjdk.org/jdk/commit/868f8745faf70c915d8294ae8f85b2d6aa096900 Stats: 6 lines in 1 file changed: 0 ins; 2 del; 4 mod 8322920: Some ProcessTools.execute* functions are declared to throw Throwable Reviewed-by: dholmes, lmesnik - PR: https://git.openjdk.org/jdk/pull/17240
Re: RFR: 8320699: Add parameter to skip progress logging of OutputAnalyzer [v3]
> Tests using ProcessTools.executeProcess gets the following output written to > stdout: > [2023-11-24T09:58:16.797540608Z] Gathering output for process 2517117 > [2023-11-24T09:58:23.070781345Z] Waiting for completion for process 2517117 > [2023-11-24T09:58:23.071045055Z] Waiting for completion finished for process > 2517117 > > This might be good for some use cases and debugging, but some tests spawns a > large number of processes and for those this output fills up the log files. > > I propose that we add a way to turn of this output for tests where we find > this output to be too noisy. Stefan Karlsson has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains three commits: - Merge remote-tracking branch 'upstream/master' into 8320699_OutputAnalyzer_progress_logging - Update OutputBuffer.java copyright years - 8320699: Add parameter to skip progress logging of OutputAnalyzer - Changes: https://git.openjdk.org/jdk/pull/16807/files Webrev: https://webrevs.openjdk.org/?repo=jdk=16807=02 Stats: 24 lines in 2 files changed: 21 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/16807.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16807/head:pull/16807 PR: https://git.openjdk.org/jdk/pull/16807
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: 8322920: Some ProcessTools.execute* functions are declared to throw Throwable [v2]
> Most functions in ProcessTools are declared to throw Exceptions, or one of > its subclasses. However, there are a small number of functions that are > unnecessarily declared to throw Throwable instead of Exception. I propose > that we change them to also be declared to throw Exception. > > This is a trivial patch to make it easier to refactor tests to use the > updated functions. > > Tested manually, but will wait for GHA to verify that the change is OK. Stefan Karlsson has updated the pull request incrementally with one additional commit since the last revision: Copyright year - Changes: - all: https://git.openjdk.org/jdk/pull/17240/files - new: https://git.openjdk.org/jdk/pull/17240/files/910a863c..402b6727 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17240=01 - incr: https://webrevs.openjdk.org/?repo=jdk=17240=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/17240.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17240/head:pull/17240 PR: https://git.openjdk.org/jdk/pull/17240
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
RFR: 8322920: Some executeProcess overloads are declared to throw Throwable
Most functions in ProcessTools are declared to throw Exceptions, or one of its subclasses. However, there are a small number of functions that are unnecessarily declared to throw Throwable instead of Exception. I propose that we change them to also be declared to throw Exception. This is a trivial patch to make it easier to refactor tests to use the updated functions. Tested manually, but will wait for GHA to verify that the change is OK. - Commit messages: - 8322920: Some executeProcess overloads are declared to throw Throwable Changes: https://git.openjdk.org/jdk/pull/17240/files Webrev: https://webrevs.openjdk.org/?repo=jdk=17240=00 Issue: https://bugs.openjdk.org/browse/JDK-8322920 Stats: 5 lines in 1 file changed: 0 ins; 2 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/17240.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17240/head:pull/17240 PR: https://git.openjdk.org/jdk/pull/17240
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
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
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
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 [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 [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 [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
Integrated: 8321718: ProcessTools.executeProcess calls waitFor before logging
On Mon, 11 Dec 2023 11:16:05 GMT, Stefan Karlsson wrote: > There is some logging printed when tests spawns processes. This logging is > triggered from calls to `OutputAnalyzer`, when it delegates calls to > `LazyOutputBuffer`. > > If we write code like this: > > ProcessBuilder pb = ProcessTools.createTestJavaProcessBuilder(...); > OutputAnalyzer output = new OutputAnalyzer(pb.start()); > int exitValue = output.getExitValue(); > > > We get the following logging: > > [timestamp0] "Gathering output for process > [timestamp1] Waiting for completion for process > [timestamp2] Waiting for completion finished for process > > > The first line comes from the `OutputAnalyzer` constructor and the two other > lines comes from the `getExitValue` call, which calls logs the above messages > around the call to `waitFor`. > > If instead write the code above as: > > ProcessBuilder pb = ProcessTools.createTestJavaProcessBuilder(...); > OutputAnalyzer output = ProcessTools.executeProcess(pb); > int exitValue = output.getExitValue(); > > > We get the same logging, but timestamp1 and timestamp2 are almost the same. > This happens because there's an extra call to `waitFor` inside > `executeProcess`, but that `waitFor` does not have the "wait for" logging. > So, instead we get the logging for the no-op `waitFor` inside `getExitValue`. > > I would like to propose a small workaround to solve this. The workaround is > that `executeProcess` delegates the `waitFor` call to the `LazyOutputBuffer` > via `OutputAnalyzer`. This is a small, limited workaround for this issue. > Ideally I would like to extract the entire Process handling code out of > LazyOutputBuffer and OutputAnalyzer, but the prototype for that rewrites all > usages of `new OutputAnalyzer(pb.start())` and stretches far and wide in the > test directories, so I'm starting out with this suggestion. > > We can see of this patch by looking at the timestamps generated from the > included test. Without the proposed workaround: > > Without > > testExecuteProcessExit > [2023-12-11T11:05:41.854579260Z] Gathering output for process 2547719 > [2023-12-11T11:05:44.018335073Z] Waiting for completion for process 2547719 > [2023-12-11T11:05:44.018851972Z] Waiting for completion finished for process > 2547719 > > testExecuteProcessStdout > [2023-12-11T11:05:44.049509860Z] Gathering output for process 2547741 > [2023-12-11T11:05:46.227768897Z] Waiting for completion for process 2547741 > [2023-12-11T11:05:46.228021173Z] Waiting for completion finished for process > 2547741 > > > testNewOutputAnalyzerExit > [2023-12-11T11:05:46.231475003Z] Gathering output for process 2547782 > [2023... This pull request has now been integrated. Changeset: 9ab29f8d Author:Stefan Karlsson URL: https://git.openjdk.org/jdk/commit/9ab29f8dcd1c0092e4251f996bd53c704e87a74a Stats: 61 lines in 3 files changed: 47 ins; 11 del; 3 mod 8321718: ProcessTools.executeProcess calls waitFor before logging Reviewed-by: dholmes, jpai - PR: https://git.openjdk.org/jdk/pull/17052
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: 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: 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 [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: 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: 8321718: ProcessTools.executeProcess calls waitFor before logging [v2]
On Tue, 12 Dec 2023 09:01:08 GMT, Stefan Karlsson wrote: >> There is some logging printed when tests spawns processes. This logging is >> triggered from calls to `OutputAnalyzer`, when it delegates calls to >> `LazyOutputBuffer`. >> >> If we write code like this: >> >> ProcessBuilder pb = ProcessTools.createTestJavaProcessBuilder(...); >> OutputAnalyzer output = new OutputAnalyzer(pb.start()); >> int exitValue = output.getExitValue(); >> >> >> We get the following logging: >> >> [timestamp0] "Gathering output for process >> [timestamp1] Waiting for completion for process >> [timestamp2] Waiting for completion finished for process >> >> >> The first line comes from the `OutputAnalyzer` constructor and the two other >> lines comes from the `getExitValue` call, which calls logs the above >> messages around the call to `waitFor`. >> >> If instead write the code above as: >> >> ProcessBuilder pb = ProcessTools.createTestJavaProcessBuilder(...); >> OutputAnalyzer output = ProcessTools.executeProcess(pb); >> int exitValue = output.getExitValue(); >> >> >> We get the same logging, but timestamp1 and timestamp2 are almost the same. >> This happens because there's an extra call to `waitFor` inside >> `executeProcess`, but that `waitFor` does not have the "wait for" logging. >> So, instead we get the logging for the no-op `waitFor` inside `getExitValue`. >> >> I would like to propose a small workaround to solve this. The workaround is >> that `executeProcess` delegates the `waitFor` call to the `LazyOutputBuffer` >> via `OutputAnalyzer`. This is a small, limited workaround for this issue. >> Ideally I would like to extract the entire Process handling code out of >> LazyOutputBuffer and OutputAnalyzer, but the prototype for that rewrites all >> usages of `new OutputAnalyzer(pb.start())` and stretches far and wide in the >> test directories, so I'm starting out with this suggestion. >> >> We can see of this patch by looking at the timestamps generated from the >> included test. Without the proposed workaround: >> >> Without >> >> testExecuteProcessExit >> [2023-12-11T11:05:41.854579260Z] Gathering output for process 2547719 >> [2023-12-11T11:05:44.018335073Z] Waiting for completion for process 2547719 >> [2023-12-11T11:05:44.018851972Z] Waiting for completion finished for process >> 2547719 >> >> testExecuteProcessStdout >> [2023-12-11T11:05:44.049509860Z] Gathering output for process 2547741 >> [2023-12-11T11:05:46.227768897Z] Waiting for completion for process 2547741 >> [2023-12-11T11:05:46.228021173Z] Waiting for completion finished for process >> 2547741 >> >> >> testNewOutp... > > Stefan Karlsson has updated the pull request incrementally with one > additional commit since the last revision: > > Typo Thanks for reviewing! I'll remove the test, merge, and will do some sanity checks before integrating. - PR Comment: https://git.openjdk.org/jdk/pull/17052#issuecomment-1874162036
Re: RFR: 8321718: ProcessTools.executeProcess calls waitFor before logging [v3]
> There is some logging printed when tests spawns processes. This logging is > triggered from calls to `OutputAnalyzer`, when it delegates calls to > `LazyOutputBuffer`. > > If we write code like this: > > ProcessBuilder pb = ProcessTools.createTestJavaProcessBuilder(...); > OutputAnalyzer output = new OutputAnalyzer(pb.start()); > int exitValue = output.getExitValue(); > > > We get the following logging: > > [timestamp0] "Gathering output for process > [timestamp1] Waiting for completion for process > [timestamp2] Waiting for completion finished for process > > > The first line comes from the `OutputAnalyzer` constructor and the two other > lines comes from the `getExitValue` call, which calls logs the above messages > around the call to `waitFor`. > > If instead write the code above as: > > ProcessBuilder pb = ProcessTools.createTestJavaProcessBuilder(...); > OutputAnalyzer output = ProcessTools.executeProcess(pb); > int exitValue = output.getExitValue(); > > > We get the same logging, but timestamp1 and timestamp2 are almost the same. > This happens because there's an extra call to `waitFor` inside > `executeProcess`, but that `waitFor` does not have the "wait for" logging. > So, instead we get the logging for the no-op `waitFor` inside `getExitValue`. > > I would like to propose a small workaround to solve this. The workaround is > that `executeProcess` delegates the `waitFor` call to the `LazyOutputBuffer` > via `OutputAnalyzer`. This is a small, limited workaround for this issue. > Ideally I would like to extract the entire Process handling code out of > LazyOutputBuffer and OutputAnalyzer, but the prototype for that rewrites all > usages of `new OutputAnalyzer(pb.start())` and stretches far and wide in the > test directories, so I'm starting out with this suggestion. > > We can see of this patch by looking at the timestamps generated from the > included test. Without the proposed workaround: > > Without > > testExecuteProcessExit > [2023-12-11T11:05:41.854579260Z] Gathering output for process 2547719 > [2023-12-11T11:05:44.018335073Z] Waiting for completion for process 2547719 > [2023-12-11T11:05:44.018851972Z] Waiting for completion finished for process > 2547719 > > testExecuteProcessStdout > [2023-12-11T11:05:44.049509860Z] Gathering output for process 2547741 > [2023-12-11T11:05:46.227768897Z] Waiting for completion for process 2547741 > [2023-12-11T11:05:46.228021173Z] Waiting for completion finished for process > 2547741 > > > testNewOutputAnalyzerExit > [2023-12-11T11:05:46.231475003Z] Gathering output for process 2547782 > [2023... 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 executeProcessLogging - Remove temporary test - Typo - Whitespace cleanups - 8321718: ProcessTools.executeProcess calls waitFor before logging - Changes: - all: https://git.openjdk.org/jdk/pull/17052/files - new: https://git.openjdk.org/jdk/pull/17052/files/3275136c..fcba9dbd Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17052=02 - incr: https://webrevs.openjdk.org/?repo=jdk=17052=01-02 Stats: 5347 lines in 349 files changed: 3069 ins; 1071 del; 1207 mod Patch: https://git.openjdk.org/jdk/pull/17052.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17052/head:pull/17052 PR: https://git.openjdk.org/jdk/pull/17052
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: 8321718: ProcessTools.executeProcess calls waitFor before logging [v2]
> There is some logging printed when tests spawns processes. This logging is > triggered from calls to `OutputAnalyzer`, when it delegates calls to > `LazyOutputBuffer`. > > If we write code like this: > > ProcessBuilder pb = ProcessTools.createTestJavaProcessBuilder(...); > OutputAnalyzer output = new OutputAnalyzer(pb.start()); > int exitValue = output.getExitValue(); > > > We get the following logging: > > [timestamp0] "Gathering output for process > [timestamp1] Waiting for completion for process > [timestamp2] Waiting for completion finished for process > > > The first line comes from the `OutputAnalyzer` constructor and the two other > lines comes from the `getExitValue` call, which calls logs the above messages > around the call to `waitFor`. > > If instead write the code above as: > > ProcessBuilder pb = ProcessTools.createTestJavaProcessBuilder(...); > OutputAnalyzer output = ProcessTools.executeProcess(pb); > int exitValue = output.getExitValue(); > > > We get the same logging, but timestamp1 and timestamp2 are almost the same. > This happens because there's an extra call to `waitFor` inside > `executeProcess`, but that `waitFor` does not have the "wait for" logging. > So, instead we get the logging for the no-op `waitFor` inside `getExitValue`. > > I would like to propose a small workaround to solve this. The workaround is > that `executeProcess` delegates the `waitFor` call to the `LazyOutputBuffer` > via `OutputAnalyzer`. This is a small, limited workaround for this issue. > Ideally I would like to extract the entire Process handling code out of > LazyOutputBuffer and OutputAnalyzer, but the prototype for that rewrites all > usages of `new OutputAnalyzer(pb.start())` and stretches far and wide in the > test directories, so I'm starting out with this suggestion. > > We can see of this patch by looking at the timestamps generated from the > included test. Without the proposed workaround: > > Without > > testExecuteProcessExit > [2023-12-11T11:05:41.854579260Z] Gathering output for process 2547719 > [2023-12-11T11:05:44.018335073Z] Waiting for completion for process 2547719 > [2023-12-11T11:05:44.018851972Z] Waiting for completion finished for process > 2547719 > > testExecuteProcessStdout > [2023-12-11T11:05:44.049509860Z] Gathering output for process 2547741 > [2023-12-11T11:05:46.227768897Z] Waiting for completion for process 2547741 > [2023-12-11T11:05:46.228021173Z] Waiting for completion finished for process > 2547741 > > > testNewOutputAnalyzerExit > [2023-12-11T11:05:46.231475003Z] Gathering output for process 2547782 > [2023... Stefan Karlsson has updated the pull request incrementally with one additional commit since the last revision: Typo - Changes: - all: https://git.openjdk.org/jdk/pull/17052/files - new: https://git.openjdk.org/jdk/pull/17052/files/145e1eaa..3275136c Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17052=01 - incr: https://webrevs.openjdk.org/?repo=jdk=17052=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/17052.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17052/head:pull/17052 PR: https://git.openjdk.org/jdk/pull/17052
Re: RFR: 8321718: ProcessTools.executeProcess calls waitFor before logging [v2]
On Tue, 12 Dec 2023 07:43:31 GMT, David Holmes wrote: >> Stefan Karlsson has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Typo > > test/lib/jdk/test/lib/process/OutputAnalyzer.java line 111: > >> 109: /** >> 110: * Delegate waitFor to the OutputBuffer. This ensures that >> 111: * the progress and timestmaps are logged correctly. > > Typo: timestmaps Thanks. Updated - PR Review Comment: https://git.openjdk.org/jdk/pull/17052#discussion_r1423664193
Re: RFR: 8321718: ProcessTools.executeProcess calls waitFor before logging
On Mon, 11 Dec 2023 11:16:05 GMT, Stefan Karlsson wrote: > There is some logging printed when tests spawns processes. This logging is > triggered from calls to `OutputAnalyzer`, when it delegates calls to > `LazyOutputBuffer`. > > If we write code like this: > > ProcessBuilder pb = ProcessTools.createTestJavaProcessBuilder(...); > OutputAnalyzer output = new OutputAnalyzer(pb.start()); > int exitValue = output.getExitValue(); > > > We get the following logging: > > [timestamp0] "Gathering output for process > [timestamp1] Waiting for completion for process > [timestamp2] Waiting for completion finished for process > > > The first line comes from the `OutputAnalyzer` constructor and the two other > lines comes from the `getExitValue` call, which calls logs the above messages > around the call to `waitFor`. > > If instead write the code above as: > > ProcessBuilder pb = ProcessTools.createTestJavaProcessBuilder(...); > OutputAnalyzer output = ProcessTools.executeProcess(pb); > int exitValue = output.getExitValue(); > > > We get the same logging, but timestamp1 and timestamp2 are almost the same. > This happens because there's an extra call to `waitFor` inside > `executeProcess`, but that `waitFor` does not have the "wait for" logging. > So, instead we get the logging for the no-op `waitFor` inside `getExitValue`. > > I would like to propose a small workaround to solve this. The workaround is > that `executeProcess` delegates the `waitFor` call to the `LazyOutputBuffer` > via `OutputAnalyzer`. This is a small, limited workaround for this issue. > Ideally I would like to extract the entire Process handling code out of > LazyOutputBuffer and OutputAnalyzer, but the prototype for that rewrites all > usages of `new OutputAnalyzer(pb.start())` and stretches far and wide in the > test directories, so I'm starting out with this suggestion. > > We can see of this patch by looking at the timestamps generated from the > included test. Without the proposed workaround: > > Without > > testExecuteProcessExit > [2023-12-11T11:05:41.854579260Z] Gathering output for process 2547719 > [2023-12-11T11:05:44.018335073Z] Waiting for completion for process 2547719 > [2023-12-11T11:05:44.018851972Z] Waiting for completion finished for process > 2547719 > > testExecuteProcessStdout > [2023-12-11T11:05:44.049509860Z] Gathering output for process 2547741 > [2023-12-11T11:05:46.227768897Z] Waiting for completion for process 2547741 > [2023-12-11T11:05:46.228021173Z] Waiting for completion finished for process > 2547741 > > > testNewOutputAnalyzerExit > [2023-12-11T11:05:46.231475003Z] Gathering output for process 2547782 > [2023... Thanks for reviewing! I'm considering removing the test before integrating, as it currently takes >8s to run and it was mainly used to show the difference between the implementations. Do you agree that it would be OK to remove the test? - PR Comment: https://git.openjdk.org/jdk/pull/17052#issuecomment-1851564295
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 [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 [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 [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
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
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
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: 8321718: ProcessTools.executeProcess calls waitFor before logging
There is some logging printed when tests spawns processes. This logging is triggered from calls to `OutputAnalyzer`, when it delegates calls to `LazyOutputBuffer`. If we write code like this: ProcessBuilder pb = ProcessTools.createTestJavaProcessBuilder(...); OutputAnalyzer output = new OutputAnalyzer(pb.start()); int exitValue = output.getExitValue(); We get the following logging: [timestamp0] "Gathering output for process [timestamp1] Waiting for completion for process [timestamp2] Waiting for completion finished for process The first line comes from the `OutputAnalyzer` constructor and the two other lines comes from the `getExitValue` call, which calls logs the above messages around the call to `waitFor`. If instead write the code above as: ProcessBuilder pb = ProcessTools.createTestJavaProcessBuilder(...); OutputAnalyzer output = ProcessTools.executeProcess(pb); int exitValue = output.getExitValue(); We get the same logging, but timestamp1 and timestamp2 are almost the same. This happens because there's an extra call to `waitFor` inside `executeProcess`, but that `waitFor` does not have the "wait for" logging. So, instead we get the logging for the no-op `waitFor` inside `getExitValue`. I would like to propose a small workaround to solve this. The workaround is that `executeProcess` delegates the `waitFor` call to the `LazyOutputBuffer` via `OutputAnalyzer`. This is a small, limited workaround for this issue. Ideally I would like to extract the entire Process handling code out of LazyOutputBuffer and OutputAnalyzer, but the prototype for that rewrites all usages of `new OutputAnalyzer(pb.start())` and stretches far and wide in the test directories, so I'm starting out with this suggestion. We can see of this patch by looking at the timestamps generated from the included test. Without the proposed workaround: Without testExecuteProcessExit [2023-12-11T11:05:41.854579260Z] Gathering output for process 2547719 [2023-12-11T11:05:44.018335073Z] Waiting for completion for process 2547719 [2023-12-11T11:05:44.018851972Z] Waiting for completion finished for process 2547719 testExecuteProcessStdout [2023-12-11T11:05:44.049509860Z] Gathering output for process 2547741 [2023-12-11T11:05:46.227768897Z] Waiting for completion for process 2547741 [2023-12-11T11:05:46.228021173Z] Waiting for completion finished for process 2547741 testNewOutputAnalyzerExit [2023-12-11T11:05:46.231475003Z] Gathering output for process 2547782 [2023-12-11T11:05:46.231972095Z] Waiting for completion for process 2547782 [2023-12-11T11:05:48.421324123Z] Waiting for completion finished for process 2547782 testNewOutputAnalyzerStdout [2023-12-11T11:05:48.424801269Z] Gathering output for process 2547929 With the proposed workaround: testExecuteProcessExit [2023-12-11T11:06:12.203633363Z] Gathering output for process 2550961 [2023-12-11T11:06:12.285826294Z] Waiting for completion for process 2550961 [2023-12-11T11:06:14.361912745Z] Waiting for completion finished for process 2550961 testExecuteProcessStdout [2023-12-11T11:06:14.398685794Z] Gathering output for process 2550982 [2023-12-11T11:06:14.399518617Z] Waiting for completion for process 2550982 [2023-12-11T11:06:16.595986889Z] Waiting for completion finished for process 2550982 testNewOutputAnalyzerExit [2023-12-11T11:06:16.602218373Z] Gathering output for process 2551067 [2023-12-11T11:06:16.603176801Z] Waiting for completion for process 2551067 [2023-12-11T11:06:18.793886942Z] Waiting for completion finished for process 2551067 testNewOutputAnalyzerStdout [2023-12-11T11:06:18.796332176Z] Gathering output for process 2551172 See how the timestamp for "Waiting for completion for process" becomes "earlier" in the "executeProcess" cases. - Commit messages: - Whitespace cleanups - 8321718: ProcessTools.executeProcess calls waitFor before logging Changes: https://git.openjdk.org/jdk/pull/17052/files Webrev: https://webrevs.openjdk.org/?repo=jdk=17052=00 Issue: https://bugs.openjdk.org/browse/JDK-8321718 Stats: 159 lines in 4 files changed: 145 ins; 11 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/17052.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17052/head:pull/17052 PR: https://git.openjdk.org/jdk/pull/17052
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
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
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
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
Re: RFR: 8321163: [test] OutputAnalyzer.getExitValue() unnecessarily logs even when process has already completed [v3]
On Wed, 6 Dec 2023 01:56:46 GMT, David Holmes wrote: > FWIW exitCode out numbers exitValue in source code 3:1 (and > 5:1 in test > code). That might be the case, but both the called function returning the value and the function we are changing uses the name exitValue. It seems irrelevant that other places in the code uses exitCode. - PR Comment: https://git.openjdk.org/jdk/pull/16919#issuecomment-1842489162
Re: RFR: 8321163: [test] OutputAnalyzer.getExitValue() unnecessarily logs even when process has already completed [v3]
On Fri, 1 Dec 2023 12:44:17 GMT, Jaikiran Pai wrote: >> Can I please get a review for this change to the test library's >> `OutputAnalyzer` class, which proposes to remove some unnecessary logging >> from the `getExitValue()` call? >> >> As noted in https://bugs.openjdk.org/browse/JDK-8321163, right now this >> method logs: >> >> >> [2023-11-24T11:47:54.557561Z] Waiting for completion for process 24909 >> [2023-11-24T11:47:54.557873Z] Waiting for completion finished for process >> 24909 >> >> >> even when the process has already completed and the exit value already >> known. The change in this PR makes it such that if the exit value is >> available then we no longer log this (nor call `process.waitFor()`). >> >> No new tests have been added given the nature of this change. tier1, tier2 >> and tier3 tests continue to pass with this change. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > remove micro optimization Looks good! - Marked as reviewed by stefank (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16919#pullrequestreview-1759744755
Re: RFR: 8321163: [test] OutputAnalyzer.getExitValue() unnecessarily logs even when process has already completed [v2]
On Fri, 1 Dec 2023 11:14:04 GMT, Jaikiran Pai wrote:
>> test/lib/jdk/test/lib/process/OutputBuffer.java line 150:
>>
>>> 148: @Override
>>> 149: public int getExitValue() {
>>> 150: Integer exitCode = this.processExitCode;
>>
>> Do we really need the local `exitCode` variable? Even if another multiple
>> threads write to processExitCode, I expect them to all write a non-null
>> Integer.
>
> Hello Stefan, this is just for a tiny optimization to prevent multiple reads
> (in the same thread) on the `volatile` field `processExitCode` in this method.
I don't think such an optimization is needed here. This is not
performance-critical code.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16919#discussion_r1412028528
Re: RFR: 8321163: [test] OutputAnalyzer.getExitValue() unnecessarily logs even when process has already completed
On Fri, 1 Dec 2023 11:09:30 GMT, Stefan Karlsson wrote:
>> Can I please get a review for this change to the test library's
>> `OutputAnalyzer` class, which proposes to remove some unnecessary logging
>> from the `getExitValue()` call?
>>
>> As noted in https://bugs.openjdk.org/browse/JDK-8321163, right now this
>> method logs:
>>
>>
>> [2023-11-24T11:47:54.557561Z] Waiting for completion for process 24909
>> [2023-11-24T11:47:54.557873Z] Waiting for completion finished for process
>> 24909
>>
>>
>> even when the process has already completed and the exit value already
>> known. The change in this PR makes it such that if the exit value is
>> available then we no longer log this (nor call `process.waitFor()`).
>>
>> No new tests have been added given the nature of this change. tier1, tier2
>> and tier3 tests continue to pass with this change.
>
> test/lib/jdk/test/lib/process/OutputBuffer.java line 158:
>
>> 156: boolean aborted = true;
>> 157: try {
>> 158: this.processExitCode = exitCode = p.waitFor();
>
> According to the `waitFor` javadocs it returns the "exit value" and this
> function is named `getExitValue()`. I propose that we rename
> `processExitCode` to `exitValue` (alt. `processExitValue`).
With the earlier suggestion, this would become:
exitValue = p.waitFor();
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16919#discussion_r1411949002
Re: RFR: 8321163: [test] OutputAnalyzer.getExitValue() unnecessarily logs even when process has already completed
On Fri, 1 Dec 2023 09:48:23 GMT, Jaikiran Pai wrote:
> Can I please get a review for this change to the test library's
> `OutputAnalyzer` class, which proposes to remove some unnecessary logging
> from the `getExitValue()` call?
>
> As noted in https://bugs.openjdk.org/browse/JDK-8321163, right now this
> method logs:
>
>
> [2023-11-24T11:47:54.557561Z] Waiting for completion for process 24909
> [2023-11-24T11:47:54.557873Z] Waiting for completion finished for process
> 24909
>
>
> even when the process has already completed and the exit value already known.
> The change in this PR makes it such that if the exit value is available then
> we no longer log this (nor call `process.waitFor()`).
>
> No new tests have been added given the nature of this change. tier1, tier2
> and tier3 tests continue to pass with this change.
Great find and nice to get rid of the extra logging!
test/lib/jdk/test/lib/process/OutputBuffer.java line 150:
> 148: @Override
> 149: public int getExitValue() {
> 150: Integer exitCode = this.processExitCode;
Do we really need the local `exitCode` variable? Even if another multiple
threads write to processExitCode, I expect them to all write a non-null Integer.
test/lib/jdk/test/lib/process/OutputBuffer.java line 158:
> 156: boolean aborted = true;
> 157: try {
> 158: this.processExitCode = exitCode = p.waitFor();
According to the `waitFor` javadocs it returns the "exit value" and this
function is named `getExitValue()`. I propose that we rename `processExitCode`
to `exitValue` (alt. `processExitValue`).
-
PR Review: https://git.openjdk.org/jdk/pull/16919#pullrequestreview-1759560718
PR Review Comment: https://git.openjdk.org/jdk/pull/16919#discussion_r1411946205
PR Review Comment: https://git.openjdk.org/jdk/pull/16919#discussion_r1411948365
Re: RFR: 8320699: Add parameter to skip progress logging of OutputAnalyzer
On Thu, 30 Nov 2023 12:19:33 GMT, Jaikiran Pai wrote: > Hello Stefan, > > > The test were I want to use this is a long-running stress test that is > > known to be good at shaking out JVM hangs. It has been written to provide > > its own output about spawned processes and what they are doing, and have > > that info written to appropriate files. For me, the OutputAnalyzer logging > > just adds redundant output to the streams where I don't want the > > information. > > Thank you for that context. What you are then proposing is for a way to reuse > this utility class in specific tests, like the one you are working on, > without needing the progress logs. So the idea isn't really to use this new > API in all newly developed tests which use this `ProcessTools` library. Plus > the implementation in this PR, retains the existing behaviour of logging the > progress by default, which is a good thing. Yes, you summarize this well. > > Given that context, I don't have any objection in introducing this > enhancemnet. During future reviews, I think, it wouldn't be too difficult to > catch misuses of this new API in newer tests where the progress logs > shouldn't be disabled. Yes, I agree. It would be quite obvious if someone tries to turn this off. > > As for whether this "config" should be externalized - I think it shouldn't > be, since whether or not the progress logs will be excessive and too > distracting (and thus need to be disabled) would be known at test development > time itself. I agree. > The copyright year on OutputBuffer.java will need an update. Fixed. Thanks for taking a look at this! - PR Comment: https://git.openjdk.org/jdk/pull/16807#issuecomment-1834103958
Re: RFR: 8320699: Add parameter to skip progress logging of OutputAnalyzer [v2]
> Tests using ProcessTools.executeProcess gets the following output written to > stdout: > [2023-11-24T09:58:16.797540608Z] Gathering output for process 2517117 > [2023-11-24T09:58:23.070781345Z] Waiting for completion for process 2517117 > [2023-11-24T09:58:23.071045055Z] Waiting for completion finished for process > 2517117 > > This might be good for some use cases and debugging, but some tests spawns a > large number of processes and for those this output fills up the log files. > > I propose that we add a way to turn of this output for tests where we find > this output to be too noisy. Stefan Karlsson has updated the pull request incrementally with one additional commit since the last revision: Update OutputBuffer.java copyright years - Changes: - all: https://git.openjdk.org/jdk/pull/16807/files - new: https://git.openjdk.org/jdk/pull/16807/files/d5b13dce..d0091eb6 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16807=01 - incr: https://webrevs.openjdk.org/?repo=jdk=16807=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/16807.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16807/head:pull/16807 PR: https://git.openjdk.org/jdk/pull/16807
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
