8357568: IGV: Show NULL and numbers up to 4 characters in "Condense graph" filter #25393

chhagedorn · 2025-05-22T13:46:39Z

When using the "Condense graph" filter in IGV, it would be useful to show NULL and numbers wider than 2 characters instead of P and I/L (fallback for larger numbers), respectively. There is a comment in idealGrapPrinter.cpp which says that maximally 2 chars are allowed for numbers:

jdk/src/hotspot/share/opto/idealGraphPrinter.cpp

Line 646 in 428d33e

// max. 2 chars allowed

But we already allow larger entries today:

I there propose to use NULL and allow up to 4 characters for numbers which could be a good trade-off between shortness and expressiveness. This allows us to quickly see null checks and larger constants.

Without patch:

With patch:

Thanks,
Christian

Progress

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

Issue

JDK-8357568: IGV: Show NULL and numbers up to 4 characters in "Condense graph" filter (Enhancement - P4)

Reviewers

Manuel Hässig (@mhaessig - Author) Review applies to 4f77d003
Tobias Hartmann (@TobiHartmann - Reviewer)
Marc Chevalier (@marc-chevalier - Committer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 25393

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

Using diff file

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

Using Webrev

Link to Webrev Comment

…raph" filter

bridgekeeper · 2025-05-22T13:47:42Z

👋 Welcome back chagedorn! 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 · 2025-05-22T13:48:05Z

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

8357568: IGV: Show NULL and numbers up to 4 characters in "Condense graph" filter

Reviewed-by: thartmann, mchevalier, mhaessig

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

aac287e: 8353581: Support for import module in JShell's code completion
4a4209f: 8357685: Change the type of Integer::digits from char[] to byte[]
b034710: 8356629: Incorrect use of {@linkplain} in java.sql
... and 77 more: https://git.openjdk.org/jdk/compare/a0cdf36bdfeca9cd8b669859700d63d5ee627458...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 · 2025-05-22T13:48:47Z

@chhagedorn 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.

mlbridge · 2025-05-22T13:52:06Z

Webrevs

mhaessig

That is a nice convenience. Thank you for the improvement.

It looks good to me 🙂

chhagedorn · 2025-05-22T14:40:40Z

Thanks Manuel for your review! As suggested, I also removed the limit for parameters which cannot exceed 255 which is more than 4 characters (e.g. P123).

kimbarrett · 2025-05-22T18:05:54Z

src/hotspot/share/opto/idealGraphPrinter.cpp

@@ -676,7 +672,11 @@ void IdealGraphPrinter::visit_node(Node* n, bool edges) {
      } else if (t->base() == Type::Return_Address) {
        print_prop(short_name, "RA");
      } else if (t->base() == Type::AnyPtr) {
-        print_prop(short_name, "P");
+        if (t->is_ptr()->ptr() == TypePtr::Null) {
+          print_prop(short_name, "NULL");


I'm surprised this doesn't trip over sources/TestNoNULL.java.

Actually it does, see failures in github actions testing:

Failed. Execution failed: `main' threw exception: java.lang.RuntimeException: Test found 1 usages of 'NULL' in source files. See errors above.

Interesting! You're right. I haven't checked the testing results, yet. Should we just change to Null or make an exclusion in TestNoNULL?

I don't think this should be given an exclusion in TestNoNULL.

If the printed name is supposed to be related to the compiler object TypePtr::Null then I suggest it should
be "Null". If it's supposed to be related to the Java null value, then "null" seems appropriate. And in the
diagram in the PR intro I see things printed as "NotNull" which might argue for "Null".

I guess Null is fine then. I only chose NULL to be in line with the other capitalized letters shown in IGV in the condensed view.

TobiHartmann

Looks good to me otherwise.

marc-chevalier

I like it.

marc-chevalier · 2025-05-23T07:28:45Z

src/hotspot/share/opto/idealGraphPrinter.cpp

-        // max. 2 chars allowed
-        if (value >= -9 && value <= 99) {
+        // Only use up to 4 chars and fall back to a generic "L" to keep it short.
+        if (value >= -999 && value <= 9999) {


Just for the sake of saying something, can we somewhat factor this test? And ideally, it could be marginally nicer, if I want to see 6 chars, to just replace a 4 into a 6 and it'd work magically. But I don't really see a way beside length(to_string(value)) <= max_length, and it's not really efficient so... If you have a brilliant idea, good. Otherwise, feel free to ignore: changing it as it is now is easy too.

Good point! I think we can directly check how many chars are written with snprintf_checked and use that as an indicator. I pushed an update.

chhagedorn · 2025-05-23T09:03:46Z

Pushed an update to use "Null", an update to count the number of chars written as suggested by @marc-chevalier, and also added a missing case when we just have

object = null

somewhere and having narrow oops. We currently miss this because we would have t->base() == NarrowOop.

TobiHartmann

Looks good to me.

chhagedorn · 2025-05-26T05:37:17Z

Thanks @mhaessig, @marc-chevalier and @TobiHartmann for your reviews!

chhagedorn · 2025-05-26T07:03:18Z

/integrate

openjdk · 2025-05-26T07:24:41Z

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

a50d3be: 8312198: [macos] metal pipeline - window rendering stops after display sleep
aac287e: 8353581: Support for import module in JShell's code completion
4a4209f: 8357685: Change the type of Integer::digits from char[] to byte[]
... and 78 more: https://git.openjdk.org/jdk/compare/a0cdf36bdfeca9cd8b669859700d63d5ee627458...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-05-26T07:25:17Z

@chhagedorn Pushed as commit 99f33b4.

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

8357568: IGV: Show NULL and numbers up to 4 characters in "Condense g…

1de489e

…raph" filter

openjdk bot added the rfr Pull request is ready for review label May 22, 2025

openjdk bot added the hotspot-compiler [email protected] label May 22, 2025

Increase number of parameters as suggested by Manuel

4f77d00

mhaessig approved these changes May 22, 2025

View reviewed changes

kimbarrett reviewed May 22, 2025

View reviewed changes

TobiHartmann approved these changes May 23, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label May 23, 2025

marc-chevalier approved these changes May 23, 2025

View reviewed changes

better way to check nof chars, also print narrow oop null

c4381a0

openjdk bot removed the ready Pull request is ready to be integrated label May 23, 2025

marc-chevalier approved these changes May 23, 2025

View reviewed changes

TobiHartmann approved these changes May 26, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label May 26, 2025

openjdk bot added the integrated Pull request has been integrated label May 26, 2025

openjdk bot closed this May 26, 2025

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels May 26, 2025

8357568: IGV: Show NULL and numbers up to 4 characters in "Condense graph" filter #25393

8357568: IGV: Show NULL and numbers up to 4 characters in "Condense graph" filter #25393

Uh oh!

Conversation

chhagedorn commented May 22, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented May 22, 2025

Uh oh!

openjdk bot commented May 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented May 22, 2025

Uh oh!

mlbridge bot commented May 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

mhaessig left a comment

Choose a reason for hiding this comment

Uh oh!

chhagedorn commented May 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kimbarrett May 22, 2025

Choose a reason for hiding this comment

Uh oh!

TobiHartmann May 23, 2025

Choose a reason for hiding this comment

Uh oh!

chhagedorn May 23, 2025

Choose a reason for hiding this comment

Uh oh!

kimbarrett May 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chhagedorn May 23, 2025

Choose a reason for hiding this comment

Uh oh!

TobiHartmann left a comment

Choose a reason for hiding this comment

Uh oh!

marc-chevalier left a comment

Choose a reason for hiding this comment

Uh oh!

marc-chevalier May 23, 2025

Choose a reason for hiding this comment

Uh oh!

chhagedorn May 23, 2025

Choose a reason for hiding this comment

Uh oh!

chhagedorn commented May 23, 2025

Uh oh!

TobiHartmann left a comment

Choose a reason for hiding this comment

Uh oh!

chhagedorn commented May 26, 2025

Uh oh!

chhagedorn commented May 26, 2025

Uh oh!

openjdk bot commented May 26, 2025

Uh oh!

openjdk bot commented May 26, 2025

Uh oh!

Uh oh!

chhagedorn commented May 22, 2025 •

edited by openjdk bot

Loading

openjdk bot commented May 22, 2025 •

edited

Loading

mlbridge bot commented May 22, 2025 •

edited

Loading

chhagedorn commented May 22, 2025 •

edited

Loading

kimbarrett May 23, 2025 •

edited

Loading