JDK-8257588: Make os::_page_sizes a bitmask by tstuefe · Pull Request #1522 · openjdk/jdk

tstuefe · 2020-11-30T16:48:35Z

Hi,

may I please have reviews for the following very small improvement:

While discussing JDK-8243315 [1], and aiming to make planned changes like JDK-8256155 [2] easier:

size_t os::_page_sizes[os::page_sizes_max];

is an array used to keep all page sizes the hotspot can use. It is sorted by size and filled in at initialization time.

Coding dealing with this can be simplified by making this a set (which is very easy since all page sizes are power-2-values so they lend themselves nicely to a bitmap).

That has the following advantages:

makes adding new sizes simple since we do not have to re-sort the array. Coding is easier to read too.
it makes it possible to encode a set of page sizes in one number, so we can hand a set-of-page-sizes around as a value.

The patch adds a new class, os::PagesizeSet, which is a bitmap containing page sizes. It adds gtests for this class. It replaces the old os::_page_sizes with an object of that class. It also removes an unused function.

Testing:

nightlies at SAP
manual tests with UseLargePages
the new gtests
gh actions (with x86 still failing because of unrelated issues)

Thank you,

Thomas

[1] https://bugs.openjdk.java.net/browse/JDK-8243315
[2] https://bugs.openjdk.java.net/browse/JDK-8256155

/label hotspot-dev

Progress

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

Issue

JDK-8257588: Make os::_page_sizes a bitmask

Reviewers

Thomas Schatzl (@tschatzl - Reviewer)
Stefan Karlsson (@stefank - Reviewer) ⚠️ Review applies to 919d90b

Download

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

bridgekeeper · 2020-11-30T16:49:10Z

👋 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 · 2020-11-30T16:49:53Z

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

openjdk · 2020-12-02T10:49:19Z

@tstuefe
The hotspot label was successfully added.

mlbridge · 2020-12-02T10:54:03Z

Webrevs

src/hotspot/share/runtime/os.cpp

src/hotspot/share/runtime/os.hpp

src/hotspot/share/runtime/os.cpp

src/hotspot/share/runtime/os.hpp

src/hotspot/share/runtime/os.cpp

tschatzl · 2020-12-03T10:19:26Z

src/hotspot/share/runtime/os.cpp

