8338912: CDS: Segmented roots array #20858

shipilev · 2024-09-04T18:44:37Z

Attempt to drop the min region alignment with JDK-8337828 highlights an interesting trouble. The roots array we are creating during the dump time can easily be larger than the min region alignment. We are currently "lucky" none of our tests hit this limit. AFAICS, about 128K classes would be enough to hit the current 1M min region alignment. Dropping the min region alignment to 256K starts to fail the test with "only" 30K classes, JDK-8338856.

We can slice that heap root array, and thus untie the roots count from the min region alignment. I am submitting something that works, but this might not be the final form for it. I would like @iklam to poke holes in this approach :)

Additional testing:

macos-aarch64-server-fastdebug, runtime/cds
linux-aarch64-server-fastdebug, all
linux-x86_64-server-fastdebug, all

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-8338912: CDS: Segmented roots array (Enhancement - P4)

Reviewers

Ioi Lam (@iklam - Reviewer) Review applies to 814fd3fd
Calvin Cheung (@calvinccheung - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 20858

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2024-09-04T18:45:25Z

👋 Welcome back shade! 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-09-04T18:46:41Z

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

8338912: CDS: Segmented roots array

Reviewed-by: ccheung, 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 112 new commits pushed to the master branch:

89ca89c: 8338626: ClassLoaderExt::process_jar_manifest() should allow / separator on Windows
3e0da58: 8333843: Provide guidelines on MemorySegment to read strings with known lengths
3c4d15b: 8334301: Errors in jpackage man page
4d01178: 8339927: Man page update for deprecating jhsdb debugd for removal
bd44cf8: 8330302: strace004 can still fail
8a4ea09: 8336492: Regression in lambda serialization
358ff19: 8339727: Open source several AWT focus tests - series 1
0c36177: 8340089: Simplify SegmentBulkOperations::powerOfProperty
bacd046: 8321010: RISC-V: C2 RoundVF
5709c37: 8340081: Test java/foreign/TestLinker.java failed failed: missing permission java.lang.foreign.native.threshold.power.fill
... and 102 more: https://git.openjdk.org/jdk/compare/1353601dcc8f9ec3e12dea21dc61b3585a154b13...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.

openjdk · 2024-09-04T18:47:12Z

@shipilev 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 · 2024-09-04T18:50:27Z

Webrevs

iklam

I think the design is good. I have some suggestions for readability and simplification.

src/hotspot/share/cds/archiveHeapWriter.cpp

src/hotspot/share/cds/heapShared.cpp

src/hotspot/share/cds/archiveHeapWriter.cpp

iklam

Looks good. Just some nits on naming, etc.

src/hotspot/share/cds/archiveHeapWriter.cpp

src/hotspot/share/cds/archiveHeapLoader.cpp

src/hotspot/share/cds/heapShared.cpp

jianglizhou

@shipilev Glad to see this change. As mentioned in yesterday's premain meeting, we ran into the single roots array scalability issue trivially when experimenting with real world applications back in 2021. At the time, I reworked it to use a 'linked-roots-array' solution to accommodate a large number of roots. If the required size was larger than the limit, multiple 'roots' arrays were allocated. The last element in the current 'roots' array contained the next 'roots' array. The last element in the last 'roots' array was NULL. The multi-roots-array solution works with GC automatically, and resolves the scalability problem. Your segmented roots array change is in the same direction and is necessary to make CDS workable for real world usages.

shipilev · 2024-09-09T11:28:08Z

@shipilev Glad to see this change. As mentioned in yesterday's premain meeting, we ran into the single roots array scalability issue trivially when experimenting with real world applications back in 2021.

Good to know this is not only the problem with our tests, but also a real-world issue!

At the time, I reworked it to use a 'linked-roots-array' solution to accommodate a large number of roots. If the required size was larger than the limit, multiple 'roots' arrays were allocated. The last element in the current 'roots' array contained the next 'roots' array.

Right, I actually started with something like this: an array of slices, like Object[][]. But then I quickly realized that CDS is manipulating "objects" outside of the heap, so constructing any object graph is getting massive frowns from GC barriers code, and it comes with a headache for relocation. Notably, storing the reference to a segment into anywhere, like in 1-st level array is problematic.

What we arrived here is basically Object[][], but we don't have a 1-st level array, we just use the layout encoded by HeapRootSegments to identify where the segments are, instead.

shipilev · 2024-09-09T17:53:29Z

I don't quite understand the Windows DeterministicDump failure. Is there anything specific about CDS and Windows that makes it only fail there?

iklam · 2024-09-09T19:02:08Z

I don't quite understand the Windows DeterministicDump failure. Is there anything specific about CDS and Windows that makes it only fail there?

This kind of failure usually can be diagnosed by diffing the map files that are generated the test case. Let me try to reproduce it on my side.

iklam · 2024-09-09T19:07:06Z

I don't quite understand the Windows DeterministicDump failure. Is there anything specific about CDS and Windows that makes it only fail there?

This kind of failure usually can be diagnosed by diffing the map files that are generated the test case. Let me try to reproduce it on my side.

Hmm, since you haven't change the logic of how the archive is created, except that you move the allocation of the root array(s) from the end to the beginning of the heap objects, I think it's probably caused by this:

class HeapRootSegments {
private:
  size_t _base_offset;
  size_t _count;
  int _roots_count;
  int _max_size_in_bytes;
  int _max_size_in_elems;
  // uninitialized padding on Windows

Maybe explicitly add an int _unused there?

iklam

Looks good to me. Just two small nits.

src/hotspot/share/cds/archiveHeapWriter.cpp

iklam · 2024-09-10T00:42:12Z

I don't quite understand the Windows DeterministicDump failure. Is there anything specific about CDS and Windows that makes it only fail there?

This kind of failure usually can be diagnosed by diffing the map files that are generated the test case. Let me try to reproduce it on my side.

Hmm, since you haven't change the logic of how the archive is created, except that you move the allocation of the root array(s) from the end to the beginning of the heap objects, I think it's probably caused by this:
class HeapRootSegments {
private:
  size_t _base_offset;
  size_t _count;
  int _roots_count;
  int _max_size_in_bytes;
  int _max_size_in_elems;
  // uninitialized padding on Windows
Maybe explicitly add an int _unused there?

Diff of the map files confirms this:

- heap_root_segments.roots_count:         2628
- heap_root_segments.seg_max_size_elems:  1048576
- heap_root_segments.seg_max_size_bytes:  131070
....
131c131
< 0x0000000000000300:   0010000000000a44 0000020a0001fffe 0000000000000002 000000000000f022   
---
> 0x0000000000000300:   0010000000000a44 000001e20001fffe 0000000000000002 000000000000f022   

    2628 = 0x00000a44
 1048576 = 0x00100000
  131080 = 0x00001ff2

The 0x000001e2 is garbage.

shipilev · 2024-09-10T08:12:19Z

Diff of the map files confirms this:

- heap_root_segments.roots_count:         2628
- heap_root_segments.seg_max_size_elems:  1048576
- heap_root_segments.seg_max_size_bytes:  131070
....
131c131
< 0x0000000000000300:   0010000000000a44 0000020a0001fffe 0000000000000002 000000000000f022   
---
> 0x0000000000000300:   0010000000000a44 000001e20001fffe 0000000000000002 000000000000f022   

    2628 = 0x00000a44
 1048576 = 0x00100000
  131080 = 0x00001ff2

The 0x000001e2 is garbage.

Whoa, that looks fragile! It feels safer to memset(0) the entire header then? Whack-a-mole-ing the alignment paddings every time we add a field is not convenient.

shipilev · 2024-09-10T08:27:15Z

Whoa, that looks fragile! It feels safer to memset(0) the entire header then? Whack-a-mole-ing the alignment paddings every time we add a field is not convenient.

Testing memset here, but would probably integrate it separately as JDK-8339830.

shipilev · 2024-09-10T08:33:12Z

Whoa, that looks fragile! It feels safer to memset(0) the entire header then? Whack-a-mole-ing the alignment paddings every time we add a field is not convenient.

Testing memset here, but would probably integrate it separately as JDK-8339830.

Hm, maybe I am misunderstanding this. I see there is the initialization here:

  _header = (FileMapHeader*)os::malloc(header_size, mtInternal);
  memset((void*)_header, 0, header_size);
  _header->populate(this,

...and here we read the header in its entirety:

  _header = (FileMapHeader*)os::malloc(gen_header->_header_size, mtInternal);
  os::lseek(fd, 0, SEEK_SET); // reset to begin of the archive
  size_t size = gen_header->_header_size;
  size_t n = ::read(fd, (void*)_header, (unsigned int)size);

I'll look around map files to understand it better.

iklam · 2024-09-10T09:01:34Z

Diff of the map files confirms this:
- heap_root_segments.roots_count:         2628
- heap_root_segments.seg_max_size_elems:  1048576
- heap_root_segments.seg_max_size_bytes:  131070
....
131c131
< 0x0000000000000300:   0010000000000a44 0000020a0001fffe 0000000000000002 000000000000f022   
---
> 0x0000000000000300:   0010000000000a44 000001e20001fffe 0000000000000002 000000000000f022   

    2628 = 0x00000a44
 1048576 = 0x00100000
  131080 = 0x00001ff2
The 0x000001e2 is garbage.
Whoa, that looks fragile! It feels safer to memset(0) the entire header then? Whack-a-mole-ing the alignment paddings every time we add a field is not convenient.

memset the header first is not going to help. The random bits are added by this call:

void set_heap_root_segments(HeapRootSegments segments) { _heap_root_segments = segments; }

segments is passed in by value on the stack. Apparently VC++ will not zero-out the unused bits when pushing segments onto the stack, but will insist copying the unused bits when doing the assignment.

Maybe you can add overload the assignment operator to copy the fields individually. This will stop VC++ from copying the garbage, so it will leave the unused slot in its original (zeroed) state.

shipilev · 2024-09-10T09:17:12Z

segments is passed in by value on the stack. Apparently VC++ will not zero-out the unused bits when pushing segments onto the stack, but will insist copying the unused bits when doing the assignment.

Maybe you can add overload the assignment operator to copy the fields individually. This will stop VC++ from copying the garbage, so it will leave the unused slot in its original (zeroed) state.

Hrmpf. I thought implicit copy constructors do this right. Apparently not. Let me try and define the explicit copy constructor + assignment operator...

shipilev · 2024-09-10T15:09:31Z

Hrmpf. I thought implicit copy constructors do this right. Apparently not. Let me try and define the explicit copy constructor + assignment operator...

All right, I think this works. I believe we still explicitly default to trivial copy constructor + assignment operator, allow them to copy the entire representation, but we also make sure that representation has no garbage in its gaps.

I am running this through our testing. @iklam, if you can run this through your tests, it would be helpful too.

iklam · 2024-09-10T20:00:27Z

Hrmpf. I thought implicit copy constructors do this right. Apparently not. Let me try and define the explicit copy constructor + assignment operator...

All right, I think this works. I believe we still explicitly default to trivial copy constructor + assignment operator, allow them to copy the entire representation, but we also make sure that representation has no garbage in its gaps.

I am running this through our testing. @iklam, if you can run this through your tests, it would be helpful too.

I ran bacc5d8 in our CI for tiers 1-4 and didn't see any regressions.

shipilev · 2024-09-11T09:07:56Z

Our testing passes here as well. I also changed MIN_GC_REGION_ALIGNMENT = 256K locally, and ran runtime/cds tests with -XX:-UseCompressedOops, which was one of the scenarios that used to run into problems. These pass as well.

I think we are done here. We need a second Reviewer, maybe @calvinccheung?

calvinccheung

Looks good. I have couple of minor suggestions.
I'm also running tiers 1 - 4 testing with your patch. Results look good so far.

src/hotspot/share/cds/archiveUtils.cpp

src/hotspot/share/cds/filemap.cpp

src/hotspot/cpu/x86/nativeInst_x86.cpp

src/hotspot/cpu/x86/nativeInst_x86.hpp

shipilev · 2024-09-16T05:32:16Z

Thanks for reviews!

/integrate

openjdk · 2024-09-16T05:33:40Z

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

74add0e: 8340105: Expose BitMap::print_on in release builds
0e0f10f: 8340102: Move assert-only loop in OopMapSort::sort under debug macro
a0794e0: 8339639: Opensource few AWT PopupMenu tests
a8f143c: 8306679: com/sun/jdi/InterruptHangTest.java asserts with -Xcomp -Dmain.wrapper=Virtual options
c91fa27: 8339725: Concurrent GC crashed due to GetMethodDeclaringClass
fa502ec: 8339943: Frame not disposed in java/awt/dnd/DropActionChangeTest.java
fdfe503: 8335288: SunPKCS11 initialization will call C_GetMechanismInfo on unsupported mechanisms
3aa8338: 8340075: Autoconf bundle cannot run on read-only filesystem
37bf589: 8339847: Broken link to the dieharder distribution website in SplittableRandom
89c172a: 8340082: Use inline return tag in java.base
... and 113 more: https://git.openjdk.org/jdk/compare/1353601dcc8f9ec3e12dea21dc61b3585a154b13...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-09-16T05:33:49Z

@shipilev Pushed as commit dc00eb8.

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

Segmented heap roots

1761cf0

openjdk bot added the rfr Pull request is ready for review label Sep 4, 2024

openjdk bot added the hotspot-runtime hotspot-runtime-dev@openjdk.org label Sep 4, 2024

iklam reviewed Sep 5, 2024

View reviewed changes

shipilev added 5 commits September 6, 2024 16:39

Introduce helper methods, further simplifications

eced033

More touchups

d0d5699

More pre-integration cleanups

8fb2229

More touchups

7218424

Amend printout as well

7e76c42

iklam reviewed Sep 6, 2024

View reviewed changes

jianglizhou reviewed Sep 6, 2024

View reviewed changes

shipilev added 2 commits September 9, 2024 13:48

Renames, adjustments after review

736d184

Replace divisions with power-of-two math

814fd3f

iklam approved these changes Sep 9, 2024

View reviewed changes

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

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

openjdk bot added the ready Pull request is ready to be integrated label Sep 9, 2024

shipilev added 2 commits September 10, 2024 10:14

More adjustments

8d91302

Try memset-ing the header

77367df

openjdk bot removed the ready Pull request is ready to be integrated label Sep 10, 2024

More touchups

ddb8d7d

Make sure HeapRootSegments representation does not hold garbage

bacc5d8

calvinccheung approved these changes Sep 12, 2024

View reviewed changes

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

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

openjdk bot added the ready Pull request is ready to be integrated label Sep 12, 2024

Range checks and format specifier

6935865

openjdk bot removed the ready Pull request is ready to be integrated label Sep 13, 2024

calvinccheung reviewed Sep 13, 2024

View reviewed changes

src/hotspot/cpu/x86/nativeInst_x86.cpp Outdated Show resolved Hide resolved

src/hotspot/cpu/x86/nativeInst_x86.hpp Outdated Show resolved Hide resolved

Revert superfluous fluff

d2a7e6d

calvinccheung approved these changes Sep 13, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Sep 13, 2024

openjdk bot added the integrated Pull request has been integrated label Sep 16, 2024

openjdk bot closed this Sep 16, 2024

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

shipilev deleted the JDK-8338912-cds-segmented-roots branch January 8, 2025 12:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8338912: CDS: Segmented roots array #20858

8338912: CDS: Segmented roots array #20858

shipilev commented Sep 4, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Sep 4, 2024

openjdk bot commented Sep 4, 2024 •

edited

Loading

openjdk bot commented Sep 4, 2024

mlbridge bot commented Sep 4, 2024 •

edited

Loading

iklam left a comment

iklam left a comment

jianglizhou left a comment

shipilev commented Sep 9, 2024

shipilev commented Sep 9, 2024

iklam commented Sep 9, 2024

iklam commented Sep 9, 2024

iklam left a comment

iklam commented Sep 10, 2024

shipilev commented Sep 10, 2024

shipilev commented Sep 10, 2024

shipilev commented Sep 10, 2024

iklam commented Sep 10, 2024 •

edited

Loading

shipilev commented Sep 10, 2024 •

edited

Loading

shipilev commented Sep 10, 2024 •

edited

Loading

iklam commented Sep 10, 2024

shipilev commented Sep 11, 2024 •

edited

Loading

calvinccheung left a comment

shipilev commented Sep 16, 2024

openjdk bot commented Sep 16, 2024

openjdk bot commented Sep 16, 2024

8338912: CDS: Segmented roots array #20858

8338912: CDS: Segmented roots array #20858

Conversation

shipilev commented Sep 4, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Sep 4, 2024

openjdk bot commented Sep 4, 2024 • edited Loading

openjdk bot commented Sep 4, 2024

mlbridge bot commented Sep 4, 2024 • edited Loading

Webrevs

iklam left a comment

Choose a reason for hiding this comment

iklam left a comment

Choose a reason for hiding this comment

jianglizhou left a comment

Choose a reason for hiding this comment

shipilev commented Sep 9, 2024

shipilev commented Sep 9, 2024

iklam commented Sep 9, 2024

iklam commented Sep 9, 2024

iklam left a comment

Choose a reason for hiding this comment

iklam commented Sep 10, 2024

shipilev commented Sep 10, 2024

shipilev commented Sep 10, 2024

shipilev commented Sep 10, 2024

iklam commented Sep 10, 2024 • edited Loading

shipilev commented Sep 10, 2024 • edited Loading

shipilev commented Sep 10, 2024 • edited Loading

iklam commented Sep 10, 2024

shipilev commented Sep 11, 2024 • edited Loading

calvinccheung left a comment

Choose a reason for hiding this comment

shipilev commented Sep 16, 2024

openjdk bot commented Sep 16, 2024

openjdk bot commented Sep 16, 2024

shipilev commented Sep 4, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Sep 4, 2024 •

edited

Loading

mlbridge bot commented Sep 4, 2024 •

edited

Loading

iklam commented Sep 10, 2024 •

edited

Loading

shipilev commented Sep 10, 2024 •

edited

Loading

shipilev commented Sep 10, 2024 •

edited

Loading

shipilev commented Sep 11, 2024 •

edited

Loading