Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8250989: Consolidate buffer allocation code for CDS static/dynamic dumping #2296

Closed

Conversation

iklam
Copy link
Member

@iklam iklam commented Jan 28, 2021

Before this PR, when a static CDS archive is dumped, we would unconditionally allocate a 4GB ReservedSpace and use that as the buffer for writing the archive content. We usually don't need such a big ReservedSpace. this also unnecessarily complicates Metaspace::global_initialize().

After this PR, both we first load all the classes and then allocate a buffer with an appropriate size.

I also simplified the pointer relocation code that prepares the archive to be mapped at the "requested location" (usually 0x800000000), and improved the comments.

We used to have lots of special cases for adjusting pointers during dynamic dump. All of those are removed.

Reviewers, please start with

  • Header comments in archiveBuilder.hpp that describe the overall process of archive dumping
  • MetaspaceShared::initialize_for_static_dump()
  • ArchiveBuilder::reserve_buffer()
  • Comments around RelocateBufferToRequested in archiveBuilder.cpp

Progress

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

Issue

  • JDK-8250989: Consolidate buffer allocation code for CDS static/dynamic dumping

Reviewers

Download

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

@bridgekeeper
Copy link

bridgekeeper bot commented Jan 28, 2021

👋 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
Copy link

openjdk bot commented Jan 28, 2021

@iklam The following label will be automatically applied to this pull request:

  • hotspot

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 openjdk bot added the hotspot hotspot-dev@openjdk.org label Jan 28, 2021
@iklam
Copy link
Member Author

iklam commented Jan 28, 2021

/label remove hotspot
/label add hotspot-runtime

@openjdk openjdk bot removed the hotspot hotspot-dev@openjdk.org label Jan 28, 2021
@openjdk
Copy link

openjdk bot commented Jan 28, 2021

@iklam
The hotspot label was successfully removed.

@openjdk openjdk bot added the hotspot-runtime hotspot-runtime-dev@openjdk.org label Jan 28, 2021
@openjdk
Copy link

openjdk bot commented Jan 28, 2021

@iklam
The hotspot-runtime label was successfully added.

@iklam iklam marked this pull request as ready for review January 28, 2021 18:30
@openjdk openjdk bot added the rfr Pull request is ready for review label Jan 28, 2021
@mlbridge
Copy link

mlbridge bot commented Jan 28, 2021

Webrevs

Comment on lines 727 to 732
uintx ArchiveBuilder::any_to_offset(address p) const {
if (is_in_mapped_static_archive(p)) {
return p - _mapped_static_archive_bottom;
}
return buffer_to_offset(p);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to add an assert(DynamicDumpSharedSpaces) after the if statement at line 728?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added per suggestion.

out.shouldContain("ArchiveRelocationMode == 1: always allocate class space at an alternative address");
out.shouldContain("Relocating archive from");
}
OutputAnalyzer out = TestCommon.dumpBaseArchive(baseArchiveName, unlockArg, logArg);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the check for out.shouldContain("Relocating archive from") be kept?
Otherwise, no need to save the OutputAnalyzer returned from TestCommon.dumpBaseArchive.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added the out.shouldContain("Relocating archive from") back (so that the changes are the same as in ArchiveRelocationTest.java)

Comment on lines -616 to -620
Symbol* sym = vmSymbols::java_lang_Object();
const char* name = (const char*)sym->bytes();
int len = sym->utf8_length();
unsigned int hash = hash_symbol(name, len, _alt_hash);
assert(sym == _shared_table.lookup(name, hash, len), "sanity");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe keeping this within a DEBUG_ONLY block if it's still applicable?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check cannot be done anymore -- _shared_table is initialized with the "requested" addresses but symis still at "buffer" address, so the check will fail (I forgot whether the lookup would crash or the assert will fail).

Copy link
Member

@calvinccheung calvinccheung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The dynamic dumping code is cleaner after this change. Looks good.
I've a few minor comments.

Copy link
Member

@tstuefe tstuefe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Ioi,

Just some initial remarks. Had a cursory glance at this patch, but this is a very large change :) seems to be a good cleanup though.

I am trying to understand how allocation works now. Just to make sure I get the gist right, where before we had three cases (with UseCompressedClassPoitners):

  1. DumpSharedSpaces = cds+ccs in a 4G range, realized by just reserving 4g
  2. UseSharedSpaces = cds+ccs in a 4G range, realized with our complicated mapping dance
  3. no sharing, where class space is allocated like in old times

