8300184: Optimize ResourceHashtableBase::iterate_all using _number_of_entries #12016

navyxliu · 2023-01-16T19:50:37Z

Current implementation needs to visit all buckets. In extreme case, an empty hashtable with 1k buckets. iterate_all() loads 1k buckets for nothing. _number_of_entries is the number of nodes in the hashtable. This patch leverages that to quit iteration earlier. The real effect is up to the distribution of nodes among buckets.

I also included a debug log in the original patch. It was used for the observation in the following section, but I deleted it because it introduces circular dependency.

log_debug(hashtables)("ResourceHashtableBase table_size = %d, break at " INTX_FORMAT, sz, (bucket - table()));

Evaluation
In release build, the patch is very marginal. class loading do use ResourceHashtable but they are saturated.

I use it to run Renaissance/finagle-http. hahstable iterate_all quits a little bit earlier and save some loads. eg. in line 2, there are 109 buckets in the hashable. the iteration quit at 68th bucket, we save 42 loads.

$java -Xlog:hashtables=debug  -jar renaissance-gpl-0.14.1.jar finagle-http
...
[12.824s][debug][hashtables] table_size = 109, break at 109
[12.824s][debug][hashtables] table_size = 109, break at 68
[12.824s][debug][hashtables] table_size = 109, break at 107
[12.825s][debug][hashtables] table_size = 109, break at 108
[12.825s][debug][hashtables] table_size = 109, break at 99
[12.825s][debug][hashtables] table_size = 109, break at 108
[12.825s][debug][hashtables] table_size = 109, break at 98
[12.825s][debug][hashtables] table_size = 109, break at 109
[12.825s][debug][hashtables] table_size = 109, break at 102
...

Another application is in AsyncLog Thread. We have a hashtable with 17 buckets. Each Log Output creates one node in the hashtable. In common cases, there are only 2~3 nodes in the hashtable. Each time AsyncLog Thread flushes the buffer, it has to scan the hashtable twice. Saving loads means saving the memory bandwidth for the application.

In fastdebug build, extra verification is deployed. It's common to observe that JVM scan an empty hashtable. It's in destroy_VM.

./build/linux-x86_64-server-fastdebug/images/jdk/bin/java -Xlog:hashtables=debug --version
openjdk 21-internal 2023-09-19
OpenJDK Runtime Environment (fastdebug build 21-internal-adhoc.xxinliu.jdk)
OpenJDK 64-Bit Server VM (fastdebug build 21-internal-adhoc.xxinliu.jdk, mixed mode, sharing)
[0.130s][debug][hashtables] table_size = 139, break at 0
[0.136s][debug][hashtables] table_size = 107, break at 0
[0.136s][debug][hashtables] table_size = 1009, break at 0
[0.136s][debug][hashtables] table_size = 109, break at 107
[0.136s][debug][hashtables] table_size = 109, break at 109
[0.137s][debug][hashtables] table_size = 109, break at 101

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-8300184: Optimize ResourceHashtableBase::iterate_all using _number_of_entries

Reviewers

