8330677: Add Per-Compilation memory usage to JFR by tstuefe · Pull Request #18864 · openjdk/jdk

tstuefe · 2024-04-19T13:12:21Z

We have the (opt-in, disabled by default) compiler memory statistics introduced with JDK-8317683.

Since temporary memory usage by compilers can significantly affect process footprint, it would make sense to expose at least the total peak usage per compilation via JFR.

This patch adds "Arena Usage" to CompilationEvent. We now see in JMC how costly a compilation had been. (The cost can get very high, as we have seen just recently again with JDK-8327247 ).

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8330677: Add Per-Compilation memory usage to JFR (Enhancement - P4)

Reviewers

Vladimir Kozlov (@vnkozlov - Reviewer) ⚠️ Review applies to 6829396e
Matthias Baesken (@MBaesken - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/18864/head:pull/18864
$ git checkout pull/18864

Update a local copy of the PR:
$ git checkout pull/18864
$ git pull https://git.openjdk.org/jdk.git pull/18864/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 18864

View PR using the GUI difftool:
$ git pr show -t 18864

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/18864.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2024-04-19T13:13:02Z

👋 Welcome back stuefe! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2024-04-19T13:13:09Z

@tstuefe This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8330677: Add Per-Compilation memory usage to JFR

Reviewed-by: kvn, mbaesken

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 22 new commits pushed to the master branch:

7272939: 8331200: Serial: Remove unused methods in SerialHeap
70d3f22: 8331175: Parallel: Remove VerifyRememberedSets
549bc6a: 8330685: ZGC: share barrier spilling logic
76cda7b: 8331118: Remove Serial includes from space.hpp
766d0b0: 8330960: Serial: Remove SerialFullGC::_total_invocations
4edac34: 8331167: UBSan enabled build fails in adlc on macOS
c615c18: 8330158: C2: Loop strip mining uses ABS with min int
fb63cba: 8330684: ClassFile API runs into StackOverflowError while parsing certain class' bytes
4e5c25e: 8331108: Unused Math.abs call in java.lang.FdLibm.Expm1#compute
16c7dcd: 8330523: Reduce runtime and improve efficiency of KeepAliveTest
... and 12 more: https://git.openjdk.org/jdk/compare/2b7176a55ad0e5c6ba34abba3fe8fc1a411a5e2d...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk · 2024-04-19T13:13:38Z

@tstuefe The following label will be automatically applied to this pull request:

hotspot

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2024-04-21T06:56:14Z

Webrevs

01: Full (ae2d6f8d)
00: Full (6829396e)

tstuefe · 2024-04-22T11:16:02Z

/label hotspot-compiler

openjdk · 2024-04-22T11:16:24Z

@tstuefe
The hotspot-compiler label was successfully added.

tstuefe · 2024-04-22T11:16:50Z

/label remove hotspot

openjdk · 2024-04-22T11:17:31Z

@tstuefe
The hotspot label was successfully removed.

vnkozlov

Looks good.

MBaesken · 2024-04-26T07:05:35Z

Please also check COPYRIGHT years, e.g. compileTask.cpp .
Could you also check if there is an easy way to enhance some existing test for the JFR Compilation event. always good to have at least some basic testing .

MBaesken · 2024-04-26T08:06:14Z

We have the (opt-in, disabled by default)

Might be helpful to add to the Description part of https://bugs.openjdk.org/browse/JDK-8317683 another sentence that describes how to enable the feature.

tstuefe · 2024-04-26T12:37:15Z

Thanks @MBaesken . Please check again to see if I addressed all your issues.

MBaesken · 2024-04-26T13:03:26Z

Regarding src/hotspot/share/jfr/metadata/metadata.xml , could you label it 'Arena Peak Usage' or something like this ? I would like to have it more clear that it is the peak usage.
Did you check my nullptr check related question ?
Otherwise looks okay to me.
It is a bit unfortunate that by default , we have now an 'empty' added field in the JFR Compilation event. But it is what it is; any chances that the compiler mem statistics would be enabled by default in the future, or is this considered too costly ?

tstuefe · 2024-04-26T16:39:15Z

Regarding src/hotspot/share/jfr/metadata/metadata.xml , could you label it 'Arena Peak Usage' or something like this ? I would like to have it more clear that it is the peak usage.

I already find "Arena Usage" to be too long, to be honest. Longer column labels make the JMC table less readable (ideally, one would have a "label," short and descriptive, and a "description," one being the column label, the other, e.g., a tooltip).

And "peak" is really the only option that makes sense here. If you ask someone what they think "usage" means, they will assume its the largest footprint accumulated during compilation over the time span of the compilation, aka peak.

Did you check my nullptr check related question ?

I read through the comments twice and did not find a nullptr related question. Which question?

Otherwise looks okay to me. It is a bit unfortunate that by default , we have now an 'empty' added field in the JFR Compilation event. But it is what it is; any chances that the compiler mem statistics would be enabled by default in the future, or is this considered too costly ?

One synchronization per compilation. So, not that costly, no. It will be enabled by default in debug builds with #18969 since it is implied in memlimit.

MBaesken · 2024-04-29T07:38:35Z

I read through the comments twice and did not find a nullptr related question. Which question?

See compilationMemoryStatistic.cpp .

At some places we check the result for nullptr e.g.

jdk/src/hotspot/share/compiler/compilationMemoryStatistic.cpp

Line 79 in b3bcc49

 const CompileTask* const task = th->task();

tstuefe · 2024-04-29T08:24:29Z

I read through the comments twice and did not find a nullptr related question. Which question?

See compilationMemoryStatistic.cpp .

At some places we check the result for nullptr e.g.

jdk/src/hotspot/share/compiler/compilationMemoryStatistic.cpp

Line 79 in b3bcc49
 const CompileTask* const task = th->task(); 

Yes, it is inconsistent. Allmost all code here (notably anything triggered from start- or end-compilation) are called from the compiler so we run on a compiler thread and in the scope of a ciEnv. So most of the existing nullptr checks are probably not needed.

We may want to make this consistent with subsequent RFEs.

MBaesken · 2024-04-29T10:42:34Z

We may want to make this consistent with subsequent RFEs.

I agree, this can be a follow up.

MBaesken · 2024-04-26T06:49:35Z

src/hotspot/share/compiler/compilationMemoryStatistic.cpp

  CompilerThread* const th = Thread::current()->as_Compiler_thread();
  ArenaStatCounter* const arena_stat = th->arena_stat();
-  const CompilerType ct = th->task()->compiler()->type();
+  CompileTask* const task = th->task();


At some places we check the result for nullptr e.g.

jdk/src/hotspot/share/compiler/compilationMemoryStatistic.cpp

Line 79 in b3bcc49

const CompileTask* const task = th->task();

Is that over cautious or should it better be done ?

MBaesken · 2024-04-26T06:59:42Z

src/hotspot/share/compiler/compileTask.hpp

  const char*          _failure_reason;
  // Specifies if _failure_reason is on the C heap.
  bool                 _failure_reason_on_C_heap;
+  size_t               _arena_bytes;  // peak size of temporary memory during compilation (e.g. node arenas)


Is there a good reason not to name it _peak_arena_bytes when it is always the peak as stated ?

MBaesken · 2024-04-26T07:03:19Z

src/hotspot/share/jfr/metadata/metadata.xml

    <Field type="boolean" name="isOsr" label="On Stack Replacement" />
    <Field type="ulong" contentType="bytes" name="codeSize" label="Compiled Code Size" />
    <Field type="ulong" contentType="bytes" name="inlinedBytes" label="Inlined Code Size" />
+    <Field type="ulong" contentType="bytes" name="arenaBytes" label="Arena Usage" />


Maybe say Peak arena usage, if this is the case as stated above ?
And some info that it is optional / must be enabled to see this data would probably help too.

Again, there is no space for this. "label" gets used as column header. No space to add a descriptive text.

tstuefe · 2024-04-29T10:57:55Z

Thanks @MBaesken . For some reason your remarks only got posted now, but I think we covered all points already.

/integrate

openjdk · 2024-04-29T10:58:08Z

Going to push as commit 151ef5d.
Since your change was applied there have been 22 commits pushed to the master branch:

7272939: 8331200: Serial: Remove unused methods in SerialHeap
70d3f22: 8331175: Parallel: Remove VerifyRememberedSets
549bc6a: 8330685: ZGC: share barrier spilling logic
76cda7b: 8331118: Remove Serial includes from space.hpp
766d0b0: 8330960: Serial: Remove SerialFullGC::_total_invocations
4edac34: 8331167: UBSan enabled build fails in adlc on macOS
c615c18: 8330158: C2: Loop strip mining uses ABS with min int
fb63cba: 8330684: ClassFile API runs into StackOverflowError while parsing certain class' bytes
4e5c25e: 8331108: Unused Math.abs call in java.lang.FdLibm.Expm1#compute
16c7dcd: 8330523: Reduce runtime and improve efficiency of KeepAliveTest
... and 12 more: https://git.openjdk.org/jdk/compare/2b7176a55ad0e5c6ba34abba3fe8fc1a411a5e2d...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-04-29T10:58:14Z

@tstuefe Pushed as commit 151ef5d.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

JDK-8330677-Add-Per-Compilation-memory-usage-to-JFR

6829396

openjdk bot changed the title ~~JDK-8330677: Add Per-Compilation memory usage to JFR~~ 8330677: Add Per-Compilation memory usage to JFR Apr 19, 2024

openjdk bot added the hotspot hotspot-dev@openjdk.org label Apr 19, 2024

tstuefe marked this pull request as ready for review April 21, 2024 06:48

openjdk bot added the rfr Pull request is ready for review label Apr 21, 2024

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Apr 22, 2024

openjdk bot removed the hotspot hotspot-dev@openjdk.org label Apr 22, 2024

vnkozlov approved these changes Apr 23, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Apr 23, 2024

tstuefe added 2 commits April 26, 2024 14:18

merge

09f32f7

Adapt test

ae2d6f8

MBaesken approved these changes Apr 29, 2024

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Apr 29, 2024

openjdk bot closed this Apr 29, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Apr 29, 2024

Conversation

tstuefe commented Apr 19, 2024 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented Apr 19, 2024

Uh oh!

openjdk bot commented Apr 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Apr 19, 2024

Uh oh!

mlbridge bot commented Apr 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

tstuefe commented Apr 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Apr 22, 2024

Uh oh!

tstuefe commented Apr 22, 2024

Uh oh!

openjdk bot commented Apr 22, 2024

Uh oh!

vnkozlov left a comment

Choose a reason for hiding this comment

Uh oh!

MBaesken commented Apr 26, 2024

Uh oh!

MBaesken commented Apr 26, 2024

Uh oh!

tstuefe commented Apr 26, 2024

Uh oh!

MBaesken commented Apr 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tstuefe commented Apr 26, 2024

Uh oh!

MBaesken commented Apr 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tstuefe commented Apr 29, 2024

Uh oh!

MBaesken commented Apr 29, 2024

Uh oh!

MBaesken Apr 26, 2024

Choose a reason for hiding this comment

Uh oh!

MBaesken Apr 26, 2024

Choose a reason for hiding this comment

Uh oh!

MBaesken Apr 26, 2024

Choose a reason for hiding this comment

Uh oh!

tstuefe Apr 29, 2024

Choose a reason for hiding this comment

Uh oh!

tstuefe commented Apr 29, 2024

Uh oh!

openjdk bot commented Apr 29, 2024

Uh oh!

openjdk bot commented Apr 29, 2024

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

tstuefe commented Apr 19, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Apr 19, 2024 •

edited

Loading

mlbridge bot commented Apr 21, 2024 •

edited

Loading

tstuefe commented Apr 22, 2024 •

edited

Loading

MBaesken commented Apr 26, 2024 •

edited

Loading

MBaesken commented Apr 29, 2024 •

edited

Loading