On Tue, 8 Apr 2025 11:43:20 GMT, Kevin Walls <[email protected]> wrote:
>> This is a long-standing oversight: HeapDumpPath does not recognise %p for
>> pid expansion.
>> The default filename uses a pid (e.g. java_pid1676937.hprof) but
>> HeapDumpPath does not.
>> It has always done a manual "root plus pid plus extension" on the default
>> filename only, and
>> should move to using Argument::copy_expand_pid() like we do with other such
>> filenames.
>>
>>
>> We also assumed the default filename is not a directory (which is very very
>> likely, but doesn't have to be true).
>
> Kevin Walls has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - length checking update
> - Chris feedback
Marked as reviewed by lmesnik (Reviewer).
test/hotspot/jtreg/runtime/ErrorHandling/TestHeapDumpOnOutOfMemoryError.java
line 100:
> 98: output.shouldContain("Dumping heap to " + type + ".hprof");
> 99: File dump = new File(heapdumpFilename);
> 100: Asserts.assertTrue(dump.exists() && dump.isFile(), "Could not
> find dump file " + dump.getAbsolutePath());
I. think you could just update the test to use
heapdumpFilename = type + ".%p.hprof";
we don't need test twice, it is quite expensive.
test/hotspot/jtreg/runtime/ErrorHandling/TestHeapDumpOnOutOfMemoryError.java
line 115:
> 113: output.stdoutShouldNotBeEmpty();
> 114: String actualHeapdumpFilename = type + "." + output.pid() +
> ".hprof";
> 115: output.shouldContain("Dumping heap to " +
> actualHeapdumpFilename);
This better to be something like expectedlHeapdumpFilename and
"Expected heap dump file".
Not very important, but make log cleaner.
-------------
PR Review: https://git.openjdk.org/jdk/pull/24482#pullrequestreview-2751644078
PR Review Comment: https://git.openjdk.org/jdk/pull/24482#discussion_r2034187020
PR Review Comment: https://git.openjdk.org/jdk/pull/24482#discussion_r2034189152
