8263002: Remove CDS MiscCode region #2861

iklam · 2021-03-07T06:26:00Z

The CDS MiscCode region is used for:
(a) C++ vtables
(b) Method trampolines

(a) can be moved to the ReadWrite region
(b) were introduced in JDK-8145221 so we can delay writing into Methods. This was intended to improve copy-on-write sharing to reduce memory footprint. However, this hasn't been shown to have any significant effect (footprint of metadata usually is much smaller than the Java heap), and introduces a lot of complexity in the HotSpot code.

Removing (b) will make it easier to implement JDK-8026297 (Generating AdapterHandlerEntry during CDS dump), which will further improve start-up time.

============
Other benefits of removing the MiscCode region:

We no longer have a read/write/executable region. This address the concern in JDK-8262922.
We can enable CDS on macOS/AArch64, which does not allow read/write/executable regions. (JDK-8253795)

Progress

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

Issue

JDK-8263002: Remove CDS MiscCode region

Reviewers

Coleen Phillimore (@coleenp - Reviewer) ⚠️ Review applies to 1c6ac95
David Holmes (@dholmes-ora - Reviewer) ⚠️ Review applies to a3e4f25

Download

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

bridgekeeper · 2021-03-07T06:26:59Z

👋 Welcome back iklam! 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 · 2021-03-07T06:28:02Z

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

iklam · 2021-03-07T06:36:39Z

/label remove serviceability

openjdk · 2021-03-07T06:37:05Z

@iklam
The serviceability label was successfully removed.

mlbridge · 2021-03-07T06:41:21Z

Webrevs

coleenp

wow. Looks good to me.

coleenp · 2021-03-08T23:50:30Z

src/hotspot/share/memory/archiveBuilder.cpp

