8371418: Methods in AdapterHandlerLibrary use HashtableBase iterate method incorrectly

[ashu-mehra]

The closure passed to HashTable::iterate in AdapterHandlerLibrary::contains and AdapterHandlerLibrary::print_handler_on is returning incorrect value. If the search is successful, it should return false to terminate the iteration, but it is returning true. This patch fixes the return value of these closures.
In addition, I noticed CompactHashTable::iterate goes through all entries unconditionally, which is not optimal for cases where we may want to terminate the iteration when some condition is met. This is the case in AdapterHandlerLibrary::contains and AdapterHandlerLibrary::print_handler_on when it iterates over _aot_adapter_handler_table. This patch updates CompactHashTable::iterate to be the same as HashTAble::iterate by using return value of the closure to determine if the iteration should continue or abort. It also adds CompactHashTable::iterate_all to iterate all the values unconditionally and the users of CompactHashTable::iterate are updated to use CompactHashTable::iterate_all.

---

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-8371418: Methods in AdapterHandlerLibrary use HashtableBase iterate method incorrectly (Bug - P4)

Reviewers

• Vladimir Kozlov (@vnkozlov - Reviewer)
• Andrew Dinn (@adinn - Reviewer) Review applies to f46d3dae

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 28197

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[bridgekeeper]

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

@ashu-mehra 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:

8371418: Methods in AdapterHandlerLibrary use HashtableBase iterate method incorrectly

Reviewed-by: kvn, adinn

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

• 0bae56b: 8367857: Implement JEP 525: Structured Concurrency (Sixth Preview)
• 72989e0: 8340093: C2 SuperWord: implement cost model
• 6e838d6: 8371474: Wrong object class or methodID passed to JNI call in TestSharedCloseJvmti with -Xshare:off
• ... and 27 more: https://git.openjdk.org/jdk/compare/d5803aa78a84caccd5c3f14ac788817c5a3b4725...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]

@ashu-mehra The following label will be automatically applied to this pull request:

• hotspot

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

• 02: Full - Incremental (94cab725)
• 01: Full - Incremental (f46d3dae)
• 00: Full (b827e012)

[vnkozlov]

Good. I submitted testing.

[vnkozlov]

@ashu-mehra, do you know what issue current code (before these changes) could cause?

[ashu-mehra]

do you know what issue current code (before these changes) could cause?

AdapterHandlerLibrary::print_handler_on is called from os::print_location() -> codeBlob::dump_for_addr().
os::print_location() is only used in error reporting to print the location of the address.
With the current code (before this patch) AdapterHandlerLibrary::print_handler_on could have returned without printing anything, even if the CodeBlob passed as parameter is of type AdapterBlob. In debug builds it could also trigger the assert:

assert(found, "Should have found handler");

btw the change that introduced this bug was made more than 3 years ago in https://bugs.openjdk.org/browse/JDK-8292384 and it went in JDK 20.

[ashu-mehra]

@vnkozlov fyi - I also opened https://bugs.openjdk.org/browse/JDK-8371493 which is going to touch the same code as this patch. I didn't includes the changes in this patch to make it easier to backport this patch if needed.

[vnkozlov]

@vnkozlov fyi - I also opened https://bugs.openjdk.org/browse/JDK-8371493 which is going to touch the same code as this patch. I didn't includes the changes in this patch to make it easier to backport this patch if needed.

Good. We usually don't port enhancement but we can consider it since its simplification of printing code which should not affect code execution.

[adinn]

Looks good

[ashu-mehra]

@adinn thanks for the review. I will fix the comment as suggested.

[vnkozlov]

My testing passed.

[ashu-mehra]

@vnkozlov thanks for the review and testing it.

[ashu-mehra]

/integrate

[openjdk]

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

• 8a911ae: 8371471: HttpClient: Log HTTP/3 handshake failures if logging errors is enabled
• 1327aa6: 8369949: Fix TestWaste.java stack overflow
• 0bae56b: 8367857: Implement JEP 525: Structured Concurrency (Sixth Preview)
• ... and 29 more: https://git.openjdk.org/jdk/compare/d5803aa78a84caccd5c3f14ac788817c5a3b4725...master

Your commit was automatically rebased without conflicts.

[openjdk]

@ashu-mehra Pushed as commit cc54d2c.

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