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

8267666: Add option to jcmd GC.heap_dump to use existing file #4183

Closed

Conversation

AntonKozlov
Copy link
Member

@AntonKozlov AntonKozlov commented May 25, 2021

Please review a small change that adds an option to GC.heap_dump to use an existing file.

The option is necessary if the target file is a predefined file-like object, like a named pipe. This opens up a lot of possibilities to process a heap dump without storing it to the file system first.

Reviews of the CSR linked to the bug would be appreciated as well.


Progress

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

Issue

  • JDK-8267666: Add option to jcmd GC.heap_dump to use existing file

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 4183

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

Using diff file

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented May 25, 2021

👋 Welcome back akozlov! 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 openjdk bot added the rfr label May 25, 2021
@openjdk
Copy link

@openjdk openjdk bot commented May 25, 2021

@AntonKozlov 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 label May 25, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented May 25, 2021

Webrevs

@plummercj
Copy link
Contributor

@plummercj plummercj commented May 25, 2021

/label serviceability

@openjdk openjdk bot added the serviceability label May 25, 2021
@openjdk
Copy link

@openjdk openjdk bot commented May 25, 2021

@plummercj
The serviceability label was successfully added.

@plummercj
Copy link
Contributor

@plummercj plummercj commented May 25, 2021

@AntonKozlov I thought there was recently a thread were you discussed this, but I can't find it. If there was one, can you include a link? Thanks.

@AntonKozlov
Copy link
Member Author

@AntonKozlov AntonKozlov commented May 26, 2021

AFAIK such enhancement was not discussed before. Please consider this review request also as a start of a discussion. The implementation is rather small, so I thought it was not worth a separate CFD in advance.

I added few details to the bug. The change does not depend on a particular operating system, but on Linux, for example, the new option allows sending the dump to the external LZ4 process. It provides a compressed dump of the heap 2x faster than it is possible now, at the speed comparable to writing uncompressed dump.

Thanks!

@linzang
Copy link
Contributor

@linzang linzang commented May 27, 2021

Hi @AntonKozlov,
I am not a reviewer, just a tiny hint that you may need to add test cases.
Thanks,
Lin

@linzang
Copy link
Contributor

@linzang linzang commented May 27, 2021

Moreover, it seems the /csr label is required

@AntonKozlov
Copy link
Member Author

@AntonKozlov AntonKozlov commented May 28, 2021

you may need to add test cases

A test is trivial. Just in case, I've added it.

it seems the /csr label is required

Is the label really required? I don't see any integrated RFR with the label. I won't push this before CSR is approved anyway.

Thanks!

@linzang
Copy link
Contributor

@linzang linzang commented May 28, 2021

Hi Anton,

Is the label really required? I don't see any integrated RFR with the label. I won't push this before CSR is approved anyway.

AFAIK, the csr label will be removed by the bot when it gets approved. I usually add it in PR when CSR is required, and maybe it could help reviewer notice it and hence review the CSR. Also it could help guarantee the PR not to be pushed when CSR is not approved.

BRs,
Lin

@AntonKozlov
Copy link
Member Author

@AntonKozlov AntonKozlov commented May 28, 2021

/csr needed

@openjdk openjdk bot added the csr label May 28, 2021
@openjdk
Copy link

@openjdk openjdk bot commented May 28, 2021

@AntonKozlov this pull request will not be integrated until the CSR request JDK-8267667 for issue JDK-8267666 has been approved.

@YaSuenag
Copy link
Member

@YaSuenag YaSuenag commented May 28, 2021

I think -f (force) is more suitable like cp on Linux - Maybe it should be discussed on CSR.

@AntonKozlov
Copy link
Member Author

@AntonKozlov AntonKozlov commented May 28, 2021

-f is overloaded with meanings in various Unix utilities, I'd like to keep it specific. For example, cp -f will actually do something different, it will delete the file and create a new one. jcmd -rewrite will reuse the same file.

@YaSuenag
Copy link
Member

@YaSuenag YaSuenag commented May 29, 2021

Hmm... it looks like the same result for the user between -f and -rewrite, and also many users will be easy to imagine the result in case of -f.

