JDK-8261334: NMT: tuning statistic shows incorrect hash distribution

[tstuefe]

This is a trivial patch.

Tuning statistics for the malloc site hash map in NMT use a MallocSiteWalker to walk the malloc sites.

There is a bug in the report code which causes hash distribution statistics displayed to be (sometimes widely) off:

Hash distribution:
  1    entry: 179
  2  entries:  79
  3  entries:  66
  4  entries:  72
  5  entries:  98
  6  entries:  75
  7  entries:  55
  8  entries:  43
  9  entries:  22
 10 entries:   16
 11 entries:    5
 12 entries:    6
                  (sum 716)

This is the bucket chain length histogram. Note that the sum of all values is 716 which exceeds the table width of 511, the total number of buckets.

The problem is caused by a bug in the walker code where the bucket index is calculated by manually mod'ing NativeCallStack::hash() with table size:

jdk/src/hotspot/share/services/memTracker.cpp

Line 263 in d0a8f2f

int hash_bucket = e->hash() % MallocSiteTable::hash_buckets();

which is wrong since the hash is defined as signed int. So it yields incorrect index values if the hash code is <0, compared with the regular hashcode-to-index calculation done in the table itself, which translates the hash into an unsigned value before mod'ing:

jdk/src/hotspot/share/services/mallocSiteTable.hpp

Line 243 in d0a8f2f

static inline unsigned int hash_to_index(unsigned int hash) {

This is an old bug introduced with JDK-8046598 in 2014. Note that it causes the statistics to look better than it actually is, since it reports a long chain as multiple short chains.

---

The patch is really minimal, just adding the necessary cast at the right place. Patch is small for easy backporting. Hash code calculation will be touched up as part of https://bugs.openjdk.java.net/browse/JDK-8261302, so I'd like to keep the patch minimal. I also added tracing code to print tuning info as part of the final NMT report in debug VMs.

This is the correct output after the patch. Sum of buckets is 511 as expected. Note that the statistics are way less flattering now, since there are really almost none 1-length chains as the broken statistic claims:

Hash distribution:
empty bucket:  1
  1    entry:  8
  2  entries: 29
  3  entries: 52
  4  entries: 58
  5  entries: 93
  6  entries: 76
  7  entries: 71
  8  entries: 52
  9  entries: 37
 10 entries:  16
 11 entries:  12
 12 entries:   6
                  (sum 511)

---

Progress

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

Issue

• JDK-8261334: NMT: tuning statistic shows incorrect hash distribution

Reviewers

• Zhengyu Gu (@zhengyu123 - Reviewer)
• Aleksey Shipilev (@shipilev - Reviewer)

Download

$ git fetch https://git.openjdk.java.net/jdk pull/2458/head:pull/2458
$ git checkout pull/2458

[bridgekeeper]

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

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

• hotspot-runtime

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

• 01: Full - Incremental (2d9b56d)
• 00: Full (95aa187)

[zhengyu123]

This is irrelevant. tuning_statistics() is undocumented and should not advertise (at least, it was the decision then).

Otherwise, looks good.

[tstuefe]

Okay, I remove it before pushing. Thank you.

[zhengyu123]

Thanks.

[shipilev]

Looks fine to me.

[openjdk]

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

8261334: NMT: tuning statistic shows incorrect hash distribution

Reviewed-by: zgu, shade

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

• d6d5d9b: 8261231: Windows IME was disabled after DnD operation
• 29a428f: 8261229: MethodData is not correctly initialized with TieredStopAtLevel=3
• 48c932e: 8231286: HTML font size too large with high-DPI scaling and W3C_LENGTH_UNITS
• dbc35f6: 8261094: Open javax/swing/text/html/CSS/4765271/bug4765271.java
• db0ca2b: 8261161: Clean up warnings in hotspot/jtreg/vmTestbase tests
• 2c28e36: 8237352: Update DatagramSocket to add support for joining multicast groups
• d0a8f2f: 8260593: javac can skip a temporary local variable when pattern matching over a local variable
• deb0544: 8261251: Shenandoah: Use object size for full GC humongous compaction

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.

[tstuefe]

Since this is trivial, and all tests ran through, I'll integrate.

/integrate

[openjdk]

@tstuefe Since your change was applied there have been 10 commits pushed to the master branch:

• 92c6e6d: 8261254: Initialize charset mapping data lazily
• 351d788: 8259074: regex benchmarks and tests
• d6d5d9b: 8261231: Windows IME was disabled after DnD operation
• 29a428f: 8261229: MethodData is not correctly initialized with TieredStopAtLevel=3
• 48c932e: 8231286: HTML font size too large with high-DPI scaling and W3C_LENGTH_UNITS
• dbc35f6: 8261094: Open javax/swing/text/html/CSS/4765271/bug4765271.java
• db0ca2b: 8261161: Clean up warnings in hotspot/jtreg/vmTestbase tests
• 2c28e36: 8237352: Update DatagramSocket to add support for joining multicast groups
• d0a8f2f: 8260593: javac can skip a temporary local variable when pattern matching over a local variable
• deb0544: 8261251: Shenandoah: Use object size for full GC humongous compaction

Your commit was automatically rebased without conflicts.

Pushed as commit 20d7713.

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