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

8273929: Remove GzipRandomAccess in heap dump test #5556

Closed
wants to merge 1 commit into from

Conversation

linzang
Copy link
Contributor

@linzang linzang commented Sep 17, 2021

The class GzipRandomAccess is used to parse heap dump file generated from jcmd/jmap tools when testing.
It has the limitation that only gzip file which has "blocksize" header field could be sucessfully parsed.
We think this class can be removed for 2 reasons:

  1. The gzip heap dump file generated by jhsdb command does not contain the "blocksize" header field, so the GzipRandomAccess can not parse the generated file successfully.
  2. The GzipInputStream could be used instead and then gziped heap dump file generated from both jcmd/jmap and jhsdb jmap could be parsed using same logic.
    Options

Progress

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

Issue

  • JDK-8273929: Remove GzipRandomAccess in heap dump test

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5556

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

Using diff file

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Sep 17, 2021

👋 Welcome back lzang! 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.

@linzang
Copy link
Contributor Author

@linzang linzang commented Sep 17, 2021

/label serviceability

@openjdk openjdk bot added rfr serviceability labels Sep 17, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Sep 17, 2021

@linzang
The serviceability label was successfully added.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Sep 17, 2021

Webrevs

in.close();
try (BufferedInputStream gzBis = new BufferedInputStream(access.asStream(0));
PositionDataInputStream pdin = new PositionDataInputStream(gzBis)) {
String deCompressedFile = "heapdump" + System.currentTimeMillis() + ".hprof";
Copy link
Contributor

@plummercj plummercj Sep 22, 2021

Choose a reason for hiding this comment

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

Is it necessary to create a file with the decompressed output rather than just stream the decompressed output to a FileInputStream?

Copy link
Contributor Author

@linzang linzang Sep 23, 2021

Choose a reason for hiding this comment

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

Correct! seems FileInputStream is enough, I will make the change. Thanks!

Copy link
Contributor Author

@linzang linzang Sep 26, 2021

Choose a reason for hiding this comment

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

Hi @plummercj,
Sorry for late response, I tried to avoid using the decompressed file, but it seems not simple as I expected.
The reason is that at line 121, the HprofReader require the filename and it use this file to create a ReadBuffer internally, and a RandomAccessFile will be created using the filename.
So I think may be it is not easy to make the change here, and maybe we could track it using a new bug if necessary, which may change the HprofReader implementation.
What do you think?

Copy link
Contributor

@plummercj plummercj Oct 5, 2021

Choose a reason for hiding this comment

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

Yes, it's fine then the way it currently is implemented. Thanks.

@linzang
Copy link
Contributor Author

@linzang linzang commented Oct 3, 2021

Dear Chris @plummercj,
May I ask your help to take look at this again? Do you think my expaination about "use of decompressed file" is OK?
Thanks!
Lin

in.close();
try (BufferedInputStream gzBis = new BufferedInputStream(access.asStream(0));
PositionDataInputStream pdin = new PositionDataInputStream(gzBis)) {
String deCompressedFile = "heapdump" + System.currentTimeMillis() + ".hprof";
Copy link
Contributor

@plummercj plummercj Oct 5, 2021

Choose a reason for hiding this comment

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

Yes, it's fine then the way it currently is implemented. Thanks.

@openjdk
Copy link

@openjdk openjdk bot commented Oct 5, 2021

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

8273929: Remove GzipRandomAccess in heap dump test

Reviewed-by: cjplummer, sspitsyn

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

  • 986ee5d: 8274670: Improve version string handling in SA
  • df7b0c7: 8274715: Implement forEach in Collections.CopiesList
  • d4e8712: 8274797: ProblemList resourcehogs/serviceability/sa/TestHeapDumpForLargeArray.java on macosx-x64
  • 4726267: 8274642: jdk/jshell/CommandCompletionTest.java fails with NoSuchElementException after JDK-8271287
  • 83b2219: 8273612: Fix for JDK-8272873 causes timeout in running some tests with -Xcomp
  • d34ec6c: 8274793: Suppress warnings on non-serializable non-transient instance fields in sun.net
  • 332f067: 8274729: Define Position.NOPOS == Diagnostic.NOPOS
  • 1e75203: 8274656: Remove default_checksum and safe_checksum_type from krb5.conf
  • 03d3c03: 8273670: Remove weak etypes from default krb5 etype list
  • c391e59: 8274244: ReportOnImportedModuleAnnotation.java fails on rerun
  • ... and 230 more: https://git.openjdk.java.net/jdk/compare/59b2478abd7f233531262b0fa190e027a785da79...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 Oct 5, 2021
@linzang
Copy link
Contributor Author

@linzang linzang commented Oct 7, 2021

Thanks all for your help reviewed this PR, I will push it now.

@linzang
Copy link
Contributor Author

@linzang linzang commented Oct 7, 2021

/integrate

@openjdk
Copy link

@openjdk openjdk bot commented Oct 7, 2021

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

  • 8319836: 8274546: Shenandoah: Remove unused ShenandoahUpdateRootsTask copy
  • d5ccfa2: 8274755: Replace 'while' cycles with iterator with enhanced-for in jdk.jdi
  • 29dcbb7: 8274716: JDWP Spec: the description for the Dispose command confuses suspend with resume.
  • 5762ec2: 8274780: ChannelInputStream.readNBytes(int) incorrectly calls readAllBytes()
  • 4e960fe: 8274497: Unnecessary Vector usage in AquaFileSystemModel
  • c833b4d: 8274329: Fix non-portable HotSpot code in MethodMatcher::parse_method_pattern
  • d57fb6f: 8274456: Remove jtreg tag manual=yesno java/awt/print/PrinterJob/PageDialogTest.java
  • 734d1fb: 8274211: Test man page that options are documented
  • 9561fea: 8273102: Delete deprecated for removal the empty finalize() in java.desktop module
  • 9945f7a: 8274318: Replace 'for' cycles with iterator with enhanced-for in java.management
  • ... and 252 more: https://git.openjdk.java.net/jdk/compare/59b2478abd7f233531262b0fa190e027a785da79...master

Your commit was automatically rebased without conflicts.

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

@openjdk openjdk bot commented Oct 7, 2021

@linzang Pushed as commit 340c715.

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

@linzang linzang deleted the rmGzip branch Oct 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated serviceability
3 participants