Let's see comments from other reviewers.

@AntonKozlov
Copy link
Member Author

@AntonKozlov AntonKozlov commented May 31, 2021

Agree, I would like to hear more feedback.

I still don't think we need to provide a (false) similarity with existing tools. For example:

  • cp succeeds if the target file exists; jcmd bails out in this case
  • cp -f unlinks and creates a new file (so it is just a convenient substitution to rm -f; cp); jcmd -rewrite rewrites the file -- this cannot be achieved without making changes to the Hotspot.

@AlanBateman
Copy link
Contributor

@AlanBateman AlanBateman commented Jun 2, 2021

I think the named pipe scenario needs more discussion. For the more simpler case of overwriting an existing file then opt-in in with an option like -overwrite might be clearer than -rewrite. It's important that we choose the right name because there are several commands that may need to do the same.

@schmelter-sap
Copy link
Contributor

@schmelter-sap schmelter-sap commented Jun 2, 2021

I would rename the option to -overwrite, since it has a clear meaning (to replace the old content with new one).

And to really implement the overwrite semantics, the file should be opened with O_TRUNC. Currently -rewrite just replaces the start of an already existing file with a the heap dump. If the original file was larger than the heap dump, we have trailing bytes, which would lead to errors when tools try to parse the dump.

And on Unix it might be a good idea to use O_NOCTTY, so we don't accidentally assign a controlling tty when dumping to a console ('ve never seen actual problems omitting this, but it seems safe to add it).

Even with this changes you can still write to a fifo/tty on Unix or a named pipe on Windows, since O_TRUNC is ignored for these types of files.

And since you already created a CSR, I would propose to close JDK-8263066 and instead try to get this into the mainline.

@AntonKozlov
Copy link
Member Author

@AntonKozlov AntonKozlov commented Jun 2, 2021

I have renamed the option to -overwrite. Nice catch about missing O_TRUNC, thanks! A case for the missing O_NOCTTY looks rather special, wouldn't it be a configuration issue?..

SInce JDK-8263066 suggests more enhancements, it may be worth keeping it to track the remaining ones.

@AlanBateman The patch does not do anything special to the named pipe use-case, although makes it possible. Do you suggest removing mentions of the use-case from the CSR? Otherwise, what concerns could I address?

Thanks!

@schmelter-sap
Copy link
Contributor

@schmelter-sap schmelter-sap commented Jun 3, 2021

Yeah, I tjhink the O_NOCTTY is more a theoretical consideration. I've never seen problems in practice.

@RealCLanger
Copy link
Contributor

@RealCLanger RealCLanger commented Jun 5, 2021

SInce JDK-8263066 suggests more enhancements, it may be worth keeping it to track the remaining ones.

No, the issue we're trying to solve with JDK-8263066 is basically the same as with JDK-8267666, so we'll close it. There's also JDK-8200579 which could be closed then.

@AlanBateman The patch does not do anything special to the named pipe use-case, although makes it possible. Do you suggest removing mentions of the use-case from the CSR? Otherwise, what concerns could I address?

I would suggest to change the CSR to use "overwrite" but other than that, it seems fine. Let's wait what Alan has to say but that would be my thinking.

@AntonKozlov
Copy link
Member Author

@AntonKozlov AntonKozlov commented Jun 5, 2021

I would suggest to change the CSR to use "overwrite" but other than that, it seems fine.

Right, thanks! Now it is fixed, CSR uses "overwrite".

@AlanBateman
Copy link
Contributor

@AlanBateman AlanBateman commented Jun 6, 2021

I would suggest to change the CSR to use "overwrite" but other than that, it seems fine. Let's wait what Alan has to say but that would be my thinking.

Using "-overwrite" to opt-in to overwrite an existing file looks good to me.

The CSR still has wording to suggest that the new option is also to support socket/other files. I read the PR as just the overwrite existing file case. If so then maybe those (two) sentences can be changed in the CSR to make it clearer.

@RealCLanger
Copy link
Contributor

@RealCLanger RealCLanger commented Jun 6, 2021

Thanks Alan for commenting on that one.