now you eliminated (1)? So the restriction that ccs has to be within narrow klass ptr range of cds does not apply anymore at dump time? And we can map ccs wherever?

You know, at some point a flowchart would be helpful.

// If UseCompressedClassPointers=1, we have two cases:
// a) if CDS is active (either dump time or runtime), it will create the ccs
// a) if CDS archive has been mapped, it will create the ccs
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is confusing. So, at this point the ccs archive has already be mapped? Where?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've changed it to:

  // a) if CDS archive(s) have been mapped, we would have already reserved the
  //    ccs (immediately above the mapped archives) and initialized
  //    the CompressedKlassPointers encoding.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, the confusing part is the order. The old comment used future tense, this uses past tense. So to the reader it looks like mapping may already have happened. But at this point in the code the archive has not been mapped yet, no?
Proposal:

  // If UseCompressedClassPointers=1, we have two cases:
  // a) if CDS is active (runtime, Xshare=on), it will create the class space
  //    for us, initialize it and set up CompressedKlassPointers encoding.
  //    Class space will be reserved above the mapped archives.
  // b) if CDS either deactivated (Xshare=off) or a static dump is to be done (Xshare:dump),
  //    we will create the class space on our own. It will be placed above the java heap,
  //    since we assume it has been placed in low
  //    address regions. We may rethink this (see JDK-8244943). Failing that,
  //    it will be placed anywhere.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good. I added it to the code.

// must be allocated near the cds such as that the compressed Klass pointer
// encoding can be used to en/decode pointers from both cds and ccs. Since
// Metaspace cannot do this (it knows nothing about cds), we do it for
// Metaspace here and pass it the space to use for ccs.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this whole comment is wrong now and obsolete?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, most of the work done under the old function MetaspaceShared::initialize_dumptime_shared_and_meta_spaces() are now unnecessary. Specifically, it doesn't initialize the "shared space" or "meta space" anymore. That's why I renamed it to MetaspaceShared::initialize_for_static_dump().

Now it only does the following:

  • reset SharedBaseAddress if too high
  • reserve _symbol_rs

===============
BTW, I have added comments at the top of archiveBuilder.hpp to give an overview of how the memory is reserved and relocated during the dump process.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the clarification.

@openjdk
Copy link

openjdk bot commented Feb 2, 2021

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

8250989: Consolidate buffer allocation code for CDS static/dynamic dumping

Reviewed-by: ccheung, coleenp

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.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Feb 2, 2021
Copy link
Member

@tstuefe tstuefe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Ioi,

not a full review, but the metaspace related changes to reservations look fine. See minor nits below.

This is unrelated to your patch: in compute_shared_base, you set SharedBaseAddress in case of an error. I think it should either be just a calculator function with no side effects - just returning the corrected value - or always set SharedBaseAddress and _requested_base_address always, returning void. That would be clearer.

One question, SharedBaseAddress vs MetaspaceShared::_requested_base_address : what is the difference? When do you use one, when the other?

Thanks, Thomas

