8382306: Remove JvmtiTagMap::check_hashmap

[lmesnik]

The check_hashmap(nullptr) doesn't do anything now.
It was used to rehash map but now just returns.

And the remaining usage can be replaced with direct call of remove_dead_entries_locked.

---

• I confirm that I make this contribution in accordance with the OpenJDK Interim AI Policy.

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issue

• JDK-8382306: Remove JvmtiTagMap::check_hashmap (Enhancement - P4)

Reviewers

• Serguei Spitsyn (@sspitsyn - Reviewer)
• Coleen Phillimore (@coleenp - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 30757

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

Using diff file

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

Using 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:

8382306: Remove JvmtiTagMap::check_hashmap

Reviewed-by: sspitsyn, 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 37 new commits pushed to the master branch:

• 0dd0108: 8377936: VoiceOver Reads Button Incorrectly
• 804ecc1: 8382436: Build failures after JDK-8326205
• 2325c36: 8378891: Problemlist TestStaticCallStub on Windows AArch64
• ... and 34 more: https://git.openjdk.org/jdk/compare/f2289d84d3b99686e90756921738d406f543337f...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

• 00: Full (d9cce05c)

[dholmes-ora]

Seems the new code no longer skips processing in the is_empty() case - IIUC that will just be an extra clearing of weak refs if anything.

[lmesnik]

I don't think it is important here. The check_hashmaps_for_heapwalk is called once during heapwalking operation which is extremely slow already.

[lmesnik]

The another difference observed by @sspitsyn while discussing this issue.
The remove_dead_entries_locked() is now called and work even if
env()->is_enabled(JVMTI_EVENT_OBJECT_FREE)
is false.
It just cleans up tagmap table, but doesn't post events. Shouldn't change behaviour observed by jvmti agent.
The performance impact is not significant comparing with heap walking.

[sspitsyn]

One more difference that the check_hashmaps_for_heapwalk() is also called when objects == nullptr.

[lmesnik]

check_hashmaps_for_heapwalk()
never called with objects == nullptr
The goal of this function is to post ObjectFree events.
I still wonder why is it needed? It collect tags for dead objects before heap walking and post them right after
corresponding VMOp is completed.
It is really unclear for me if there are any reasons to do this.

[openjdk]

The total number of required reviews for this PR has been set to 2 based on the presence of this label: hotspot. This can be overridden with the /reviewers command.

[openjdk]

⚠️ @lmesnik The .jcheck/conf in the target branch of this pull request is invalid. Until that is resolved, this pull request cannot be processed. Please notify the repository owner.

[sspitsyn]

I'm okay with this fix in general. But this code has a history to be fragile. What tests did you run for this?

[coleenp]

This does seem unnecessary when called with nullptr certainly. It appears that remove_dead_entries_locked also checks if needs_cleanup and if ObjectFree events should be posted. Looks like a nice cleanup.
As Serguei says, this code has been very fragile but the jvmti/jdi tests should verify if this is okay. Also run with ZGC stress options like -XX:ZCollectionInterval=1 or 0.1 I think.

[jsikstro]

Just chiming in: you can run a lot of frequent major GCs with ZGC using:

-XX:+UseZGC -XX:+ZCollectionIntervalOnly -XX:ZCollectionIntervalMajor=0.001

[lmesnik]

Thank you for review and testing suggestions.
I run jvmti tests with -XX:+UseZGC -XX:+ZCollectionIntervalOnly -XX:ZCollectionIntervalMajor=0.001.
/integrate

[openjdk]

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

• b96473c: 8382396: RepaintManager throws NPE when painting detached JPanel without volatile buffering
• be85091: 8380971: New -Werror syntax does not exclude categories
• ff77538: 8381985: Shenandoah: allocation rate sample could be wrong in some rare cases
• ... and 61 more: https://git.openjdk.org/jdk/compare/f2289d84d3b99686e90756921738d406f543337f...master

Your commit was automatically rebased without conflicts.

[openjdk]

@lmesnik Pushed as commit 0993467.

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