8265919: RunThese30M fails "assert((!(((((JfrTraceIdBits::load(value)) & ((1 << 4) << 8)) != 0))))) failed: invariant"

[mgronlun]

Greetings,

Please help review this small changeset to remove an invalid assertion; reasoning is summarized in the JIRA issue.

Testing: jdk_jfr

Thanks
Markus

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed

Issue

• JDK-8265919: RunThese30M fails "assert((!(((((JfrTraceIdBits::load(value)) & ((1 << 4) << 8)) != 0))))) failed: invariant"

Reviewers

• Erik Gahlin (@egahlin - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/4583/head:pull/4583
$ git checkout pull/4583

Update a local copy of the PR:
$ git checkout pull/4583
$ git pull https://git.openjdk.java.net/jdk pull/4583/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 4583

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/4583.diff

[bridgekeeper]

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

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

• hotspot-jfr

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

• 00: Full (dfa2c65d)

[egahlin]

I'm OK with removing the assert, given that it is unlikely to happen in production code, but I think we should try to fix it for JDK 18 (and possibly backport).

Starting up JFR is an expensive operation, in this case a second time, so It doesn't seem unreasonable to add some code to reset the state properly, even if it involves iterating all classes, introducing a safepoint or taking some lock.

[openjdk]

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

8265919: RunThese30M fails "assert((!(((((JfrTraceIdBits::load(value)) & ((1 << 4) << 8)) != 0))))) failed: invariant"

Reviewed-by: egahlin

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

• c37988d: 8268276: Base64 Decoding optimization for x86 using AVX-512
• 08ee7ae: 8268855: Cleanup name handling in the Thread class and subclasses
• c79034e: 8269303: Remove unnecessary forward declaration of PSPromotionManager in cpCache.hpp
• 42968db: 8269293: ObjectMonitor thread id fields should be 64 bits.
• 2fd7943: 8256425: Obsolete Biased Locking in JDK 18
• 595446b: 8269087: CheckSegmentedCodeCache test fails in an emulated-client VM
• 7c31903: 8267075: jcmd VM.cds should print directory of the output files

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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.

[mgronlun]

I'm OK with removing the assert, given that it is unlikely to happen in production code, but I think we should try to fix it for JDK 18 (and possibly backport).

Starting up JFR is an expensive operation, in this case a second time, so It doesn't seem unreasonable to add some code to reset the state properly, even if it involves iterating all classes, introducing a safepoint or taking some lock.

Thanks Erik. I don't think there is anything to fix "properly". This is the state reset code that runs when JFR starts, JfrTypeSet::clear(). It is just that it built on an invalid assumption (that no class unload will happen in-between the last rotate() and end_recording()).

[mgronlun]

/integrate

[openjdk]

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

• fdcae66: 8269092: Add OldObjectSampleEvent.allocationSize field
• fd43d9c: 8269225: JFR.stop misses the written info when the filename is only specified by JFR.start
• 3a8f3d6: 8269280: (bf) Replace StringBuffer in *Buffer.toString()
• c37988d: 8268276: Base64 Decoding optimization for x86 using AVX-512
• 08ee7ae: 8268855: Cleanup name handling in the Thread class and subclasses
• c79034e: 8269303: Remove unnecessary forward declaration of PSPromotionManager in cpCache.hpp
• 42968db: 8269293: ObjectMonitor thread id fields should be 64 bits.
• 2fd7943: 8256425: Obsolete Biased Locking in JDK 18
• 595446b: 8269087: CheckSegmentedCodeCache test fails in an emulated-client VM
• 7c31903: 8267075: jcmd VM.cds should print directory of the output files

Your commit was automatically rebased without conflicts.

[openjdk]

@mgronlun Pushed as commit ffa34ed.

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

[egahlin]

Thanks Erik. I don't think there is anything to fix "properly". This is the state reset code that runs when JFR starts, JfrTypeSet::clear(). It is just that it built on an invalid assumption (that no class unload will happen in-between the last rotate() and end_recording()).

Great! I got the impression in JIRA that it could result in a reference being null (in the file), but I may have misunderstood.

[mgronlun]

Right. For product builds, the return (just after the assert), would have caused the serialized state not to be reset, because the expectation was that no artifacts would not have such a state. A non-cleared serialized state could cause the artifact to not be written to the constant pool correctly later, as it would be assumed to have been written already (has the serialized state bit set). The removal of the return lets the code fall-through for the serialized state bit(s) to be cleared explicitly.