8296470: Refactor VMError::report STEP macro to improve readability #11018

xmas92 · 2022-11-07T13:25:53Z

Refactor the STEP macro in VMError::report to improve readability.
Right now the macro contains multiple statements on one line and the non-conventional control flow is even harder to understand.

This enhancement aims to do two things:

It splits the macro into multiple lines with indentations which makes the structure of the C++ code generated by the preprocessor clearer.
Separates the internal step logic from the decision logic which decides if a step should be taken with a STEP_IF(step_name_str, condition) macro

Testing: tier 1 + GHA

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-8296470: Refactor VMError::report STEP macro to improve readability

Reviewers

Thomas Stuefe (@tstuefe - Reviewer) ⚠️ Review applies to b483c21c
Coleen Phillimore (@coleenp - Reviewer) ⚠️ Review applies to b483c21c

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 11018

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

Using diff file

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

bridgekeeper · 2022-11-07T13:27:40Z

👋 Welcome back aboldtch! 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 · 2022-11-07T13:29:59Z

@xmas92 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 · 2022-11-07T13:33:35Z

Webrevs

tstuefe

Hi Axel,

I'm fine with the line break, but unsure about the STEP_IF.

I don't think it adds much readability, but it is a wide-spread patch that will add unnecessary noise for us backporters. Just my opinion, lets see what others think.

Cheers, Thomas

tstuefe · 2022-11-07T15:25:14Z

src/hotspot/share/utilities/vmError.cpp

      st->print_cr("Will crash now (TestCrashInErrorHandler=%u)...",
        TestCrashInErrorHandler);
      controlled_crash(TestCrashInErrorHandler);
-    }
+      return true;
+    }())


I don't think this is necessary. The tests for secondary crash handling are exhaustive, there is no reason why it should differ from crash condition handling outside of a crash condition. If the signal handler is correctly set up, it should work. If it is not, the prior step is sufficient.

I don't think it adds much readability, but it is a wide-spread patch that will add unnecessary noise for us backporters. Just my opinion, lets see what others think.

I was worried about that too. This patch initially came about when I was writing #11017 which required rewriting the macro logic and add extra nested scope. The fact that the common case for a step was checking the verbose flag made me refactor out just the verbose case. And then I just refactored out the rest the few extra conditions as well.

But I am pretty ambivalent about this change. #11017 contains the newline changes as well. So if there are more opinions that this makes back porting work harder it can be dropped.

I don't think this is necessary. The tests for secondary crash handling are exhaustive, there is no reason why it should differ from crash condition handling outside of a crash condition. If the signal handler is correctly set up, it should work. If it is not, the prior step is sufficient.

I added it because in my initial change (9449501), a theoretical crash in a condition would recover but not proceed to the next step. The test change is to avoid such a regression if someone thinks of doing the same simplification of the macro condition logic. (It would crash before _current_step was updated.)

Ah, I understand. But you changed this to two nested ifs. So this is no problem anymore.

I still don't think it is needed with your new version. IIUC it prevents us from the problem you describe if someone changes the STEP macros in the future and has a very complex if statement that would crash.

About the STEP_IF, the more I look at it, the more I like it. So I am ambivalent. Let's hear what others say (@coleenp?).

coleenp

Ok, I was afraid to look at this. I do like STEP_IF. Maybe the longer expressions could be functions ?

xmas92 · 2022-11-10T13:27:18Z

Ok, I was afraid to look at this. I do like STEP_IF. Maybe the longer expressions could be functions ?

Are you think of the conditions in the STEP_IF macro? I think at least some of the longer expressions can be put on multiple lines. I guess it is already done for should_report_bug. I think verbose should always be seen in the macro, but a should_perform_step might be better, but I feel it might just be a noisy indirection when reading (as all the conditions are fairly simple). I should however fix the implicit bool conversions, they are both agains our style guide, and ugly.

For the step logic I think there are a few places that things can be refactored without losing the ability of easily comparing the VMError::report code to the hs_err file output.

tstuefe · 2022-11-10T14:40:34Z

One thing that worries me a bit is that our test coverage is not that great. We have jtreg/runtime/Errorhandling, but that is not much. In particular we miss good tests for robustness (e.g. that alert us if more STEPs start failing, that reporting can cope with a partially corrupted JVM context, or an invalid Thread::current, or pre-init, or if we have very little stack or C-Heap left)...

Unfortunately, tests like these tend to be annoyingly hard to get right. At SAP, we have more tests, but never really managed to get them completely clean.

tstuefe

Ok!

tstuefe · 2022-11-17T13:17:26Z

src/hotspot/share/utilities/vmError.cpp

