8217612: (CL)HSDB cannot show some JVM flags #431

ktakakuri · 2024-01-25T11:42:51Z

Hi all, this is a backport of JDK-8217612: (CL)HSDB cannot show some JVM flags.
The bug reported is reproducible in JDK8, so this patch should be applied.
This patch requires the follow-up patch JDK-8217850 and the correspoding pull request has been submitted.
The original patch does not apply cleanly, and the following modifications are needed:

hotspot/agent/src/share/classes/sun/jvm/hotspot/runtime/VM.java

Long.toUnsignedString method was introduced in JDK8, and the build fails with JDK7 as the boot-jdk when using this method. Consequently, an alternative private method to Long.toUnsignedString(long i, 10) is required in VM.java.
The size_t type was introduced in JDK9, so the related fix is skipped.
To return a String value in the getValue method, the original fix uses a static method, whereas this fix creates a new instance each time. The use of a static method was introduced to JDK-9 in the enhancement JDK-8145061. This enhancement has not been backported, so the same format as the other part of the getValue() method should be used.
Replace var with String.

hotspot/src/share/vm/runtime/globals.hpp

A comment on the flag type double and uint64_t is added, which matches teh implementation. This comment was added in JDK-8059847, but this fix is an enhancement and not necessary to backport, so I believe it is valid to fix the comment in this PR.

hotspot/test/serviceability/sa/ClhsdbFlags.java

This test is backported from the original commit with @test removed. This test will not function because the serviceability/sa test framework has not been backported. This follows the discussion in backporting JDK-8196969 to JDK8 (https://mail.openjdk.org/pipermail/jdk8u-dev/2020-May/011785.html ). Backporting SA-related tests is excessive and may require some follow-up test fixes, but it is beneficial to indicate that this test append should be integrated when the test framework is backported in the future.

Testing

manually check the behaviour
Consider Running java program with option -XX:NativeMemoryTracking=off -XX:OnError=echo -XX:MaxRAMPercentage=20.0 -XX:MaxRAM=10000000. The flag types correspond to ccstr, ccstrlist, double, and uint64_t, respectively. When attaching the process using clhsdb, the value is corrected by this patch as follows. Note that the default value of MaxMetaspaceSize is max_uintx.

without patch

hsdb> flags NativeMemoryTracking
NativeMemoryTracking = null 1
hsdb> flags OnError
OnError = null 1
hsdb> flags MaxRAMPercentage
MaxRAMPercentage = null 1
hsdb> flags MaxRAM
MaxRAM = null 1
hsdb> flags MaxMetaspaceSize
MaxMetaspaceSize = -4096 0

with patch

hsdb> flags NativeMemoryTracking
NativeMemoryTracking = "off" 1
hsdb> flags OnError
OnError = "echo" 1
hsdb> flags MaxRAMPercentage
MaxRAMPercentage = 20.0 1
hsdb> flags MaxRAM
MaxRAM = 10000000 1
hsdb> flags MaxMetaspaceSize
MaxMetaspaceSize = 18446744073709547520 0

hotspot_tier1 after applying the follow-up patch JDK-8217850

Thank you.

Progress

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

Issue

JDK-8217612: (CL)HSDB cannot show some JVM flags (Bug - P4 - Requested)

Reviewers

Paul Hohensee (@phohensee - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk8u-dev.git pull/431/head:pull/431
$ git checkout pull/431

Update a local copy of the PR:
$ git checkout pull/431
$ git pull https://git.openjdk.org/jdk8u-dev.git pull/431/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 431

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk8u-dev/pull/431.diff

Using Webrev

Link to Webrev Comment

bridgekeeper · 2024-01-25T11:43:26Z

👋 Welcome back ktakakuri! 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-01-25T11:43:59Z

This backport pull request has now been updated with issue from the original commit.

mlbridge · 2024-01-25T11:47:43Z

Webrevs

bridgekeeper · 2024-02-22T13:40:36Z

@ktakakuri This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

openjdk · 2024-03-13T20:05:27Z

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

ktakakuri · 2024-04-10T11:44:15Z

Could anyone review this backport please?
Failure of gc/metaspace/CompressedClassSpaceSizeInJmapHeap.java in GHA will be resolved in #433.

bridgekeeper · 2024-05-08T17:37:17Z

@ktakakuri This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

ktakakuri · 2024-05-21T08:55:52Z

Can someone please review this backport?

bridgekeeper · 2024-06-18T12:39:05Z

@ktakakuri This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

ktakakuri · 2024-06-23T23:44:48Z

Could anyone please review this fix?

bridgekeeper · 2024-07-21T23:58:37Z

@ktakakuri This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

ktakakuri · 2024-08-05T12:45:42Z

Could anyone review this backport please?
Failure of gc/metaspace/CompressedClassSpaceSizeInJmapHeap.java in GHA will be resolved in #433.

phohensee

I think we can safely require JDK 8 as the boot JDK. I doubt any JDK 8 developer uses JDK 7 for anything.

ktakakuri · 2024-08-06T13:11:51Z

In my environment, I am building JDK8 using JDK7 according to the N-1 rule.
As for me, I would like to support builds using JDK7.

phohensee

I agree with adding ClhsdbFlags.java as a "needed for a future backport", but please add a comment to that effect in place of @test.

ktakakuri · 2024-08-19T12:09:24Z

I added a comment.

phohensee

Thank you.

openjdk · 2024-08-19T15:27:53Z

⚠️ @ktakakuri This change is now ready for you to apply for maintainer approval. This can be done directly in each associated issue or by using the /approval command.

ktakakuri · 2024-08-20T06:26:30Z

/approval request This bug is reproducible in JDK8, and backporting this patch is needed. The patch does not apply cleanly due to enhancements introduced since JDK9 and the method that can't be used for building with JDK7. Low risk, tool bug fix. Backport requires follow-up issue JDK-8217850 and the correspoding backport has been submitted.
Tesing: GHA, hotspot_tier1, and manual behaviour checks.

openjdk · 2024-08-20T06:27:10Z

@ktakakuri
8217612: The approval request has been created successfully.

bridgekeeper · 2024-09-17T11:35:30Z

@ktakakuri This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

ktakakuri · 2024-09-18T10:46:28Z