Re: RFR: 8337418: Fix -Wzero-as-null-pointer-constant warnings in prims code
On Tue, 30 Jul 2024 06:54:04 GMT, Kim Barrett wrote:
>> src/hotspot/share/prims/jni.cpp line 1151:
>>
>>> 1149: \
>>> 1150: EntryProbe; \
>>> 1151: ResultType ret{}; \
>>
>> This looks bogus. ResultType is just a macro variable and could be a
>> primitive type. ?? Does the local need initializing?
>
> This is value-initialization syntax. Value-initialization of a primitive
> type is zero-initialization.
>
> However, I think we don't need the local variable at all. Here and in the
> other 5(?) similar places, rather than
>
> ResultType ret{};
> ...
> ret = jvalue.get_##ResultType();
> return ret;
>
> I think we could just have
>
> ...
> return jvalue.get_##ResultType();
Looks like eliminating the variable doesn't work. It gets used in a
`DT_RETURN_MARK_FOR` form, which
needs the address of the return value. That address is obtained using a
reference. Taking a reference
to an uninitialized variable is (I think) okay, so long as one doesn't attempt
to use the uninitialized value.
But then the assignment could be problematic if it's uninitialized and the
assignment operator is non-trivial.
I expect the compiler will optimize away a trivial zero initialization if it's
not needed. So ensuring it is
value-initialized seems like the cleanest thing to do.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20385#discussion_r1696441217
Re: RFR: 8337418: Fix -Wzero-as-null-pointer-constant warnings in prims code
On Tue, 30 Jul 2024 04:12:33 GMT, Kim Barrett wrote: > Please review this change that removes some uses of literal 0 as a null > pointer constant in prims code. > > Testing: mach5 tier1 Okay - looks good. Thanks. - Marked as reviewed by dholmes (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/20385#pullrequestreview-2206930451
Re: RFR: 8337418: Fix -Wzero-as-null-pointer-constant warnings in prims code
On Tue, 30 Jul 2024 07:16:21 GMT, Kim Barrett wrote:
>> This is value-initialization syntax. Value-initialization of a primitive
>> type is zero-initialization.
>>
>> However, I think we don't need the local variable at all. Here and in the
>> other 5(?) similar places, rather than
>>
>> ResultType ret{};
>> ...
>> ret = jvalue.get_##ResultType();
>> return ret;
>>
>> I think we could just have
>>
>> ...
>> return jvalue.get_##ResultType();
>
> Looks like eliminating the variable doesn't work. It gets used in a
> `DT_RETURN_MARK_FOR` form, which
> needs the address of the return value. That address is obtained using a
> reference. Taking a reference
> to an uninitialized variable is (I think) okay, so long as one doesn't
> attempt to use the uninitialized value.
> But then the assignment could be problematic if it's uninitialized and the
> assignment operator is non-trivial.
> I expect the compiler will optimize away a trivial zero initialization if
> it's not needed. So ensuring it is
> value-initialized seems like the cleanest thing to do.
One day I will remember what this syntax is and does.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20385#discussion_r1696496369
Re: RFR: 8334492: DiagnosticCommands (jcmd) should accept %p in output filenames and substitute PID [v17]
On Mon, 29 Jul 2024 19:08:17 GMT, Sonia Zaldana Calles
wrote:
>> Hi all,
>>
>> This PR addresses [8334492](https://bugs.openjdk.org/browse/JDK-8334492)
>> enabling jcmd diagnostic commands that issue an output file to accept the
>> `%p` pattern in the file name and substitute it for the PID.
>>
>> This PR addresses the following diagnostic commands:
>> - [x] Compiler.perfmap
>> - [x] GC.heap_dump
>> - [x] System.dump_map
>> - [x] Thread.dump_to_file
>> - [x] VM.cds
>>
>> Note that some jcmd diagnostic commands already enable this functionality
>> (`JFR.configure, JFR.dump, JFR.start and JFR.stop`).
>>
>> I propose opening a separate issue to track updating the man page similarly
>> to how it’s done for the JFR diagnostic commands. For example,
>>
>>
>> filename (Optional) Name of the file to which the flight recording
>> data is
>>written when the recording is stopped. If no filename is
>> given, a
>>filename is generated from the PID and the current date
>> and is
>>placed in the directory where the process was started. The
>>filename may also be a directory in which case, the
>> filename is
>>generated from the PID and the current date in the
>> specified
>>directory. (STRING, no default value)
>>
>>Note: If a filename is given, '%p' in the filename will be
>>replaced by the PID, and '%t' will be replaced by the
>> time in
>>'_MM_dd_HH_mm_ss' format.
>>
>>
>> Unfortunately, per [8276265](https://bugs.openjdk.org/browse/JDK-8276265),
>> sources for the jcmd manpage remain in Oracle internal repos so this PR
>> can’t address that.
>>
>> Testing:
>>
>> - [x] Added test case passes.
>> - [x] Modified existing VM.cds tests to also check for `%p` filenames.
>>
>> Looking forward to your comments and addressing any diagnostic commands I
>> might have missed (if any).
>>
>> Cheers,
>> Sonia
>
> Sonia Zaldana Calles has updated the pull request incrementally with one
> additional commit since the last revision:
>
> last lingering change
Thanks Sonia, and thanks Thomas!
I did just see a poblem with DumpPerfMapAtExit that I didn't notice before.
When -XX:+DumpPerfMapAtExit causes a call to CodeCache::write_perf_map, there's
now no %p substitution so /tmp/perf-%p.map gets created.
We all hate duplication but CodeCache::write_perf_map has two very different
callers. It could do something like this (feel free to adjust/correct/do
something else):
src/hotspot/share/code/codeCache.cpp
#ifdef LINUX
void CodeCache::write_perf_map(const char* filename, outputStream* st) {
MutexLocker mu(CodeCache_lock, Mutex::_no_safepoint_check_flag);
+ if (filename == nullptr) {
+st->print_cr("Warning: Not writing perf map as null filename provided.");
+return;
+ }
+ char fname[JVM_MAXPATHLEN];
+ if (strstr(filename, "%p") != nullptr) {
+// Unnecessary if filename contains %%p but will be a rare waste of time:
+if (!Arguments::copy_expand_pid(filename, strlen(filename), fname,
JVM_MAXPATHLEN)) {
+ st->print_cr("Warning: Not writing perf map as substitution failed.");
+ return;
+}
+filename = fname;
+ }
+
fileStream fs(filename, "w");
JVM_MAXPATHLEN will have a lot of slack space there as if it contains %p it
really should be the default filename, so you could go with a lower value.
-
PR Comment: https://git.openjdk.org/jdk/pull/20198#issuecomment-2257986965
Re: RFR: 8337418: Fix -Wzero-as-null-pointer-constant warnings in prims code
On Tue, 30 Jul 2024 04:12:33 GMT, Kim Barrett wrote:
> Please review this change that removes some uses of literal 0 as a null
> pointer constant in prims code.
>
> Testing: mach5 tier1
All right, this looks fine. (I am somewhat allergic to `{}` syntax, but it is
what it is.)
-
Marked as reviewed by shade (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/20385#pullrequestreview-2207285678
Re: RFR: 8335610: DiagnosticFramework: CmdLine::is_executable() correction
On Wed, 3 Jul 2024 13:58:51 GMT, Kevin Walls wrote: > CmdLine::is_executable() looks wrong, surely an empty line is not executable. > > With this change, in DCmd::parse_and_execute() we will avoid needlessly > entering the code block to try and execute the command. > > DCmd tests all good: > make images test TEST="test/hotspot/jtreg/serviceability/dcmd > test/jdk/sun/tools/jcmd" Thanks for the reviews, integrating. - PR Comment: https://git.openjdk.org/jdk/pull/20006#issuecomment-2258044360
Integrated: 8335610: DiagnosticFramework: CmdLine::is_executable() correction
On Wed, 3 Jul 2024 13:58:51 GMT, Kevin Walls wrote: > CmdLine::is_executable() looks wrong, surely an empty line is not executable. > > With this change, in DCmd::parse_and_execute() we will avoid needlessly > entering the code block to try and execute the command. > > DCmd tests all good: > make images test TEST="test/hotspot/jtreg/serviceability/dcmd > test/jdk/sun/tools/jcmd" This pull request has now been integrated. Changeset: 0325ab8d Author:Kevin Walls URL: https://git.openjdk.org/jdk/commit/0325ab8d2353f29ac40ff4b028cbc29bff40c59b Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod 8335610: DiagnosticFramework: CmdLine::is_executable() correction Reviewed-by: dholmes, jsjolen - PR: https://git.openjdk.org/jdk/pull/20006
Re: RFR: 8331015: Obsolete -XX:+UseNotificationThread
On Tue, 30 Jul 2024 01:57:33 GMT, Alex Menkov wrote: > Obsolete UseNotificationThread flag which was deprecated in JDK 23. > > Testing: tier1..tier5 Looks good - Marked as reviewed by kevinw (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/20381#pullrequestreview-2207376511
Re: RFR: 8337331: crash: pinned virtual thread will lead to jvm crash when running with the javaagent option [v4]
On Tue, 30 Jul 2024 06:46:20 GMT, Jiawei Tang wrote:
>> I add the testcase which can reproduce the crash. I hope that I could get
>> some advise if the codes need changing.
>
> Jiawei Tang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> refactor testcase and change the location of fix codes
Changes requested by sspitsyn (Reviewer).
src/hotspot/share/prims/jvmtiExport.cpp line 1098:
> 1096: if (JavaThread::current()->is_in_any_VTMS_transition()) {
> 1097: return false; // no events should be posted if thread is in any
> VTMS transition
> 1098: }
Sorry, I was not clear the 3 lines above 1093-1095 had to be replaced with new
lines 1096-1098.
The check for `is_in_any_VTMS_transition()` includes the checks
`is_in_tmp_VTMS_transition()`.
-
PR Review: https://git.openjdk.org/jdk/pull/20373#pullrequestreview-2207409392
PR Review Comment: https://git.openjdk.org/jdk/pull/20373#discussion_r1696791636
Re: RFR: 8337331: crash: pinned virtual thread will lead to jvm crash when running with the javaagent option [v3]
On Tue, 30 Jul 2024 06:46:38 GMT, Alan Bateman wrote: > > Can you convert the test to use .cpp instead of .c as well? > or maybe it could use VThreadPinner which allows calling through a native > frame for tests like this. This is a good suggestion, I was also thinking about it. An example can be found in the test: `test/hotspot/jtreg/serviceability/jvmti/vthread/GetThreadState/GetThreadStateTest.java` - PR Comment: https://git.openjdk.org/jdk/pull/20373#issuecomment-2258119426
Re: RFR: 8337418: Fix -Wzero-as-null-pointer-constant warnings in prims code
On Tue, 30 Jul 2024 04:12:33 GMT, Kim Barrett wrote: > Please review this change that removes some uses of literal 0 as a null > pointer constant in prims code. > > Testing: mach5 tier1 Looks Good! - Marked as reviewed by jwaters (Committer). PR Review: https://git.openjdk.org/jdk/pull/20385#pullrequestreview-2207470079
Re: RFR: 8337418: Fix -Wzero-as-null-pointer-constant warnings in prims code
On Tue, 30 Jul 2024 06:54:10 GMT, Kim Barrett wrote:
>> src/hotspot/share/prims/methodHandles.cpp line 439:
>>
>>> 437: default:
>>> 438: fatal("unexpected intrinsic id: %d %s", vmIntrinsics::as_int(iid),
>>> vmIntrinsics::name_at(iid));
>>> 439: return 0;
>>
>> Do we no longer need these returns after `fatal` to keep compilers happy?
>
> Now that we have, and are using, `[[noreturn]]` on all platforms, we no
> longer need that dead code.
I'll admit, I do prefer having a return to end all possible control flows in a
non void method, but oh well
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20385#discussion_r1696829614
Re: RFR: 8337031: Improvements to CompilationMemoryStatistic [v2]
On Tue, 30 Jul 2024 05:18:17 GMT, Thomas Stuefe wrote:
>> Ashutosh Mehra has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Address review comments by Thomas S.
>>
>> Signed-off-by: Ashutosh Mehra
>
> src/hotspot/share/compiler/compilationMemoryStatistic.hpp line 40:
>
>> 38:
>> 39: // Helper class to wrap the array of arena tags for easier processing
>> 40: class ArenaTagsCounter {
>
> Sorry for being a stickler for precise names, but I would like plural for
> counters here - it is not a single counter, its a series/vector/array of
> counters.
> Any of these work for me: ArenaCountersByTag - ArenaCountersByTagVector -
> ArenaTagCounterVector - ArenaTagCounters
I am pretty bad in naming things, so I welcome these suggestions. I will go
with ArenaCountersByTag.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20304#discussion_r1696973723
Re: RFR: 8334492: DiagnosticCommands (jcmd) should accept %p in output filenames and substitute PID [v18]
> Hi all, > > This PR addresses [8334492](https://bugs.openjdk.org/browse/JDK-8334492) > enabling jcmd diagnostic commands that issue an output file to accept the > `%p` pattern in the file name and substitute it for the PID. > > This PR addresses the following diagnostic commands: > - [x] Compiler.perfmap > - [x] GC.heap_dump > - [x] System.dump_map > - [x] Thread.dump_to_file > - [x] VM.cds > > Note that some jcmd diagnostic commands already enable this functionality > (`JFR.configure, JFR.dump, JFR.start and JFR.stop`). > > I propose opening a separate issue to track updating the man page similarly > to how it’s done for the JFR diagnostic commands. For example, > > > filename (Optional) Name of the file to which the flight recording > data is >written when the recording is stopped. If no filename is > given, a >filename is generated from the PID and the current date > and is >placed in the directory where the process was started. The >filename may also be a directory in which case, the > filename is >generated from the PID and the current date in the > specified >directory. (STRING, no default value) > >Note: If a filename is given, '%p' in the filename will be >replaced by the PID, and '%t' will be replaced by the time > in >'_MM_dd_HH_mm_ss' format. > > > Unfortunately, per [8276265](https://bugs.openjdk.org/browse/JDK-8276265), > sources for the jcmd manpage remain in Oracle internal repos so this PR can’t > address that. > > Testing: > > - [x] Added test case passes. > - [x] Modified existing VM.cds tests to also check for `%p` filenames. > > Looking forward to your comments and addressing any diagnostic commands I > might have missed (if any). > > Cheers, > Sonia Sonia Zaldana Calles has updated the pull request incrementally with one additional commit since the last revision: Fixing invocation outside of jcmd - Changes: - all: https://git.openjdk.org/jdk/pull/20198/files - new: https://git.openjdk.org/jdk/pull/20198/files/ceb96eb9..564349e3 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=20198&range=17 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=20198&range=16-17 Stats: 12 lines in 2 files changed: 11 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/20198.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20198/head:pull/20198 PR: https://git.openjdk.org/jdk/pull/20198
Re: RFR: 8334492: DiagnosticCommands (jcmd) should accept %p in output filenames and substitute PID [v18]
On Tue, 30 Jul 2024 14:33:10 GMT, Sonia Zaldana Calles wrote: >> Hi all, >> >> This PR addresses [8334492](https://bugs.openjdk.org/browse/JDK-8334492) >> enabling jcmd diagnostic commands that issue an output file to accept the >> `%p` pattern in the file name and substitute it for the PID. >> >> This PR addresses the following diagnostic commands: >> - [x] Compiler.perfmap >> - [x] GC.heap_dump >> - [x] System.dump_map >> - [x] Thread.dump_to_file >> - [x] VM.cds >> >> Note that some jcmd diagnostic commands already enable this functionality >> (`JFR.configure, JFR.dump, JFR.start and JFR.stop`). >> >> I propose opening a separate issue to track updating the man page similarly >> to how it’s done for the JFR diagnostic commands. For example, >> >> >> filename (Optional) Name of the file to which the flight recording >> data is >>written when the recording is stopped. If no filename is >> given, a >>filename is generated from the PID and the current date >> and is >>placed in the directory where the process was started. The >>filename may also be a directory in which case, the >> filename is >>generated from the PID and the current date in the >> specified >>directory. (STRING, no default value) >> >>Note: If a filename is given, '%p' in the filename will be >>replaced by the PID, and '%t' will be replaced by the >> time in >>'_MM_dd_HH_mm_ss' format. >> >> >> Unfortunately, per [8276265](https://bugs.openjdk.org/browse/JDK-8276265), >> sources for the jcmd manpage remain in Oracle internal repos so this PR >> can’t address that. >> >> Testing: >> >> - [x] Added test case passes. >> - [x] Modified existing VM.cds tests to also check for `%p` filenames. >> >> Looking forward to your comments and addressing any diagnostic commands I >> might have missed (if any). >> >> Cheers, >> Sonia > > Sonia Zaldana Calles has updated the pull request incrementally with one > additional commit since the last revision: > > Fixing invocation outside of jcmd OK great, thanks for that last update, nothing more to say! 8-) I will do the man page update and get it out for review soon. - Marked as reviewed by kevinw (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/20198#pullrequestreview-2207924716 PR Comment: https://git.openjdk.org/jdk/pull/20198#issuecomment-2258543294
Re: RFR: 8337031: Improvements to CompilationMemoryStatistic [v3]
> Some minor improvements to CompilationMemoryStatistic. More details are in > [JDK-8337031](https://bugs.openjdk.org/browse/JDK-8337031) > > Testing: > test/hotspot/jtreg/compiler/print/CompileCommandPrintMemStat.java > > test/hotspot/jtreg/serviceability/dcmd/compiler/CompilerMemoryStatisticTest.java Ashutosh Mehra has updated the pull request incrementally with one additional commit since the last revision: Rename ArenaTagsCounter to ArenaCountersByTag Signed-off-by: Ashutosh Mehra - Changes: - all: https://git.openjdk.org/jdk/pull/20304/files - new: https://git.openjdk.org/jdk/pull/20304/files/008ac6b9..70762110 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=20304&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=20304&range=01-02 Stats: 7 lines in 2 files changed: 0 ins; 0 del; 7 mod Patch: https://git.openjdk.org/jdk/pull/20304.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20304/head:pull/20304 PR: https://git.openjdk.org/jdk/pull/20304
Re: RFR: 8337031: Improvements to CompilationMemoryStatistic
On Wed, 24 Jul 2024 10:45:05 GMT, Thomas Stuefe wrote: >> Some minor improvements to CompilationMemoryStatistic. More details are in >> [JDK-8337031](https://bugs.openjdk.org/browse/JDK-8337031) >> >> Testing: >> test/hotspot/jtreg/compiler/print/CompileCommandPrintMemStat.java >> >> test/hotspot/jtreg/serviceability/dcmd/compiler/CompilerMemoryStatisticTest.java > > I plan to look at this later this week. @tstuefe renamed ArenaTagsCounter to ArenaCountersByTag - PR Comment: https://git.openjdk.org/jdk/pull/20304#issuecomment-2258563618
Re: RFR: 8337031: Improvements to CompilationMemoryStatistic [v3]
On Tue, 30 Jul 2024 15:02:51 GMT, Ashutosh Mehra wrote: >> Some minor improvements to CompilationMemoryStatistic. More details are in >> [JDK-8337031](https://bugs.openjdk.org/browse/JDK-8337031) >> >> Testing: >> test/hotspot/jtreg/compiler/print/CompileCommandPrintMemStat.java >> >> test/hotspot/jtreg/serviceability/dcmd/compiler/CompilerMemoryStatisticTest.java > > Ashutosh Mehra has updated the pull request incrementally with one additional > commit since the last revision: > > Rename ArenaTagsCounter to ArenaCountersByTag > > Signed-off-by: Ashutosh Mehra Looks reasonable. I submitted our testing to make sure tests passed in our testing. - PR Review: https://git.openjdk.org/jdk/pull/20304#pullrequestreview-2208245618
Re: RFR: 8327114: Attach in Linux may have wrong behaviour when pid == ns_pid (Kubernetes debug container) [v6]
On Tue, 2 Jul 2024 11:07:56 GMT, Sebastian Lövdahl wrote: >> 8327114: Attach in Linux may have wrong behaviour when pid == ns_pid >> (Kubernetes debug container) > > Sebastian Lövdahl has updated the pull request incrementally with one > additional commit since the last revision: > > Clarify PID 1 check with comment Waiting for another reviewer to have time to review the PR. - PR Comment: https://git.openjdk.org/jdk/pull/19055#issuecomment-2258866242
Integrated: 8334492: DiagnosticCommands (jcmd) should accept %p in output filenames and substitute PID
On Tue, 16 Jul 2024 16:25:50 GMT, Sonia Zaldana Calles wrote: > Hi all, > > This PR addresses [8334492](https://bugs.openjdk.org/browse/JDK-8334492) > enabling jcmd diagnostic commands that issue an output file to accept the > `%p` pattern in the file name and substitute it for the PID. > > This PR addresses the following diagnostic commands: > - [x] Compiler.perfmap > - [x] GC.heap_dump > - [x] System.dump_map > - [x] Thread.dump_to_file > - [x] VM.cds > > Note that some jcmd diagnostic commands already enable this functionality > (`JFR.configure, JFR.dump, JFR.start and JFR.stop`). > > I propose opening a separate issue to track updating the man page similarly > to how it’s done for the JFR diagnostic commands. For example, > > > filename (Optional) Name of the file to which the flight recording > data is >written when the recording is stopped. If no filename is > given, a >filename is generated from the PID and the current date > and is >placed in the directory where the process was started. The >filename may also be a directory in which case, the > filename is >generated from the PID and the current date in the > specified >directory. (STRING, no default value) > >Note: If a filename is given, '%p' in the filename will be >replaced by the PID, and '%t' will be replaced by the time > in >'_MM_dd_HH_mm_ss' format. > > > Unfortunately, per [8276265](https://bugs.openjdk.org/browse/JDK-8276265), > sources for the jcmd manpage remain in Oracle internal repos so this PR can’t > address that. > > Testing: > > - [x] Added test case passes. > - [x] Modified existing VM.cds tests to also check for `%p` filenames. > > Looking forward to your comments and addressing any diagnostic commands I > might have missed (if any). > > Cheers, > Sonia This pull request has now been integrated. Changeset: f5c9e8f1 Author:Sonia Zaldana Calles URL: https://git.openjdk.org/jdk/commit/f5c9e8f1229f3aa00743119a2dee3e15d57048d8 Stats: 173 lines in 11 files changed: 134 ins; 16 del; 23 mod 8334492: DiagnosticCommands (jcmd) should accept %p in output filenames and substitute PID Reviewed-by: kevinw, stuefe, lmesnik - PR: https://git.openjdk.org/jdk/pull/20198
Re: RFR: 8311990: Two JDI tests may interfere with each other [v2]
> "Attach fails" scenarios (debuggee starts listening and debugger is expected > to fail trying to attach) sometimes interfere with other JDI tests (so > JdwpNetProps.java test or other JDI test or both fail). > The fix disables the scenarios to remove noise in the CI. Alex Menkov has updated the pull request incrementally with one additional commit since the last revision: add sys.prop to enable negative testing - Changes: - all: https://git.openjdk.org/jdk/pull/20362/files - new: https://git.openjdk.org/jdk/pull/20362/files/737b0869..0378fc48 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=20362&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=20362&range=00-01 Stats: 5 lines in 2 files changed: 2 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/20362.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20362/head:pull/20362 PR: https://git.openjdk.org/jdk/pull/20362
Re: RFR: 8337031: Improvements to CompilationMemoryStatistic [v3]
On Tue, 30 Jul 2024 15:02:51 GMT, Ashutosh Mehra wrote: >> Some minor improvements to CompilationMemoryStatistic. More details are in >> [JDK-8337031](https://bugs.openjdk.org/browse/JDK-8337031) >> >> Testing: >> test/hotspot/jtreg/compiler/print/CompileCommandPrintMemStat.java >> >> test/hotspot/jtreg/serviceability/dcmd/compiler/CompilerMemoryStatisticTest.java > > Ashutosh Mehra has updated the pull request incrementally with one additional > commit since the last revision: > > Rename ArenaTagsCounter to ArenaCountersByTag > > Signed-off-by: Ashutosh Mehra My testing passed. - Marked as reviewed by kvn (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/20304#pullrequestreview-2208698602
RFR: 8335836: serviceability/jvmti/StartPhase/AllowedFunctions/AllowedFunctions.java fails with unexpected exit code: 112
The test has been fixed to: - add a guard against JVMTI_ERROR_WRONG_PHASE - replace `exit(err)` with `abort()` in the `check_jvmti_error()` The JVMTI `VMDeath` event is enabled and a `RawMonitor` is introduced to prevent JVMTI_ERROR_WRONG_PHASE in the `ClassPrepare` events. Testing: - run the test AllowedFunctions.java locally - TBD: submit a mach5 run for fixed test - Commit messages: - 8335836: serviceability/jvmti/StartPhase/AllowedFunctions/AllowedFunctions.java fails with unexpected exit code: 112 Changes: https://git.openjdk.org/jdk/pull/20397/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=20397&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8335836 Stats: 32 lines in 1 file changed: 29 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/20397.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20397/head:pull/20397 PR: https://git.openjdk.org/jdk/pull/20397
Re: RFR: 8335836: serviceability/jvmti/StartPhase/AllowedFunctions/AllowedFunctions.java fails with unexpected exit code: 112
On Tue, 30 Jul 2024 23:13:23 GMT, Serguei Spitsyn wrote: > The test has been fixed to: > - add a guard against JVMTI_ERROR_WRONG_PHASE > - replace `exit(err)` with `abort()` in the `check_jvmti_error()` > > The JVMTI `VMDeath` event is enabled and a `RawMonitor` is introduced to > prevent JVMTI_ERROR_WRONG_PHASE in the `ClassPrepare` events. > > Testing: > - run the test AllowedFunctions.java locally > - TBD: submit a mach5 run for fixed test Converting the native agent to C++ would simplify the test and allow to use utility functions from the `jvmti_common.hpp`. But I wanted to make the WRONG_PHASE related fix to be clear for reviewers. So, this conversion can be done as a separate step or as a part of a separate effort covered by an enhancement. - PR Comment: https://git.openjdk.org/jdk/pull/20397#issuecomment-2259356963
Integrated: 8337031: Improvements to CompilationMemoryStatistic
On Tue, 23 Jul 2024 21:46:50 GMT, Ashutosh Mehra wrote: > Some minor improvements to CompilationMemoryStatistic. More details are in > [JDK-8337031](https://bugs.openjdk.org/browse/JDK-8337031) > > Testing: > test/hotspot/jtreg/compiler/print/CompileCommandPrintMemStat.java > > test/hotspot/jtreg/serviceability/dcmd/compiler/CompilerMemoryStatisticTest.java This pull request has now been integrated. Changeset: e63d0165 Author:Ashutosh Mehra URL: https://git.openjdk.org/jdk/commit/e63d01654e0b702b3a8c0c4de97a6bb6893fbd1f Stats: 182 lines in 6 files changed: 84 ins; 27 del; 71 mod 8337031: Improvements to CompilationMemoryStatistic Reviewed-by: kvn, stuefe - PR: https://git.openjdk.org/jdk/pull/20304
Re: RFR: 8337031: Improvements to CompilationMemoryStatistic
On Wed, 24 Jul 2024 10:45:05 GMT, Thomas Stuefe wrote: >> Some minor improvements to CompilationMemoryStatistic. More details are in >> [JDK-8337031](https://bugs.openjdk.org/browse/JDK-8337031) >> >> Testing: >> test/hotspot/jtreg/compiler/print/CompileCommandPrintMemStat.java >> >> test/hotspot/jtreg/serviceability/dcmd/compiler/CompilerMemoryStatisticTest.java > > I plan to look at this later this week. thanks @tstuefe @vnkozlov for reviewing and testing - PR Comment: https://git.openjdk.org/jdk/pull/20304#issuecomment-2259466393
Re: RFR: 8337418: Fix -Wzero-as-null-pointer-constant warnings in prims code
On Tue, 30 Jul 2024 11:51:37 GMT, Julian Waters wrote: >> Now that we have, and are using, `[[noreturn]]` on all platforms, we no >> longer need that dead code. > > I'll admit, I do prefer having a return to end all possible control flows in > a non void method, but oh well I would rather it not look like it can return null (or some other manufactured "default") when it actually can't. - PR Review Comment: https://git.openjdk.org/jdk/pull/20385#discussion_r1697794269
Re: RFR: 8337418: Fix -Wzero-as-null-pointer-constant warnings in prims code
On Tue, 30 Jul 2024 10:19:59 GMT, Aleksey Shipilev wrote:
> All right, this looks fine. (I am somewhat allergic to `{}` syntax, but it is
> what it is.)
The hoops one had to go through to get guaranteed value-initialization before
we had brace initialization are really
not pretty. See
https://www.boost.org/doc/libs/1_85_0/libs/utility/doc/html/utility/utilities/value_init.html
and its associated implementation.
It might help if we were to commit to using direct brace initialization
whenever appropriate, but that hasn't happened.
-
PR Comment: https://git.openjdk.org/jdk/pull/20385#issuecomment-2259500102
Re: [jdk23] RFR: 8334167: Test java/lang/instrument/NativeMethodPrefixApp.java timed out
On Thu, 25 Jul 2024 15:13:29 GMT, Jaikiran Pai wrote: > Can I please get a review of this test-only backport of the fix that we did > in master branch a few weeks back? > > This isn't a clean backport, there were trivial merge issues in the > `NativeMethodPrefixApp` test class, which I have resolved manually. The > merged content matches with what we have in master branch. The original fix > was done in https://github.com/openjdk/jdk/pull/20154 and we have seen this > test fail since it was integrated. > > Backporting this to jdk23 branch will provide test stability in that branch > where currently this test occasionally times out. Thank you David for the review. Given that this is the serviceability area, can I get a second review please? - PR Comment: https://git.openjdk.org/jdk/pull/20333#issuecomment-2259632800
