8329418: Replace pointers to tables with offsets in relocation bitmap #19107

matias9927 · 2024-05-06T17:05:47Z

The beginning of the RW region contains pointers to c++ vtables which are always located at a fixed offset from the shared base address at runtime. This offset can be calculated at dumptime and stored with the read-only tables at the top of the RO region. As a further improvement, all the pointers to RO tables are replaced with offsets as well.

These changes will reduce the number of pointers in the RW and RO regions and will allow for the relocation bitmap size optimizations to be more effective. Verified with tier 1-5 tests.

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-8329418: Replace pointers to tables with offsets in relocation bitmap (Sub-task - P4)

Reviewers

Chris Plummer (@plummercj - Reviewer) ⚠️ Review applies to d40afef9
Ioi Lam (@iklam - Reviewer) ⚠️ Review applies to 11f39483

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 19107

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2024-05-06T17:06:29Z

👋 Welcome back matsaave! 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 · 2024-05-06T17:07:02Z

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

8329418: Replace pointers to tables with offsets in relocation bitmap

Reviewed-by: cjplummer, iklam

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

d47a4e9: 8332008: Enable issuestitle check
0bf7282: 8331231: containers/docker/TestContainerInfo.java fails
ffbdfff: 8331999: BasicDirectoryModel/LoaderThreadCount.java frequently fails on Windows in CI

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.

openjdk · 2024-05-06T17:07:26Z

@matias9927 The following labels will be automatically applied to this pull request:

hotspot
serviceability

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

mlbridge · 2024-05-06T20:50:02Z

Webrevs

plummercj · 2024-05-06T22:30:28Z

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/memory/FileMapInfo.java

-        Address vtableInfoAddress = vtablesIndex.getAddressAt(i * addressSize); // = _index[i]
-        Address vtableAddress = vtableInfoAddress.addOffsetTo(addressSize); // = &_index[i]->_cloned_vtable[0]
+        long vtable_offset = vtablesIndex.getJLongAt(i * addressSize); // long offset = _index[i]
+        System.out.printf("Offset: %x\n", vtable_offset);


Remove printf().

plummercj · 2024-05-06T22:30:55Z

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/memory/FileMapInfo.java

+      //   ...
+      // };
+      //
+      // The following loop compues the following


Since there are lot of comments after this point, maybe the wording should instead be "The loop below...". Also, should be "computes"

plummercj · 2024-05-06T22:32:12Z

test/hotspot/jtreg/serviceability/sa/TestSysProps.java

@@ -65,7 +65,7 @@ public static void countProps(String[] propLines, int expectedCount, String cmdN
            }
        }
        if (numProps != expectedCount) {
-            throw new RuntimeException("Wrong number of " + cmdName + " properties: " + numProps);
+            throw new RuntimeException("Wrong number of " + cmdName + " properties: " + numProps + " Expected: " + expectedCount);


I think it would be good to add parenthesis around the extra output you added.

This was an accidental leftover from debugging, I didn't intend for this to be part of the change. I should revert this since it's beyond the scope of this change.

plummercj

SA changes look good. Thanks for taking care of this.

iklam

Looks good. I will suggest making some clean up to improve the readability of the existing code.

src/hotspot/share/cds/archiveUtils.cpp

src/hotspot/share/cds/cppVtables.cpp

iklam

LGTM. Just one small nit.

src/hotspot/share/cds/serializeClosure.hpp

matias9927 · 2024-05-10T01:33:37Z

Thanks for the reviews @plummercj and @iklam!
/integrate

openjdk · 2024-05-10T01:34:03Z

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

a643d6c: 8331862: Remove split relocation info implementation
d47a4e9: 8332008: Enable issuestitle check
0bf7282: 8331231: containers/docker/TestContainerInfo.java fails
ffbdfff: 8331999: BasicDirectoryModel/LoaderThreadCount.java frequently fails on Windows in CI

Your commit was automatically rebased without conflicts.

openjdk · 2024-05-10T01:34:09Z

@matias9927 Pushed as commit a706ca4.

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

matias9927 added 9 commits April 26, 2024 15:04

8329418: Replace pointers to tables with offsets in relocation bitmap

73e2519

Serializing offsets

041b62d

Adjusted serialization

e113cbc

Now works with -Xshare:on

e5597a7

Fixed dynamic dumping

b3c9ae0

Editing SA

ec16bc6

Corrected SA

b085d68

Cleanup

cb4892c

Merge branch 'master' into pointer_to_offset_8329418

c925025

openjdk bot added serviceability serviceability-dev@openjdk.org hotspot hotspot-dev@openjdk.org labels May 6, 2024

matias9927 marked this pull request as ready for review May 6, 2024 20:46

openjdk bot added the rfr Pull request is ready for review label May 6, 2024

plummercj reviewed May 6, 2024

View reviewed changes

Chris comments and cleanup

d40afef

plummercj approved these changes May 7, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label May 7, 2024

iklam suggested changes May 8, 2024

View reviewed changes

src/hotspot/share/cds/archiveUtils.cpp Outdated Show resolved Hide resolved

src/hotspot/share/cds/cppVtables.cpp Show resolved Hide resolved

matias9927 added 2 commits May 9, 2024 17:26

Ioi comments

32d69bc

Merge branch 'master' into pointer_to_offset_8329418

11f3948

iklam approved these changes May 9, 2024

View reviewed changes

src/hotspot/share/cds/serializeClosure.hpp Outdated Show resolved Hide resolved

u_char* to void**

7fca658

openjdk bot added the integrated Pull request has been integrated label May 10, 2024

openjdk bot closed this May 10, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels May 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8329418: Replace pointers to tables with offsets in relocation bitmap #19107

8329418: Replace pointers to tables with offsets in relocation bitmap #19107

matias9927 commented May 6, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented May 6, 2024

openjdk bot commented May 6, 2024 •

edited

Loading

openjdk bot commented May 6, 2024

mlbridge bot commented May 6, 2024 •

edited

Loading

plummercj May 6, 2024

plummercj May 6, 2024

plummercj May 6, 2024

matias9927 May 7, 2024

plummercj left a comment

iklam left a comment

iklam left a comment

matias9927 commented May 10, 2024

openjdk bot commented May 10, 2024

openjdk bot commented May 10, 2024

8329418: Replace pointers to tables with offsets in relocation bitmap #19107

8329418: Replace pointers to tables with offsets in relocation bitmap #19107

Conversation

matias9927 commented May 6, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented May 6, 2024

openjdk bot commented May 6, 2024 • edited Loading

openjdk bot commented May 6, 2024

mlbridge bot commented May 6, 2024 • edited Loading

Webrevs

plummercj May 6, 2024

Choose a reason for hiding this comment

plummercj May 6, 2024

Choose a reason for hiding this comment

plummercj May 6, 2024

Choose a reason for hiding this comment

matias9927 May 7, 2024

Choose a reason for hiding this comment

plummercj left a comment

Choose a reason for hiding this comment

iklam left a comment

Choose a reason for hiding this comment

iklam left a comment

Choose a reason for hiding this comment

matias9927 commented May 10, 2024

openjdk bot commented May 10, 2024

openjdk bot commented May 10, 2024

matias9927 commented May 6, 2024 •

edited by openjdk bot

Loading

openjdk bot commented May 6, 2024 •

edited

Loading

mlbridge bot commented May 6, 2024 •

edited

Loading