jdk icon indicating copy to clipboard operation
jdk copied to clipboard

8288897: Clean up node dump code

Open eme64 opened this issue 2 years ago • 7 comments

I recently did some work in the area of Node::dump and Node::find, see JDK-8287647 and JDK-8283775.

This change sets cleans up the code around, and tries to reduce code duplication.

Things I did:

  • remove Node::related. It was added 7 years ago, with JDK-8004073. However, it was not extended to many nodes, and hence it is incomplete, and nobody I know seems to use it.
  • refactor dump(int) to use dump_bfs (reduce code duplication).
  • redefine categories in dump_bfs, focusing on output types. Mixed type is now also control if it has control output, and memory if it has memory output, etc. Plus, a node is also in the control category if it is_CFG. This makes dump_bfs much more usable, to traverse control and memory flow.
  • Other small cleanups, like replacing rarely used dump functions with dump, making removing dead code, make some functions private
  • Adding call from debugger comment to VM functions that are useful in debugger
  • rename find_node_by_name to find_nodes_by_name and find_node_by_dump to find_nodes_by_dump.
  • remove now unused dump indent compiler flag PrintIdealIndentThreshold (notproduct)

Progress

  • [x] Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • [x] Change must not contain extraneous whitespace
  • [x] Commit message must refer to an issue

Issue

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/9234/head:pull/9234
$ git checkout pull/9234

Update a local copy of the PR:
$ git checkout pull/9234
$ git pull https://git.openjdk.org/jdk pull/9234/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 9234

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/9234.diff

eme64 avatar Jun 22 '22 11:06 eme64

:wave: Welcome back epeter! 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.

bridgekeeper[bot] avatar Jun 22 '22 11:06 bridgekeeper[bot]

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

  • hotspot-compiler

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[bot] avatar Jun 22 '22 11:06 openjdk[bot]

@chhagedorn

Have you checked that the printed node order with dump(X) is the same as before? I'm not sure if that is a strong requirement.

I did try to make sure that the output of dump stays equivalent. As far as I manually inspected, they are. The visit order is the same, and the same nodes are dumped.

eme64 avatar Jul 08 '22 15:07 eme64

@chhagedorn

Have you checked that the printed node order with dump(X) is the same as before? I'm not sure if that is a strong requirement.

I did try to make sure that the output of dump stays equivalent. As far as I manually inspected, they are. The visit order is the same, and the same nodes are dumped.

I also verify that. root()->dump(9999) is still same. furthermore, I update it with colorful style. it looks pretty cool,huh?

-    root()->dump(9999);
+    tty->print_raw("AFTER: ");
+    tty->print_raw_cr(phase_name);
+    root()->dump_bfs(9999, nullptr, "+#$S");

Screen Shot 2022-07-14 at 12 20 46 AM

navyxliu avatar Jul 14 '22 07:07 navyxliu

@eme64 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!

bridgekeeper[bot] avatar Aug 12 '22 05:08 bridgekeeper[bot]

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

8288897: Clean up node dump code

Reviewed-by: chagedorn, xliu

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

  • 0c40128fec41cf69821dbf7f1b19600560e8ac12: 7194212: NPE in Flow.visitIdent
  • 6eb7c3ad0ee6fc187172e68cc838797981203b39: 8290732: JNI DestroyJavaVM can start shutdown when one non-daemon thread remains
  • 083e014d0caf673f5da04229ba263f45724cb418: 8292233: Increase symtab hash table size
  • 45e5b31a183e2ddca8f8d10a922b20af97efdaff: 8292244: Remove unnecessary include directories
  • 9bfffa082e85372ec39a0fdab6d5f2c175162246: 8291945: Add OSInfo API for static OS information
  • bd5855337c9eebc0044fd467fa39a671e260f891: 8290833: Remove ConstantPoolCache::walk_entries_for_initialization()
  • 755ecf6b7384e67ccb51c4498f94336631db690d: 8292153: x86: Represent Registers as values
  • dedc05cb40617f7b7e2cc235528b1892dcba4cd3: 8291640: java/beans/XMLDecoder/8028054/Task.java should use the 3-arg Class.forName
  • 3d20a8b20a636e4c11ad1568b011191726b45b90: 8291959: FileFontStrike#initNative does not properly initialize IG Table on Windows
  • a28ab7b62abcfce56425d62d5a8162d8f1623393: 8288568: Reduce runtime of java.security microbenchmarks
  • ... and 321 more: https://git.openjdk.org/jdk/compare/c7c20661eee727ed8354b19723c359ae7c2d4bd8...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[bot] avatar Aug 12 '22 08:08 openjdk[bot]

Thanks @chhagedorn @navyxliu for the review and comments! /integrate

eme64 avatar Sep 05 '22 14:09 eme64

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

  • 9833c025fd7daf6bb1be81d93148a4204a9f184c: 8292946: GC lock/jni/jnilock001 test failed "assert(gch->gc_cause() == GCCause::_scavenge_alot || !gch->incremental_collection_failed()) failed: Twice in a row"
  • d7536588b38e368eaa6395bcbcc6724a39303fc5: 8293107: GHA: Bump to Ubuntu 22.04
  • d4e3e69505db1b114afec2f6a61acf1261a8e69c: 8293344: JDK-8242181 broke stack printing for non-attached threads
  • 2c61efe3de9095631077aaf67edd2695edac6259: 8282648: Weaken the InflaterInputStream specification in order to allow faster Zip implementations
  • e31c537f0583261ac326cb74fd90c155629e1b50: 8293224: Add link to openjdk.org/jtreg/ from doc/testing
  • 955baa3c44805538df5f95ccc91d2d5a08c85b81: 8267374: macOS: Option+Up/Down Arrow don't traverse to beginning/end of line in JTextArea
  • 8df671c908d10b586540cc916a91e083c6098648: 8293355: JDK-8293167 included bad copyright header
  • 5bed9f767507bb0f123247d149ead84d2d635f52: 8293290: RISC-V: Explicitly pass a third temp register to MacroAssembler::store_heap_oop
  • 48b3ab02f9339f97e49f775c9d5d3a598a158abf: 8293167: Memory leak in JfrThreadSampler if stackdepth is larger than default (64)
  • 4067321ef413cc6c934aa5e7ec652c56dfdf81eb: 8291586: Broken links in JVMTI specification
  • ... and 592 more: https://git.openjdk.org/jdk/compare/c7c20661eee727ed8354b19723c359ae7c2d4bd8...master

Your commit was automatically rebased without conflicts.

openjdk[bot] avatar Sep 05 '22 14:09 openjdk[bot]

@eme64 Pushed as commit dbb2c4b6ac01d2a3367a2354213d3b4230dfbb96.

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

openjdk[bot] avatar Sep 05 '22 14:09 openjdk[bot]