+  if (v2 == 0) {
+    return 0;
+  }
+  while ((v2 & 1) == 0) {


Maybe the count_trailing_zeros() method could be used here instead of doing the same manually.

tschatzl

To be really nitpicky: the subject of the PR should be made more specific to mention that the new set is a bitset not just a set. I.e. os::page_sizes has already previously been a set (of integers), so where's the change? :P

tstuefe · 2020-12-03T13:56:03Z

Hi Thomas,

thanks again for the review. I hope I addressed all points. I completely removed the comments from the cpp file and corrected them in the hpp file, which removes the need to keep them in sync. And I rather have them in the hpp file, that makes my IDE happy.

Cheers, Thomas

mgkwill · 2020-12-03T17:55:10Z

+1 on the general thrust of this change.

I'd be happy to incorporate into #1153 & JDK-8256155 [1] if they are approved.

[1] https://bugs.openjdk.java.net/browse/JDK-8256155

stefank

Just a few drive-by comment: Could PagesizeSet be named PageSizeSet. The rest of the code separates the word, so I think that name would be better. The same for some of the parameters that are named pagesize.

Also, I think you could have called the class PageSizes. I don't think the fact that this is a 'set' matters for the usages of the class. It also makes the variable name match the type name, which I guess is an indication that it wouldn't be such a bad name:

static PageSizes        _page_sizes;`

vs

  static PagesizeSet        _page_sizes;

stefank · 2020-12-03T21:16:07Z

src/hotspot/share/runtime/os.cpp

+
+bool os::PagesizeSet::is_set(size_t pagesize) const {
+  assert(is_power_of_2(pagesize), "pagesize must be a power of 2: " INTPTR_FORMAT, pagesize);
+  return (_v & pagesize) > 0;


Why is this > 0 and not simply != 0

I'm not sure if you saw this comment, but is there a reason why > 0 is used here?

stefank · 2020-12-03T21:18:16Z

src/hotspot/share/runtime/os.cpp

+  if (v2 > 0) {
+    return round_down_power_of_2(v2);
+  }
+  return 0;


In next_larger you use the for:

if (v2 == 0) { return 0; } return ...

I think it would be nice to be consistent between the two functions.

Just a few drive-by comment: Could PagesizeSet be named PageSizeSet. The rest of the code separates the word, so I think that name would be better. The same for some of the parameters that are named pagesize.

Also, I think you could have called the class PageSizes. I don't think the fact that this is a 'set' matters for the usages of the class. It also makes the variable name match the type name, which I guess is an indication that it wouldn't be such a bad name:

static PageSizes _page_sizes;`

vs

static PagesizeSet _page_sizes;

Hi Stefan,

thanks for your review! I adapted most of your suggestions.

Cheers, Thomas

src/hotspot/share/runtime/os.cpp

src/hotspot/share/runtime/os.hpp

stefank

I think this mostly looks good, but there are a number of cleanups that I'd like you to consider.

stefank · 2020-12-04T13:47:51Z

src/hotspot/share/runtime/os.cpp

+
+bool os::PagesizeSet::is_set(size_t pagesize) const {
+  assert(is_power_of_2(pagesize), "pagesize must be a power of 2: " INTPTR_FORMAT, pagesize);
+  return (_v & pagesize) > 0;


I'm not sure if you saw this comment, but is there a reason why > 0 is used here?

src/hotspot/share/runtime/os.cpp

src/hotspot/share/runtime/os.hpp

test/hotspot/gtest/runtime/test_os.cpp

tstuefe · 2020-12-04T17:21:49Z

Hi Stefan,

I pushed a new commit with the changes you requested.

Thanks, Thomas

stefank

Thanks! Looks good.

openjdk · 2020-12-07T08:13:57Z

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

8257588: Make os::_page_sizes a bitmask

Reviewed-by: tschatzl, stefank

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

05dac03: 8257803: Add -Xbatch to compiler/blackhole tests
29a09c8: 8257668: SA JMap - skip non-java thread stack dump for heap dump
e590618: 8252505: C1/C2 compiler support for blackholes
972bc3b: 8256167: Convert JDK use of Reference::get to Reference::refersTo
78be334: 8242332: Add SHA3 support to SunPKCS11 provider
c4339c3: 8243614: Typo in ReentrantLock's Javadoc
d3ac1bf: 8198390: Test MultiResolutionDrawImageWithTransformTest.java fails when -esa is passed
51d325e: 8257633: Missing -mmacosx-version-min=X flag when linking libjvm
e27ea4d: 8257750: writeBuffer field of java.io.DataOutputStream should be final
dd0b945: 8257531: Super word not applied to a loop of simple Buffer operations
... and 113 more: https://git.openjdk.java.net/jdk/compare/a3e1980c084409c24ce8eb745ae65777fd2725ad...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.

tstuefe · 2020-12-07T08:49:57Z

Thanks! Looks good.

Thank you Stefan.

tstuefe · 2020-12-07T10:31:02Z

Thanks Thomas

tstuefe · 2020-12-07T10:31:36Z

/integrate

openjdk · 2020-12-07T10:32:31Z

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

566d77a: 8254802: ThrowingPushPromisesAsStringCustom.java fails in "try throwing in GET_BODY"
f5a582c: 8257575: C2: "failed: only phis" assert failure in loop strip mining verification
d05401d: 8256679: Update serialization javadoc once JOSS changes for records are complete
7620124: 8257230: assert(InitialHeapSize >= MinHeapSize) failed: Ergonomics decided on incompatible initial and minimum heap sizes
05dac03: 8257803: Add -Xbatch to compiler/blackhole tests
29a09c8: 8257668: SA JMap - skip non-java thread stack dump for heap dump
e590618: 8252505: C1/C2 compiler support for blackholes
972bc3b: 8256167: Convert JDK use of Reference::get to Reference::refersTo
78be334: 8242332: Add SHA3 support to SunPKCS11 provider
c4339c3: 8243614: Typo in ReentrantLock's Javadoc
... and 117 more: https://git.openjdk.java.net/jdk/compare/a3e1980c084409c24ce8eb745ae65777fd2725ad...master

Your commit was automatically rebased without conflicts.

Pushed as commit 8e8e584.

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

mgkwill · 2020-12-09T17:26:43Z

test/hotspot/gtest/runtime/test_os.cpp

+    for (size_t s = os::page_sizes().largest(); s != 0; s = os::page_sizes().next_smaller(s)) {
+      const size_t expected = os::page_sizes().next_smaller(s);
+      if (expected != 0) {
+        size_t actual = os::page_size_for_region_unaligned(expected - 17, 1);


@tstuefe should expected here be s ?

It seems like we are trying to compare the next smaller page size of S, with a slightly smaller size of the size S.

os::page_size_for_region_unaligned(expected - 17, 1);
vs.
os::page_size_for_region_unaligned(s - 17, 1);

Running these tests with 2 largepage sizes (3 total sizes) fails, however default of 2 page_sizes passes (4k and 2m or 1g):
(./hotspot/variant-server/libjvm/gtest/gtestLauncher -jdk:./images/jdk -Xms2G -Xmx2G -XX:+UseLargePages -XX:LargePageSizeInBytes=2M)

[----------] 17 tests from os [ RUN ] os.page_size_for_region_vm [ OK ] os.page_size_for_region_vm (0 ms) [ RUN ] os.page_size_for_region_aligned_vm [ OK ] os.page_size_for_region_aligned_vm (0 ms) [ RUN ] os.page_size_for_region_alignment_vm [ OK ] os.page_size_for_region_alignment_vm (0 ms) [ RUN ] os.page_size_for_region_unaligned_vm test/hotspot/gtest/runtime/test_os.cpp:106: Failure Expected equality of these values: actual Which is: 4096 expected Which is: 2097152 [ FAILED ] os.page_size_for_region_unaligned_vm (0 ms)

This only happen when #1153 is present in code, because otherwise you will only have two page_sizes, but still this should return the correct results.

See #1719 for fix.

Yes, you are right, it should. I filed JDK-8257989. Feel free to fix it.

Thanks, Thomas

Oh, I see you already have a PR open, I reply there.

Fixed in #1719. Thanks Thomas.

Add PagesizeSet

7014f81

openjdk bot added the hotspot-runtime hotspot-runtime-dev@openjdk.org label Nov 30, 2020

tstuefe changed the title ~~Add PagesizeSet~~ JDK-8257588: Make os::_page_sizes a set Dec 2, 2020

openjdk bot added the hotspot hotspot-dev@openjdk.org label Dec 2, 2020

tstuefe marked this pull request as ready for review December 2, 2020 10:50

openjdk bot added the rfr Pull request is ready for review label Dec 2, 2020

Fix 32bit issues

9e936bc

tschatzl suggested changes Dec 3, 2020

View reviewed changes

tstuefe changed the title ~~JDK-8257588: Make os::_page_sizes a set~~ JDK-8257588: Make os::_page_sizes a bitmask Dec 3, 2020

Feedback Thomas

fdae764

stefank reviewed Dec 3, 2020

View reviewed changes

mgkwill added a commit to mgkwill/jdk that referenced this pull request Dec 3, 2020

JDK-8257588: Make os::_page_sizes a bitmask openjdk#1522

979742c

mgkwill mentioned this pull request Dec 3, 2020

8256155: Allow multiple large page sizes to be used on Linux #1153

Closed

3 tasks

Feedback Stefan K

2356972

stefank reviewed Dec 4, 2020

View reviewed changes

Feedback Stefan (2)

919d90b

stefank approved these changes Dec 7, 2020

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Dec 7, 2020

Add missing include (ppc,s390 builds failed)

163b308

tschatzl approved these changes Dec 7, 2020

View reviewed changes

openjdk bot closed this Dec 7, 2020

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Dec 7, 2020

tstuefe deleted the pagesizeset branch December 7, 2020 13:13

mgkwill reviewed Dec 9, 2020

View reviewed changes

Conversation

tstuefe commented Nov 30, 2020 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Download

Uh oh!

bridgekeeper bot commented Nov 30, 2020

Uh oh!

openjdk bot commented Nov 30, 2020

Uh oh!

openjdk bot commented Dec 2, 2020

Uh oh!

mlbridge bot commented Dec 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tschatzl left a comment

Choose a reason for hiding this comment

Uh oh!

tstuefe commented Dec 3, 2020

Uh oh!

mgkwill commented Dec 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stefank left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stefank left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tstuefe commented Dec 4, 2020

Uh oh!

stefank left a comment

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Dec 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tstuefe commented Dec 7, 2020

Uh oh!

tstuefe commented Dec 7, 2020

Uh oh!

tstuefe commented Dec 7, 2020

Uh oh!

openjdk bot commented Dec 7, 2020

tstuefe commented Nov 30, 2020 •

edited by openjdk bot

Loading

mlbridge bot commented Dec 2, 2020 •

edited

Loading

mgkwill commented Dec 3, 2020 •

edited

Loading

openjdk bot commented Dec 7, 2020 •

edited

Loading

mgkwill Dec 9, 2020 •

edited

Loading