The CSR still has wording to suggest that the new option is also to support socket/other files. I read the PR as just the overwrite existing file case. If so then maybe those (two) sentences can be changed in the CSR to make it clearer.

I think it's ok to mention the use-cases like pipe support which is something that'll be enabled by this change. But feel free to remove it. I've marked the CSR as reviewed from my end.

@AntonKozlov
Copy link
Member Author

@AntonKozlov AntonKozlov commented Jun 6, 2021

Thanks for the comments! Indeed, the change is about the "overwrite" option only. The CSR describes why the option is necessary, and sockets and pipes are just examples. I have rephrased CSR to be more explicit about the option, but not examples.

@RealCLanger
Copy link
Contributor

@RealCLanger RealCLanger commented Jun 6, 2021

Thanks for the comments! Indeed, the change is about the "overwrite" option only. The CSR describes why the option is necessary, and sockets and pipes are just examples. I have rephrased CSR to be more explicit about the option, but not examples.

Perfect, looks good to me.

Copy link
Contributor

@schmelter-sap schmelter-sap left a comment

Looks good, thanks for the change.

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jul 6, 2021

@AntonKozlov This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@RealCLanger
Copy link
Contributor

@RealCLanger RealCLanger commented Jul 6, 2021

@AntonKozlov This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

Please keep this open :) We're waiting for the CSR. @jddarcy, is there any update?

@openjdk openjdk bot removed the csr label Jul 9, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jul 9, 2021

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

8267666: Add option to jcmd GC.heap_dump to use existing file

Reviewed-by: rschmelter, clanger

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

  • 5a74291: 8266565: Spec of ForwardingJavaFileManager/ForwardingFileObject/ForwardingJavaFileObject methods should mention delegation instead of being copied
  • 3d193ad: 8270082: Remove unnecessary gc_timer null check in ReferenceProcessorPhaseTimes
  • 676f1d7: 8270094: Shenandoah: Provide human-readable labels for test configurations
  • c93204c: 8269914: Factor out heap printing for G1 young and full gc
  • dfd6b2b: Merge
  • 6401633: 8269722: NPE in HtmlDocletWriter
  • 9acb2a6: 8270109: ProblemList 4 SA tests on macOS-aarch64
  • f46a917: 6766844: ByteArrayInputStream#read with a byte array of length 0 not consistent with InputStream when at EOF
  • 9e75f92: 8269738: AssertionError when combining pattern matching and function closure
  • 168af2e: 8269828: corrections in some instruction patterns for KNL x86 platform
  • ... and 674 more: https://git.openjdk.java.net/jdk/compare/a52c4ede2f043b7d4a234c7d06f91871312e9654...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 openjdk bot added the ready label Jul 9, 2021
@AntonKozlov
Copy link
Member Author

@AntonKozlov AntonKozlov commented Jul 12, 2021

/integrate

@openjdk
Copy link

@openjdk openjdk bot commented Jul 12, 2021

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

  • 8973867: 8269295: Verification time before/after young collection only covers parts of the verification
  • 6a9bc10: 8270184: [TESTBUG] Add coverage for jvmci ResolvedJavaType.toJavaName() for lambdas
  • 86a2008: 8051680: (ref) unnecessary process_soft_ref_reconsider
  • ac75a53: 8253779: Amalloc may be wasting space by overaligning
  • 68b6e11: 8270083: -Wnonnull errors happen with GCC 11.1.1
  • ec975c6: Merge
  • 6889a39: 8268826: Cleanup Override in Context-Specific Deserialization Filters
  • f791fdf: 8261147: C2: Node is wrongly marked as reduction resulting in a wrong execution due to wrong vector instructions
  • 1196b35: 8270151: IncompatibleClassChangeError on empty pattern switch statement case
  • 885f7b1: 8269146: Missing unreported constraints on pattern and other case label combination
  • ... and 690 more: https://git.openjdk.java.net/jdk/compare/a52c4ede2f043b7d4a234c7d06f91871312e9654...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Jul 12, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Jul 12, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jul 12, 2021

@AntonKozlov Pushed as commit 7cbb67a.

💡 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 integrated serviceability
7 participants