8332112: Update nsk.share.Log to don't print summary during VM shutdown hook

[lmesnik]

The nsk.share.Log doing some cleanup and reporting errors in the cleanup method. This method is supposed to be executed by finalizer originally. However, now it is called only during shutdown hook.
The cleanup using Cleaner doesn't work. See https://bugs.openjdk.org/browse/JDK-8330760

The cleanup() method flush stream and print summary which should be already printed by complain method.

This cleanup is not necessary and printing summary usually is just disabled. It is enabled if the test called 'complain' method. However, the error should have been printed already in this method.

So it would be simple to remove this cleanup and reduce usage of Finalizable in vmTestbase tests.

Note: The 'verboseOnErrorEnabled' is just not used.

See isVerboseOnErrorEnabled.

    public boolean isVerboseOnErrorEnabled() {
        return errorsSummaryEnabled;
    }

Tested with by running tests with different combinations (tier4-7) and tier1.

---

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-8332112: Update nsk.share.Log to don't print summary during VM shutdown hook (Enhancement - P4)

Reviewers

• David Holmes (@dholmes-ora - Reviewer) ⚠️ Review applies to 68b20e65
• Chris Plummer (@plummercj - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 19209

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

Using diff file

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

Webrev

Link to Webrev Comment

[bridgekeeper]

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

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

8332112: Update nsk.share.Log to don't print summary during VM shutdown hook

Reviewed-by: dholmes, cjplummer

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

• 0bb5ae6: 8332248: (fc) java/nio/channels/FileChannel/BlockDeviceSize.java failed with RuntimeException
• 4d32c60: 8322008: Exclude some CDS tests from running with -Xshare:off
• e91492a: 8313674: (fc) java/nio/channels/FileChannel/BlockDeviceSize.java should test for more block devices
• 95a6013: 8332042: Move MEMFLAGS to its own include file
• 5a4415a: 8331858: [nmt] VM.native_memory statistics should work in summary mode
• 4ba7447: 8326957: Implement JEP 474: ZGC: Generational Mode by Default
• 7ce4a13: 8332130: RISC-V: remove wrong instructions of Vector Crypto Extension
• ea5eb74: 8326404: Assertion error when trying to compile switch with fallthrough with pattern
• beea530: 8331907: BigInteger and BigDecimal should use optimized division
• 440782e: 8331466: Problemlist serviceability/dcmd/gc/RunFinalizationTest.java on generic-all
• ... and 10 more: https://git.openjdk.org/jdk/compare/5053b70a7fc67ce9b73dbeecbdd88fbc34d45e04...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]

@lmesnik The following labels will be automatically applied to this pull request:

• hotspot
• serviceability

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

[mlbridge]

Webrevs

• 01: Full - Incremental (69ffd5b7)
• 00: Full (68b20e65)

[dholmes-ora]

There seems to be very little in this PR that pertains to "finalize" so perhaps the JBS title etc could be updated to reflect what most of this PR is actually about.

However, now it is called only during shutdown hook.

Where does this get set up?

[lmesnik]

Every log (as any Finalazible object) is registered using
registerCleanup()
https://github.com/openjdk/jdk/blob/master/test/hotspot/jtreg/vmTestbase/nsk/share/Finalizable.java#L59
This function add object to FinalizerThread stack . This stack is processed and method cleanup is called for each object during shutdown.
See
https://github.com/openjdk/jdk/blob/master/test/hotspot/jtreg/vmTestbase/nsk/share/Finalizer.java#L105
for adding hook
and
https://github.com/openjdk/jdk/blob/master/test/hotspot/jtreg/vmTestbase/nsk/share/Finalizer.java#L118
for processing methods.

The line
Cleaner.create().register(this, () -> cleanup());
in https://github.com/openjdk/jdk/blob/master/test/hotspot/jtreg/vmTestbase/nsk/share/Finalizable.java#L63

doesn't work as supposed. The object is saved in FnalizerThread stack and never become unreachable.
Also, the cleaner itself capture this.

[lmesnik]

Please, not that shutdown hook is not compatible with jtreg agentvm execution. Really, it is not the recommended to use System.exit() and do something after main() in jtreg. Even in main/othervm mode jtreg call class using some wrapper.
This worked differently in previous in tonga and need to be adopted.

So the plan is to remove such cleanup as much as possible.

[dholmes-ora]

Every log (as any Finalazible object) is registered using registerCleanup()

But you have changed Log so it is no longer a FinalizableObject. ?? Ah I see this is what you meant by disabling it. Now a Log is a plain old Java object with no special cleanup.

[dholmes-ora]

Okay - seems fine. Thanks.

[plummercj]

Copyrights needs updating.

[lmesnik]

/integrate

[openjdk]

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

• 30bb066: 8332003: Clarify javadoc for MemoryLayout::offsetHandle
• c4867c6: 8329273: C2 SuperWord: Some basic MemorySegment IR tests
• 8032d64: 8332245: C2: missing record_for_ign() call in GraphKit::must_be_not_null()
• fa043ae: 8294880: Review running time of jdk/internal/shellsupport/doc/JavadocHelperTest.java
• a5005c8: 8330814: Cleanups for KeepAliveCache tests
• 1a94447: 8332111: [BACKOUT] A way to align already compiled methods with compiler directives
• 957eb61: 8331429: [JVMCI] Cleanup JVMCIRuntime allocation routines
• 2f10a31: 8302850: Implement C1 clone intrinsic that reuses arraycopy code for primitive arrays
• c642f44: 8329839: Cleanup ZPhysicalMemoryBacking trace logging
• d04ac14: 8332236: javac crashes with module imports and implicitly declared class
• ... and 22 more: https://git.openjdk.org/jdk/compare/5053b70a7fc67ce9b73dbeecbdd88fbc34d45e04...master

Your commit was automatically rebased without conflicts.

[openjdk]

@lmesnik Pushed as commit 61aff6d.

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