David Holmes (@dholmes-ora - Reviewer)
Robbin Ehn (@robehn - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 12016

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

Using diff file

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

…_entries

bridgekeeper · 2023-01-16T19:51:27Z

👋 Welcome back xliu! 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 · 2023-01-16T19:54:20Z

@navyxliu 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 · 2023-01-16T19:57:54Z

Webrevs

01: Full - Incremental (00a8abb5)
00: Full (93533a64)

Thread::name() isn't safe in gtest. We don't need the log line.

dholmes-ora

This seems quite reasonable to me.

Thanks.

dholmes-ora · 2023-01-17T04:50:52Z

Nit: the bug synopsis says iterate_all but you actually changed iterate.

openjdk · 2023-01-17T04:52:09Z

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

8300184: Optimize ResourceHashtableBase::iterate_all using _number_of_entries

Reviewed-by: dholmes, rehn

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

e82dc69: 8300205: Swing test bug8078268 make latch timeout configurable
9a36f8a: 8299673: Simplify object pinning interactions with string deduplication
3462438: 8299576: Reimplement java.io.Bits using VarHandle access
f829a67: 8300227: [macos_aarch64] assert(cpu_has("hw.optional.arm.FEAT_AES")) failed after JDK-8297092
6a81d52: 8299213: Bad cast in GrowableArrayWithAllocator<>::grow
8365c67: 8300084: AquaFileChooserUI.getDefaultButton returns null
382fe51: 8163229: several regression tests have a main method that is never executed
06f9374: 8298867: Basics.java fails with SSL handshake exception
859ccd1: 8299847: RISC-V: Improve PrintOptoAssembly output of CMoveI/L nodes
240830d: 8297092: [macos_aarch64] Add support for SHA feature detection
... and 322 more: https://git.openjdk.org/jdk/compare/10737e168c967a08e257927251861bf2c14795ab...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.

navyxliu · 2023-01-17T07:18:19Z

Hi, David,

Thank you for reviewing this patch.

Nit: the bug synopsis says iterate_all but you actually changed iterate.

This is intentional. iterate_all() calls iterate() with a lambda '()->true'. For iterate(), there are two cases. If the lambda returns constant value true, it mandates iterate() to traverse all elements. By contrast, user-defined lambdas could return false and stop iterating immediately. This patch only optimizes the former case. That's why I claim to optimize iterate_all rather than iterate.

Sometimes, we need to traverse all elements. eg. we want to print out the contents, copy hashable or do statistics.

thanks,
--lx

robehn

Thanks!

navyxliu · 2023-01-17T17:06:45Z

/integrate

openjdk · 2023-01-17T17:09:38Z

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

75b122f: 8300120: Configure should support different defaults for CI/dev build environments
e139ec3: 8300069: Left shift of negative value in share/adlc/dict2.cpp
fb147aa: 8300228: ModuleReader.find on exploded module throws if resource name maps to invalid file path
4cd166f: 8300125: Serial: Remove unused Generation::reset_saved_marks
85d70ac: Merge
e15bdc5: 8299789: Compilation of gtest causes build to fail if runtime libraries are in different dirs
b7fb8ef: 8299323: Allow extended registers for cmpw
07d5753: 8300168: Typo in AccessibleJTableHeaderEntry javadoc
e82dc69: 8300205: Swing test bug8078268 make latch timeout configurable
9a36f8a: 8299673: Simplify object pinning interactions with string deduplication
... and 330 more: https://git.openjdk.org/jdk/compare/10737e168c967a08e257927251861bf2c14795ab...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-01-17T17:09:54Z

@navyxliu Pushed as commit 0b9ff06.

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

dholmes-ora · 2023-01-18T02:53:07Z

That's why I claim to optimize iterate_all rather than iterate.

A call to iterate can result in iterating all elements if the lambda does not find an element to trigger iteration to cease; in which case this optimisation also applies.

8300184: Optimize ResourceHashtableBase::iterate_all using _number_of…

93533a6

…_entries

openjdk bot added the rfr Pull request is ready for review label Jan 16, 2023

openjdk bot added the hotspot hotspot-dev@openjdk.org label Jan 16, 2023

Xin Liu added 2 commits January 16, 2023 22:51

Fixed the build error on x86-32.

8c8b541

Remove log_debug.

00a8abb

Thread::name() isn't safe in gtest. We don't need the log line.

dholmes-ora approved these changes Jan 17, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jan 17, 2023

robehn approved these changes Jan 17, 2023

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Jan 17, 2023

openjdk bot closed this Jan 17, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jan 17, 2023

iklam mentioned this pull request Jan 29, 2023

8301136: Improve unlink() and unlink_all() of ResourceHashtableBase #12213

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8300184: Optimize ResourceHashtableBase::iterate_all using _number_of_entries #12016

8300184: Optimize ResourceHashtableBase::iterate_all using _number_of_entries #12016

navyxliu commented Jan 16, 2023 •

edited by openjdk bot

Loading

bridgekeeper bot commented Jan 16, 2023

openjdk bot commented Jan 16, 2023

mlbridge bot commented Jan 16, 2023 •

edited

Loading

dholmes-ora left a comment

dholmes-ora commented Jan 17, 2023

openjdk bot commented Jan 17, 2023 •

edited

Loading

navyxliu commented Jan 17, 2023

robehn left a comment

navyxliu commented Jan 17, 2023

openjdk bot commented Jan 17, 2023

openjdk bot commented Jan 17, 2023

dholmes-ora commented Jan 18, 2023

8300184: Optimize ResourceHashtableBase::iterate_all using _number_of_entries #12016

8300184: Optimize ResourceHashtableBase::iterate_all using _number_of_entries #12016

Conversation

navyxliu commented Jan 16, 2023 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Jan 16, 2023

openjdk bot commented Jan 16, 2023

mlbridge bot commented Jan 16, 2023 • edited Loading

Webrevs

dholmes-ora left a comment

Choose a reason for hiding this comment

dholmes-ora commented Jan 17, 2023

openjdk bot commented Jan 17, 2023 • edited Loading

navyxliu commented Jan 17, 2023

robehn left a comment

Choose a reason for hiding this comment

navyxliu commented Jan 17, 2023

openjdk bot commented Jan 17, 2023

openjdk bot commented Jan 17, 2023

dholmes-ora commented Jan 18, 2023

navyxliu commented Jan 16, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Jan 16, 2023 •

edited

Loading

openjdk bot commented Jan 17, 2023 •

edited

Loading