MetaspaceShared::initialize_dumptime_shared_and_meta_spaces();
} else if (UseSharedSpaces) {
// If any of the archived space fails to map, UseSharedSpaces
// is reset to false.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to preserve this comment. It makes it easier to understand why the followup check needs to query UseSharedSpaces again instead of being placed in the else path.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. I added the comments back, but put it below the call toMetaspaceShared::initialize_runtime_shared_and_meta_spaces();

@@ -143,6 +143,14 @@ bool oopDesc::has_klass_gap() {
return UseCompressedClassPointers;
}

#if INCLUDE_CDS_JAVA_HEAP
void oopDesc::set_narrow_klass(narrowKlass nk) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make private and access via a CDS-local friend class?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure about that. Friend classes can access all private methods.

Copy link
Contributor

@coleenp coleenp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Some questions though.

@@ -161,7 +205,7 @@ class ArchiveBuilder : public StackObj {
DumpAllocStats* _alloc_stats;

// For global access.
static ArchiveBuilder* _singleton;
static ArchiveBuilder* _current;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is kind of a strange concept since ArchiveBuilder is a StackObj. Maybe it should just be static if only one thread uses it at a time?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And can a different thread call current() ? yikes.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ArchiveBuilder has temporarily allocated objects such as ArchiveBuilder::_ptrmap. These are easily freed when the ArchiveBuilder falls out of scope. For example, in DynamicArchive::dump().

current() should be callable only inside the VM thread, where the dumping operations take place. I'll add an assert for that.

@iklam
Copy link
Member Author

iklam commented Feb 6, 2021

Hi Ioi,

not a full review, but the metaspace related changes to reservations look fine. See minor nits below.

This is unrelated to your patch: in compute_shared_base, you set SharedBaseAddress in case of an error. I think it should either be just a calculator function with no side effects - just returning the corrected value - or always set SharedBaseAddress and _requested_base_address always, returning void. That would be clearer.

I changed compute_shared_base() to have no side effect.

One question, SharedBaseAddress vs MetaspaceShared::_requested_base_address : what is the difference? When do you use one, when the other?

I added comments in metaspaceShared.hpp:

  // This is the base address as specified by -XX:SharedBaseAddress during -Xshare:dump.
  // Both the base/top archives are written using this as their base address.
  //
  // During static dump: _requested_base_address == SharedBaseAddress.
  //
  // During dynamic dump: _requested_base_address is not always the same as SharedBaseAddress:
  // - SharedBaseAddress is used for *reading the base archive*. I.e., CompactHashtable uses
  //   it to convery offsets to pointers to Symbols in the base archive.
  //   The base archive may be mapped to an OS-selected address due to ASLR. E.g.,
  //   you may have SharedBaseAddress == 0x00ff123400000000.
  // - _requested_base_address is used for *writing the output archive*. It's usually
  //   0x800000000 (unless it was set by -XX:SharedBaseAddress during -Xshare:dump).
  static char* requested_base_address() {
    return _requested_base_address;
  }

Thanks!
Ioi

@tstuefe
Copy link
Member

tstuefe commented Feb 6, 2021

Hi Ioi,
not a full review, but the metaspace related changes to reservations look fine. See minor nits below.
This is unrelated to your patch: in compute_shared_base, you set SharedBaseAddress in case of an error. I think it should either be just a calculator function with no side effects - just returning the corrected value - or always set SharedBaseAddress and _requested_base_address always, returning void. That would be clearer.

I changed compute_shared_base() to have no side effect.

One question, SharedBaseAddress vs MetaspaceShared::_requested_base_address : what is the difference? When do you use one, when the other?

I added comments in metaspaceShared.hpp:

  // This is the base address as specified by -XX:SharedBaseAddress during -Xshare:dump.
  // Both the base/top archives are written using this as their base address.
  //
  // During static dump: _requested_base_address == SharedBaseAddress.
  //
  // During dynamic dump: _requested_base_address is not always the same as SharedBaseAddress:
  // - SharedBaseAddress is used for *reading the base archive*. I.e., CompactHashtable uses
  //   it to convery offsets to pointers to Symbols in the base archive.
  //   The base archive may be mapped to an OS-selected address due to ASLR. E.g.,
  //   you may have SharedBaseAddress == 0x00ff123400000000.
  // - _requested_base_address is used for *writing the output archive*. It's usually
  //   0x800000000 (unless it was set by -XX:SharedBaseAddress during -Xshare:dump).
  static char* requested_base_address() {
    return _requested_base_address;
  }

Thanks!
Ioi

Much clearer, thanks!

Cheers, Thomas

//
// During dynamic dump: _requested_base_address is not always the same as SharedBaseAddress:
// - SharedBaseAddress is used for *reading the base archive*. I.e., CompactHashtable uses
// it to convery offsets to pointers to Symbols in the base archive.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps:
s/convery/convey/

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for spotting it. It's "convert". I'll fixed it :-)

@@ -348,6 +350,7 @@ class ArchiveBuilder : public StackObj {
}

static ArchiveBuilder* current() {
assert_is_vm_thread();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, thank you!

@iklam
Copy link
Member Author

iklam commented Feb 7, 2021

/integrate

@openjdk openjdk bot closed this Feb 7, 2021
@openjdk 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 Feb 7, 2021
@openjdk
Copy link

openjdk bot commented Feb 7, 2021

@iklam Pushed as commit c5ff454.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot-runtime hotspot-runtime-dev@openjdk.org integrated Pull request has been integrated
5 participants