+    record_step_start_time();                              \
+    _step_did_timeout = false;                             \
+    if ((cond)) {
+      // [Step logic]


nit: newline please?

tstuefe · 2022-11-17T13:17:32Z

src/hotspot/share/utilities/vmError.cpp

+    _step_did_timeout = false;                             \
+    if ((cond)) {
+      // [Step logic]
+# define STEP(s) STEP_IF(s, true)


nit: newline please?

openjdk · 2022-11-17T13:28:50Z

@xmas92 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:

8296470: Refactor VMError::report STEP macro to improve readability

Reviewed-by: stuefe, coleenp

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 316 new commits pushed to the master branch:

48017b1: 8296804: Document HttpClient configuration properties in java.net.http module-info
bd51b7e: 8296645: org.openjdk.bench.javax.tools.Javac leaves class files in current directory
b27a61e: 8297216: Search results omit some methods
33dfc7d: 8296954: G1: Enable parallel scanning for heap region remset
6f06f44: 8297242: Use-after-free during library unloading on Linux
4e8e853: 6788481: CellEditorListener.editingCanceled is never called
692bedb: 8297106: Remove the -Xcheck:jni local reference capacity checking
05128c2: 8286185: The Java manpage can be more platform inclusive
d450314: 8297276: Remove thread text from Subject.current
cdf9ed0: 8297528: java/io/File/TempDirDoesNotExist.java test failing on windows-x64
... and 306 more: https://git.openjdk.org/jdk/compare/8836b92593565e66d8063b1228d6701316f3078d...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.

coleenp

I apologize for the latency in reviewing this. It looks really nice. Thank you.

xmas92 · 2022-11-29T11:37:08Z

No worries, thanks for the reviews.

xmas92 · 2022-11-29T11:37:18Z

/integrate

openjdk · 2022-11-29T11:39:11Z

Going to push as commit 1301fb0.
Since your change was applied there have been 316 commits pushed to the master branch:

48017b1: 8296804: Document HttpClient configuration properties in java.net.http module-info
bd51b7e: 8296645: org.openjdk.bench.javax.tools.Javac leaves class files in current directory
b27a61e: 8297216: Search results omit some methods
33dfc7d: 8296954: G1: Enable parallel scanning for heap region remset
6f06f44: 8297242: Use-after-free during library unloading on Linux
4e8e853: 6788481: CellEditorListener.editingCanceled is never called
692bedb: 8297106: Remove the -Xcheck:jni local reference capacity checking
05128c2: 8286185: The Java manpage can be more platform inclusive
d450314: 8297276: Remove thread text from Subject.current
cdf9ed0: 8297528: java/io/File/TempDirDoesNotExist.java test failing on windows-x64
... and 306 more: https://git.openjdk.org/jdk/compare/8836b92593565e66d8063b1228d6701316f3078d...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-11-29T11:39:37Z

@xmas92 Pushed as commit 1301fb0.

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

xmas92 added 2 commits November 7, 2022 09:41

VMError::report STEP_IF and cleanup

9449501

Fix STEP_IF condition crash and clear_step_start_time

944ce8d

openjdk bot added the rfr Pull request is ready for review label Nov 7, 2022

openjdk bot added the hotspot hotspot-dev@openjdk.org label Nov 7, 2022

tstuefe suggested changes Nov 7, 2022

View reviewed changes

coleenp reviewed Nov 8, 2022

View reviewed changes

xmas92 added 3 commits November 17, 2022 10:10

Revert extended test

cbacf9f

Respect 100 character line

c842809

Follow HotSpot code style: no implicit boolean

b483c21

tstuefe approved these changes Nov 17, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Nov 17, 2022

coleenp approved these changes Nov 28, 2022

View reviewed changes

Add newlines

165e8ae

openjdk bot added the integrated Pull request has been integrated label Nov 29, 2022

openjdk bot closed this Nov 29, 2022

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8296470: Refactor VMError::report STEP macro to improve readability #11018

8296470: Refactor VMError::report STEP macro to improve readability #11018

xmas92 commented Nov 7, 2022 •

edited by openjdk bot

Loading

bridgekeeper bot commented Nov 7, 2022

openjdk bot commented Nov 7, 2022

mlbridge bot commented Nov 7, 2022 •

edited

Loading

tstuefe left a comment

tstuefe Nov 7, 2022

xmas92 Nov 7, 2022

tstuefe Nov 8, 2022

tstuefe Nov 8, 2022

coleenp left a comment

xmas92 commented Nov 10, 2022

tstuefe commented Nov 10, 2022 •

edited

Loading

tstuefe left a comment

tstuefe Nov 17, 2022

tstuefe Nov 17, 2022

openjdk bot commented Nov 17, 2022 •

edited

Loading

coleenp left a comment

xmas92 commented Nov 29, 2022

xmas92 commented Nov 29, 2022

openjdk bot commented Nov 29, 2022

openjdk bot commented Nov 29, 2022

8296470: Refactor VMError::report STEP macro to improve readability #11018

8296470: Refactor VMError::report STEP macro to improve readability #11018

Conversation

xmas92 commented Nov 7, 2022 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Nov 7, 2022

openjdk bot commented Nov 7, 2022

mlbridge bot commented Nov 7, 2022 • edited Loading

Webrevs

tstuefe left a comment

Choose a reason for hiding this comment

tstuefe Nov 7, 2022

Choose a reason for hiding this comment

xmas92 Nov 7, 2022

Choose a reason for hiding this comment

tstuefe Nov 8, 2022

Choose a reason for hiding this comment

tstuefe Nov 8, 2022

Choose a reason for hiding this comment

coleenp left a comment

Choose a reason for hiding this comment

xmas92 commented Nov 10, 2022

tstuefe commented Nov 10, 2022 • edited Loading

tstuefe left a comment

Choose a reason for hiding this comment

tstuefe Nov 17, 2022

Choose a reason for hiding this comment

tstuefe Nov 17, 2022

Choose a reason for hiding this comment

openjdk bot commented Nov 17, 2022 • edited Loading

coleenp left a comment

Choose a reason for hiding this comment

xmas92 commented Nov 29, 2022

xmas92 commented Nov 29, 2022

openjdk bot commented Nov 29, 2022

openjdk bot commented Nov 29, 2022

xmas92 commented Nov 7, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Nov 7, 2022 •

edited

Loading

tstuefe commented Nov 10, 2022 •

edited

Loading

openjdk bot commented Nov 17, 2022 •

edited

Loading