8302206: Factor out duplicate G1VerificationClosure

[tschatzl]

Hi all,

can I have reviews for this cleanup that merges G1VerifyLiveClosure from g1FullGCMarker.cpp and VerifyLiveClosure from heapRegion.cpp into a single one as they are virtually identical.

There is probably one change that needs some discussion, and that is about changing the type and meaning of G1MaxVerifyFailures: after changing the type of G1VerificationClosure::_num_failures to uint, the given comparison does not work any more due to differing types.
However the change of the default value to 0 meaning "show all messages" does not seem too bad: even before this change, a value of 0 at least showed one failure as this check (now it shows all messages) is located after the error printing in the closures.
It's a develop flag too, so instead of casting around I chose to simply change the meaning.

Testing: gha

Thanks,
Thomas

---

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-8302206: Factor out duplicate G1VerificationClosure

Reviewers

• Kim Barrett (@kimbarrett - Reviewer) ⚠️ Review applies to e6bfc0cf
• Albert Mingkun Yang (@albertnetymk - Reviewer) ⚠️ Review applies to b1dc2be1

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 12511

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

Using diff file

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

[bridgekeeper]

👋 Welcome back tschatzl! 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]

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

• hotspot-gc

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]

Webrevs

• 04: Full - Incremental (16072b3d)
• 03: Full - Incremental (e6bfc0cf)
• 02: Full - Incremental (b1dc2be1)
• 01: Full - Incremental (dc91f256)
• 00: Full (736f033f)

[albertnetymk]

However the change of the default value to 0 meaning "show all messages" does not seem too bad: even before this change, a value of 0 at least showed one failure as this check (now it shows all messages) is located after the error printing in the closures.

I find *Max* == 0 == show all msg too surprising. Is it possible to impl the most obvious interpretation, 0 means no msgs at all?

[tschatzl]

However the change of the default value to 0 meaning "show all messages" does not seem too bad: even before this change, a value of 0 at least showed one failure as this check (now it shows all messages) is located after the error printing in the closures.

I find *Max* == 0 == show all msg too surprising. Is it possible to impl the most obvious interpretation, 0 means no msgs at all?

This has been a (somewhat bad) tradeoff between implementation effort for this change and usefulness.

This is a develop flag, i.e. available only when debugging, and not printing any of the errors does not seem useful in that situation (I can hardly see a situation where one runs a debug build to find errors and then suppress all the messages you need for debugging. That corresponds to my use of that flag to set this value to something fairly low to decrease the amount of errors reported to something manageable, but still get an idea whether the issues are limited e.g. to a special type of region, but never to turn them off).

As for proper support of the 0 value, I would like to not implement that at least in this change; as its use is limited, I feel it is not worth the effort and feels out of scope. So I also set the minimum value to 1 instead of 0.

The flag now is a size_t and its default max_uintx (=max_size_t). That makes the interpretation clear I hope.

[albertnetymk]

This is a develop flag, i.e. available only when debugging, and not printing any of the errors does not seem useful in that situation

I see.

So I also set the minimum value to 1 instead of 0.

Thanks for the revision.

[openjdk]

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

8302206: Factor out duplicate G1VerificationClosure

Reviewed-by: kbarrett, ayang

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

• eaae0ba: 8302215: G1: Last-ditch Full GC should do serial compaction for tail regions in per thread compaction points.
• 7e08275: 8302668: [TESTBUG] Tests require feature sse4_1 which does not exist, should be sse4.1
• 5c0f50b: 8295979: [IR Framework] Improve IR matching warning
• 743a85d: 8302656: Missing spaces in output of -XX:+CIPrintMethodCodes
• 432cf68: 6753661: JFileChooser font not reset after Look & Feel change
• 2009dc2: 8302462: [REDO] 8297487: G1 Remark: no need to keep alive oop constants of nmethods on stack
• 7abe269: 8302781: CDS archive heap not reproducible after JDK-8296344
• d6716d2: 8302315: Examine cost of clone of primitive arrays compared to arraycopy
• 53be5dc: 8302812: JDK-8302455 broke ClassLoaderStatsTest on 32-bit
• 78f71b4: 8301873: Avoid string decoding in ZipFile.Source.getEntryPos
• ... and 115 more: https://git.openjdk.org/jdk/compare/1c7b09bc23ac37f83b9043de35b71bea7e814da5...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.

[kimbarrett]

Looks good except for the one simple change suggested.

[tschatzl]

Thanks @albertnetymk @kimbarrett for your reviews

/integrate

[openjdk]

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

• eaae0ba: 8302215: G1: Last-ditch Full GC should do serial compaction for tail regions in per thread compaction points.
• 7e08275: 8302668: [TESTBUG] Tests require feature sse4_1 which does not exist, should be sse4.1
• 5c0f50b: 8295979: [IR Framework] Improve IR matching warning
• 743a85d: 8302656: Missing spaces in output of -XX:+CIPrintMethodCodes
• 432cf68: 6753661: JFileChooser font not reset after Look & Feel change
• 2009dc2: 8302462: [REDO] 8297487: G1 Remark: no need to keep alive oop constants of nmethods on stack
• 7abe269: 8302781: CDS archive heap not reproducible after JDK-8296344
• d6716d2: 8302315: Examine cost of clone of primitive arrays compared to arraycopy
• 53be5dc: 8302812: JDK-8302455 broke ClassLoaderStatsTest on 32-bit
• 78f71b4: 8301873: Avoid string decoding in ZipFile.Source.getEntryPos
• ... and 115 more: https://git.openjdk.org/jdk/compare/1c7b09bc23ac37f83b9043de35b71bea7e814da5...master

Your commit was automatically rebased without conflicts.

[openjdk]

@tschatzl Pushed as commit e971f90.

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