8313708: NMT: cleanup _mst_marker

[gerard-ziemski]

While learning the NMT code I came across _mst_marker and I don't really like how it combines 16bit index and 16bit position into single 32bit mst_marker using bit sizzling.

Consequently, right now we need the following 3 APIs: build_marker(), bucket_idx_from_marker(), pos_idx_from_marker() to support this. They are really not adding any value, in my opinion, and in fact obfuscate the code.

I'd like to propose that we simplify the code and pass a struct value (as suggested by Thomas) which hides away the 2 individual fields that are implementation detail.

---

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-8313708: NMT: cleanup _mst_marker (Enhancement - P4)

Reviewers

• Johan Sjölen (@jdksjolen - Reviewer)
• Afshin Zafari (@afshin-zafari - Committer)
• Thomas Stuefe (@tstuefe - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 15145

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

Using diff file

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

Webrev

Link to Webrev Comment

[bridgekeeper]

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

@gerard-ziemski 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.

[tstuefe]

Hi Gerard,

I added the mst_marker with JDK-8281015 b96b743 for two reasons:

• to simplify things: I hated that many interfaces had to carry two separate arguments that both together refer to an MST entry. One argument too many, and implementation details exposed. The mst_marker OTOH is an opaque reference to an MST entry, and its form is an implementation detail that is not exposed and could be changed.
• to prepare further improvements: the point of the marker is to quickly resolve MST entries. But that could be done in a better way, without having to manually walk bucket chains and count buckets with O(n). For example, by storing MST entries in a linear array (they never go away, so one would not have to deal with deletion) and redefining the mst_marker to be an index into that table. That way, access to these entries would be O(1).

The latter improvement never materialized outside of a prototype I did. But it should be easy enough to do if you feel up to it. Just reverting 8281015 is, I feel, just a sideways step that arguably increases complexity, since a lot of interfaces now carry one argument more.

Cheers, Thomas

(P.S. if you really hate the bit fiddling, just make the mst marker a struct with two 16 bit members and pass it by reference; but a STATIC_ASSERT would be needed to make sure the compiler does not add padding.).

[jdksjolen]

Yes, the packing of two values into one is perhaps a bit unnecessary, but I do prefer the struct approach. Perhaps something like:

struct alignas(uint32_t) MSTM  {
  const uint16_t _bucket_idx;
  const uint16_t _bucket_pos;
};
STATIC_ASSERT(sizeof(MSTM) == sizeof(uint32_t), "must");

(Shouldn't MallocHeader be a packed data structure by the way?)

the point of the marker is to quickly resolve MST entries. But that could be done in a better way, without having to manually walk bucket chains and count buckets with O(n). For example, by storing MST entries in a linear array (they never go away, so one would not have to deal with deletion) and redefining the mst_marker to be an index into that table. That way, access to these entries would be O(1).

This would be very nice.

[gerard-ziemski]

Hi Gerard,

I added the mst_marker with JDK-8281015 b96b743 for two reasons:

• to simplify things: I hated that many interfaces had to carry two separate arguments that both together refer to an MST entry. One argument too many, and implementation details exposed. The mst_marker OTOH is an opaque reference to an MST entry, and its form is an implementation detail that is not exposed and could be changed.
• to prepare further improvements: the point of the marker is to quickly resolve MST entries. But that could be done in a better way, without having to manually walk bucket chains and count buckets with O(n). For example, by storing MST entries in a linear array (they never go away, so one would not have to deal with deletion) and redefining the mst_marker to be an index into that table. That way, access to these entries would be O(1).

The latter improvement never materialized outside of a prototype I did. But it should be easy enough to do if you feel up to it. Just reverting 8281015 is, I feel, just a sideways step that arguably increases complexity, since a lot of interfaces now carry one argument more.

Cheers, Thomas

(P.S. if you really hate the bit fiddling, just make the mst marker a struct with two 16 bit members and pass it by reference; but a STATIC_ASSERT would be needed to make sure the compiler does not add padding.).

I like the suggestion, thank you Thomas for the feedback!

[gerard-ziemski]

Yes, the packing of two values into one is perhaps a bit unnecessary, but I do prefer the struct approach. Perhaps something like:

struct alignas(uint32_t) MSTM  {
  const uint16_t _bucket_idx;
  const uint16_t _bucket_pos;
};
STATIC_ASSERT(sizeof(MSTM) == sizeof(uint32_t), "must");

(Shouldn't MallocHeader be a packed data structure by the way?)

the point of the marker is to quickly resolve MST entries. But that could be done in a better way, without having to manually walk bucket chains and count buckets with O(n). For example, by storing MST entries in a linear array (they never go away, so one would not have to deal with deletion) and redefining the mst_marker to be an index into that table. That way, access to these entries would be O(1).

This would be very nice.

Agree. I used your code pretty much verbatim - thank you!

[mlbridge]

Webrevs

• 00: Full (1bd8245e)

[jdksjolen]

One comment but this definitely looks like a nice improvement now, I want to read the surrounding code in a bit more depth before committing to the approval, thanks for the good work :).

[gerard-ziemski]

One comment but this definitely looks like a nice improvement now, I want to read the surrounding code in a bit more depth before committing to the approval, thanks for the good work :).

Thank you for the feedback.

[gerard-ziemski]

Ping

[jdksjolen]

Thank you for the PR! This looks good to me.

[openjdk]

@gerard-ziemski 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:

8313708: NMT: cleanup _mst_marker

Reviewed-by: jsjolen, azafari, stuefe

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

• b594f01: 8314748: 1-10% regressions on Crypto micros
• 39f1813: 8315318: Typo in comment on sun.nio.ch.Net.unblock4
• 47aa6f3: 8315219: G1: Improve allocator pathological case where it keeps doing direct allocations instead of retiring a PLAB
• 218829e: 8315421: [BACKOUT] 8314834 serviceability/jdwp/AllModulesCommandTest.java ignores VM flags
• 3c8a667: 8314611: Provide more explicative error message parsing Currencies
• df5e6e5: 8315248: AssertionError in Name.compareTo
• 3eac890: 8315061: Make LockingMode a product flag
• 8419a53: 8315072: Remove unneeded AdaptivePaddedAverage::operator new
• c90cd2c: 8286789: Test forceEarlyReturn002.java timed out
• 89d18ea: 8312018: Improve reservation of class space and CDS
• ... and 298 more: https://git.openjdk.org/jdk/compare/bb3aac606397481cb4832cb75ec0a549d079ab13...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.

[afshin-zafari]

Nice work. LGTM.

[tstuefe]

I don't like much the increased complexity of MallocSiteTable::lookup_or_add. It now needs two args to let the caller track the MST bucket, and exposes implementation details: "marker" had been a neutral term, "bucketXX" implies an open hash table and therefore exposes details that may change in the future.

But it's a minor gripe, and the rest is good. Let's ship it.

[tstuefe]

Curious, why the alignas? Internally, 16-bit alignment would be sufficient, but these live in MallocHeaders anyway which are placed by us manually.

[gerard-ziemski]

I don't like much the increased complexity of MallocSiteTable::lookup_or_add. It now needs two args to let the caller track the MST bucket

Yes, that's the only thing that bothers me about my proposed fix, but I can't think of a different way, other than bit sizzling it into one value (like it was), or removing "const" from BucketInfo fields. I thought that the best compromise was to do it the way I did, but I understand that it is not ideal.

and exposes implementation details: "marker" had been a neutral term, "bucketXX" implies an open hash table and therefore exposes details that may change in the future.

When reading this code, without prior knowledge of it, I had no idea what the "marker" was and how much attention I should pay to it. With the name like "bucket" it actually tells me something right away, even if at the same time it gives away the implementation. From the point of view of someone learning the NMT code, I like it better this way, but I do see how it might be a personal choice.

Thank you for accepting this change, even if you don't 100% agree with it.

[gerard-ziemski]

Thank you Johan and Afshin for your reviews!

[gerard-ziemski]

I'm going to wait with checking this in until after the dust settles with JDK-8315378 though.

[tstuefe]

I don't like much the increased complexity of MallocSiteTable::lookup_or_add. It now needs two args to let the caller track the MST bucket

Yes, that's the only thing that bothers me about my proposed fix, but I can't think of a different way, other than bit sizzling it into one value (like it was), or removing "const" from BucketInfo fields. I thought that the best compromise was to do it the way I did, but I understand that it is not ideal.

and exposes implementation details: "marker" had been a neutral term, "bucketXX" implies an open hash table and therefore exposes details that may change in the future.

When reading this code, without prior knowledge of it, I had no idea what the "marker" was and how much attention I should pay to it. With the name like "bucket" it actually tells me something right away, even if at the same time it gives away the implementation. From the point of view of someone learning the NMT code, I like it better this way, but I do see how it might be a personal choice.

Thank you for accepting this change, even if you don't 100% agree with it.

Sure, no problem. Sorry for the delay, I had completely forgotten about this issue.

Cheers, Thomas

[bridgekeeper]

@gerard-ziemski This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[bridgekeeper]

@gerard-ziemski This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.