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

8283474: Include detailed heap object info in CDS map file #7895

Closed

Conversation

iklam
Copy link
Member

@iklam iklam commented Mar 21, 2022

The cds.map file produced by java -Xshare:dump -Xlog:cds+map=trace:file=cds.map:none:filesize=0 is useful in diagnosing problems with deterministic CDS archive files, such as JDK-8253495.

Currently, cds.map has information of each individual metaspace object. For example:

[ro region 0x00000008004ca000 - 0x0000000800c452e8 7844584 bytes]
0x00000008004ca000:
0x00000008004ca000: @@ Symbol 8 [Z
0x00000008004ca000: 5a5b0002e99effff
0x00000008004ca008: @@ Symbol 8 [C
0x00000008004ca008: 435b000299a9ffff
...
0x0000000800bcd838: @@ TypeArrayU1 16
0x0000000800bcd838: 3e00010000000006 0000000000000000
0x0000000800bcd848: @@ ConstMethod 104 java.lang.String java.lang.Object.toString()
0x0000000800bcd848: 0000000000000018 0000000800651da8 0000000000000000 000c00050000000d
0x0000000800bcd868: 0006001200250024 0006000100010002 ec0000b7590100bb 03b60002b60001b6
0x0000000800bcd888: b6ec0003b600e600 0003b60005b80004 05c000ffb00006b6 003b002400000000
0x0000000800bcd8a8: 000100000000003c

This RFE adds the following info into the cds.map file to describe the heap objects:

[closed heap region 0x00000007bf800000 - 0x00000007bf87e000 516096 bytes]
0x00000007bf800000: @@ Object [B
0x00000007bf800000: 00000003f021ab01 0000000000002090
0x00000007bf800010: @@ Object java.lang.String
0x00000007bf800010: 0000000d37cfa501 00000000000091d8 f7f0000000010100
...

Progress

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

Issue

  • JDK-8283474: Include detailed heap object info in CDS map file

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/7895/head:pull/7895
$ git checkout pull/7895

Update a local copy of the PR:
$ git checkout pull/7895
$ git pull https://git.openjdk.java.net/jdk pull/7895/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 7895

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/7895.diff

@bridgekeeper
Copy link

bridgekeeper bot commented Mar 21, 2022

👋 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 Mar 21, 2022

@iklam 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 openjdk bot added the hotspot-runtime hotspot-runtime-dev@openjdk.org label Mar 21, 2022
@iklam iklam marked this pull request as ready for review March 21, 2022 22:26
@openjdk openjdk bot added the rfr Pull request is ready for review label Mar 21, 2022
@mlbridge
Copy link

mlbridge bot commented Mar 21, 2022

Webrevs

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.

LGTM

@openjdk
Copy link

openjdk bot commented Mar 22, 2022

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

8283474: Include detailed heap object info in CDS map file

Reviewed-by: ccheung, stuefe

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:

  • 028fbf4: 8254935: Deprecate the PSSParameterSpec(int) constructor
  • 1dfa1ea: 8284094: Memory leak in invoker_completeInvokeRequest()
  • 943d4ee: 8284180: Some files missing newlines

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 openjdk bot added the ready Pull request is ready to be integrated label Mar 22, 2022
@mlbridge
Copy link

mlbridge bot commented Mar 28, 2022

Mailing list message from Ioi Lam on hotspot-runtime-dev:

Hi, could I get one more review? Thanks!

- Ioi

On 3/22/22 9:08 AM, Calvin Cheung wrote:

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,

nice, I had a fun time looking at the archive dump.

I wondered whether it would be more convenient to print out 32-bit words instead of 64-bit words, to better separate the narrow class word from the object start. But that would tear the mark word, so one would have to do mental gymnastics either way.

I tried this on 32-bit too, but this is not supported on 32bit?

Stupid question, the fact that all objects are having "01" as least byte means all are unlocked and we never had a GC running before dump time, right?

Cheers, Thomas

}
#endif
Copy link
Member

Choose a reason for hiding this comment

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

Not part of your patch, but as a casual reader I would like it if you reserved "write" and "dump" for actual archive writing, and used "log" consistently for logging. There are functions named "write_xxx" doing either, or both. Also, comment talks about "Dump all the data.." (at write_data) when all it does is logging.

I know we are within the context of "CDSLogger" here, so it's kind of obvious. But it would help bystanders to understand the code.

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 agree that using write_xxx is a bad idea. I changed all the functions to log_xxx and changed the comment to say "Log all the data".

Copy link
Member

Choose a reason for hiding this comment

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

Thanks a lot! That makes it a lot easier.

ResourceObj::C_HEAP,
mtClassShared,
HeapShared::oop_hash> OriginalObjectTable;
static OriginalObjectTable* _original_object_table;
Copy link
Member

Choose a reason for hiding this comment

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

Did a small test, and a hello world creates a map with 33k objects. So this will have a load factor > 2, is that okay?

Maybe we should create and fill this map only when logging. Its only used for logging, right? Why pay for this map always. Archive creation at build is already kind of slow.

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 changed the table size to 36137 and also use this table only when -Xlog:cds+map is enabled.

… oops; more efficient use of _original_object_table
@iklam
Copy link
Member Author

iklam commented Apr 1, 2022

Hi Ioi,

nice, I had a fun time looking at the archive dump.

Thank you so much for reviewing :-)

I wondered whether it would be more convenient to print out 32-bit words instead of 64-bit words, to better separate the narrow class word from the object start. But that would tear the mark word, so one would have to do mental gymnastics either way.

I changed the code to dump 32-bit words for the heap region if CompressedOops are used. Here's an example

0x00000007bf000af8: @@ Object java.lang.Class
0x00000007bf000af8:   701da101 0000001b 00015698 00000000 0001fe00 00000008 00000000 00000000
0x00000007bf000b18:   0000000e 00000000 00000000 00000000 00000000 00000000 00000000 00000000
0x00000007bf000b38:   00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
0x00000007bf000b58:   00000000 00000000 00000000 00000000 
0x00000007bf000b68: @@ Object [Ljava.lang.Object;
0x00000007bf000b68:   79955b01 00000047 00001238 00000010 f7f07d09 f7f02c99 f7f00002 f7f0155d
0x00000007bf000b88:   f7f03f89 f7f010ff f7f02eeb 00000000 00000000 00000000 00000000 00000000
0x00000007bf000ba8:   00000000 00000000 00000000 00000000 

I tried this on 32-bit too, but this is not supported on 32bit?

CDS object heap is not supported for 32-bit platforms.

The rest of the cds map should be printed with 32-bit words on 32-bit builds. The printing is done via os::print_hex_dump(..., sizeof(address), ...)

Stupid question, the fact that all objects are having "01" as least byte means all are unlocked and we never had a GC running before dump time, right?

We force all archived objects to be unlocked. Or else we would run into problems when trying to lock them at runtime!

int hash_original = obj->identity_hash();
archived_oop->set_mark(markWord::prototype().copy_set_hash(hash_original));
assert(archived_oop->mark().is_unlocked(), "sanity");

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.

Thank you for taking my suggestions. Looks good to me now (modulo the windows build errors, but it looks like this is simple to fix).

@iklam
Copy link
Member Author

iklam commented Apr 2, 2022

Thanks @calvinccheung and @tstuefe for the review.
/integrate

@openjdk
Copy link

openjdk bot commented Apr 2, 2022

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

  • 060a188: 8283865: riscv: Break down -XX:+UseRVB into seperate options for each bitmanip extension
  • e5e1aab: 8284068: riscv: should call Atomic::release_store in JavaThread::set_thread_state
  • 0b09f70: 8273553: sun.security.ssl.SSLEngineImpl.closeInbound also has similar error of JDK-8253368
  • afd0f5a: 8284190: disable G1RegionToSpaceMapper.largeStressAdjacent_vm on windows
  • 028fbf4: 8254935: Deprecate the PSSParameterSpec(int) constructor
  • 1dfa1ea: 8284094: Memory leak in invoker_completeInvokeRequest()
  • 943d4ee: 8284180: Some files missing newlines

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Apr 2, 2022
@openjdk openjdk bot closed this Apr 2, 2022
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Apr 2, 2022
@openjdk
Copy link

openjdk bot commented Apr 2, 2022

@iklam Pushed as commit c1e67b6.

💡 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
4 participants