-        *p += _buffer_to_requested_delta;
-        assert(_builder->is_in_requested_static_archive(*p), "new pointer must point inside requested archive");
+        if (!_builder->is_in_buffer_space(*p)) {
+          tty->print_cr("ohashii %p %p", p, *p);


What is ohashii ? Should this be an assert? Or log_error instead of tty->print

This was debug code that I left behind by mistake. I've removed it.

coleenp · 2021-03-08T23:53:48Z

src/hotspot/share/memory/cppVtables.cpp

@@ -215,7 +215,7 @@ CppVtableInfo** CppVtables::_index = NULL;
 char* CppVtables::dumptime_init() {
  assert(DumpSharedSpaces, "must");
  size_t vtptrs_bytes = _num_cloned_vtable_kinds * sizeof(CppVtableInfo*);
-  _index = (CppVtableInfo**)ArchiveBuilder::current()->mc_region()->allocate(vtptrs_bytes);
+  _index = (CppVtableInfo**)ArchiveBuilder::current()->rw_region()->allocate(vtptrs_bytes);


I thought these would be read-only?

The vtables contain function pointers and need to be updated at runtime to match the latest location of libjvm.so, so they need to be R/W.

coleenp · 2021-03-08T23:54:17Z

src/hotspot/share/memory/dumpAllocStats.cpp

@@ -107,7 +103,7 @@ void DumpAllocStats::print_stats(int ro_all, int rw_all, int mc_all) {
                       all_count, all_bytes, all_perc);

  assert(all_ro_bytes == ro_all, "everything should have been counted");
-  assert(all_rw_bytes == rw_all, "everything should have been counted");
+//assert(all_rw_bytes == rw_all, "everything should have been counted"); FIXME!


? Does this need to be fixed?

This is fixed in the latest version. I added accounting for the vtables.

openjdk · 2021-03-09T00:03:33Z

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

8263002: Remove CDS MiscCode region

Reviewed-by: coleenp, dholmes

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 no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential 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.

…izes

dholmes-ora

Hi Ioi,

This looks really good from a code simplification point of view!

I can't claim to fully understand all the code involved though.

Thanks,
David

iklam · 2021-03-10T06:07:04Z

Thanks @coleenp and @dholmes-ora for the review!
/integrate

openjdk · 2021-03-10T06:08:03Z

@iklam Since your change was applied there has been 1 commit pushed to the master branch:

67ea3bd: 8263102: Expand documention of Method.isBridge

Your commit was automatically rebased without conflicts.

Pushed as commit d8a9c3c.

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

y1yang0 · 2021-03-20T05:20:51Z

Hi @iklam, sorry to bother you, I found there are some comments that still refer to lingering mc regions.

iklam · 2021-03-22T20:01:57Z

Hi @iklam, sorry to bother you, I found there are some comments that still refer to lingering mc regions.

Thanks @kelthuzadx , I have filed JDK-8263998 (Remove mentions of mc region in comments)

ioilam added 2 commits March 6, 2021 16:35

8263002: Remove CDS MiscCode region

7637e5a

remove MC region

1c6ac95

openjdk bot added serviceability serviceability-dev@openjdk.org hotspot hotspot-dev@openjdk.org labels Mar 7, 2021

openjdk bot removed the serviceability serviceability-dev@openjdk.org label Mar 7, 2021

iklam marked this pull request as ready for review March 7, 2021 06:37

openjdk bot added the rfr Pull request is ready for review label Mar 7, 2021

iklam changed the title ~~8263002 Remove CDS MiscCode region~~ 8263002: Remove CDS MiscCode region Mar 8, 2021

coleenp approved these changes Mar 9, 2021

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Mar 9, 2021

ioilam added 2 commits March 8, 2021 20:42

@coleenp review: remove temp debug code; fixed accounting of vtable s…

ab435ab

…izes

bumped CURRENT_CDS_ARCHIVE_VERSION by one since format has changed

a3e4f25

dholmes-ora approved these changes Mar 9, 2021

View reviewed changes

ioilam added 2 commits March 9, 2021 16:48

Merge branch 'master' into 8263002-remove-cds-mc-region

b4c4827

Merge branch 'master' into 8263002-remove-cds-mc-region

a74edc2

openjdk bot closed this Mar 10, 2021

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 Mar 10, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8263002: Remove CDS MiscCode region #2861

8263002: Remove CDS MiscCode region #2861

iklam commented Mar 7, 2021 •

edited by openjdk bot

Loading

bridgekeeper bot commented Mar 7, 2021

openjdk bot commented Mar 7, 2021

iklam commented Mar 7, 2021

openjdk bot commented Mar 7, 2021

mlbridge bot commented Mar 7, 2021 •

edited

Loading

coleenp left a comment

coleenp Mar 8, 2021

iklam Mar 9, 2021

coleenp Mar 8, 2021

iklam Mar 9, 2021

coleenp Mar 8, 2021

iklam Mar 9, 2021

openjdk bot commented Mar 9, 2021 •

edited

Loading

dholmes-ora left a comment

iklam commented Mar 10, 2021

openjdk bot commented Mar 10, 2021

y1yang0 commented Mar 20, 2021

iklam commented Mar 22, 2021

8263002: Remove CDS MiscCode region #2861

8263002: Remove CDS MiscCode region #2861

Conversation

iklam commented Mar 7, 2021 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Download

bridgekeeper bot commented Mar 7, 2021

openjdk bot commented Mar 7, 2021

iklam commented Mar 7, 2021

openjdk bot commented Mar 7, 2021

mlbridge bot commented Mar 7, 2021 • edited Loading

Webrevs

coleenp left a comment

Choose a reason for hiding this comment

coleenp Mar 8, 2021

Choose a reason for hiding this comment

iklam Mar 9, 2021

Choose a reason for hiding this comment

coleenp Mar 8, 2021

Choose a reason for hiding this comment

iklam Mar 9, 2021

Choose a reason for hiding this comment

coleenp Mar 8, 2021

Choose a reason for hiding this comment

iklam Mar 9, 2021

Choose a reason for hiding this comment

openjdk bot commented Mar 9, 2021 • edited Loading

dholmes-ora left a comment

Choose a reason for hiding this comment

iklam commented Mar 10, 2021

openjdk bot commented Mar 10, 2021

y1yang0 commented Mar 20, 2021

iklam commented Mar 22, 2021

iklam commented Mar 7, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented Mar 7, 2021 •

edited

Loading

openjdk bot commented Mar 9, 2021 •

edited

Loading