JDK-8302335: IGV: Bytecode not showing #12537

tobiasholenstein · 2023-02-13T15:09:11Z

Currently, IGV does not show the bytecode of a graph in the "Bytecode" window although the bytecode is present in the XML file.

Solution

Following is the <bytecodes> block of the XML file

<bytecodes>
<![CDATA[
   0 iconst_3
   1 istore_1
   2 bipush 9
   4 istore_2
   5 iload_2
   6 ifle 34
  0   bci: 6    BranchData          taken(0) displacement(112) not taken(0)
   9 iconst_2
  10 istore_3
  11 iload_3
  12 iload_2
  13 if_icmpge 28
  32  bci: 13   BranchData          taken(0) displacement(56)  not taken(0)
  16 iload_1
  17 istore_1
  18 iconst_1
  19 iload_3
  20 irem
  21 istore_1
  22 iinc #3 1
  25 goto 11
  64  bci: 25   JumpData            taken(0) displacement(-32)
  28 iinc #2 -1
  31 goto 5
  88  bci: 31   JumpData            taken(0) displacement(-88)
  34 return
]]>
</bytecodes>

The problem was that during paring IGV skipped all lines that started with an empty space: This just skipped all bytecodes. We only want to skip bytecode like 0 bci: 6 BranchData taken(0) displacement(112) not taken(0) . Meaning: whitespace - number - exactly two white spaces - and then arbitrary text.

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-8302335: IGV: Bytecode not showing

Reviewers

Roberto Castañeda Lozano (@robcasloz - Reviewer)
Tobias Hartmann (@TobiHartmann - Reviewer)
Xin Liu (@navyxliu - Committer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 12537

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

Using diff file

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

bridgekeeper · 2023-02-13T15:10:55Z

👋 Welcome back tholenstein! 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 · 2023-02-13T15:12:30Z

@tobiasholenstein 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 · 2023-02-13T15:40:12Z

Webrevs

robcasloz

Good catch! The issue with the more liberal matching of bytecodes proposed in this PR is that bytecode attributes are now also matched and treated as bytecodes, see e.g. the second last line in this screenshot:

Somewhere between JDK 20 b17 and JDK 20 b25 (most likely with the integration of JDK-8292699), the indentation in bytecode dumps produced by BytecodeTracer::print_method_codes() changed. The change introduced indentation for the bytecodes but did not update the indentation of their attributes, which I guess was unintended. Here is an example:

Before:

0 nofast_aload_0
1 invokevirtual 478 <java/lang/String.isLatin1()Z> 
  0   bci: 1    VirtualCallData     count(5119) nonprofiled_count(0) entries(0)

After:

   0 nofast_aload_0
   1 invokevirtual 478 <java/lang/String.isLatin1()Z> 
  0   bci: 1    VirtualCallData     count(5119) nonprofiled_count(0) entries(0)

I am not sure about what is the best solution to this, but ideally we would want to support the old and current indentation, and probably also fix and support the proper indentation of bytecode attributes in BytecodeTracer::print_method_codes().

A possible solution from IGV side is to rule out attribute lines, e.g. likes starting with a number then the bci keyword such as
56 bci: 4 BranchData taken(0) displacement(48)
or lines starting with non-digits such as
not taken(5119)
And so on. Another possible solution is to actually parse these attributes and display them somehow (e.g. as bytecode sub-items), since they actually convey useful information. What do you think?

tobiasholenstein · 2023-02-15T15:35:49Z

Good catch! The issue with the more liberal matching of bytecodes proposed in this PR is that bytecode attributes are now also matched and treated as bytecodes, see e.g. the second last line in this screenshot:

Somewhere between JDK 20 b17 and JDK 20 b25 (most likely with the integration of JDK-8292699), the indentation in bytecode dumps produced by BytecodeTracer::print_method_codes() changed. The change introduced indentation for the bytecodes but did not update the indentation of their attributes, which I guess was unintended. Here is an example:

Before:
0 nofast_aload_0
1 invokevirtual 478 <java/lang/String.isLatin1()Z> 
  0   bci: 1    VirtualCallData     count(5119) nonprofiled_count(0) entries(0)
After:
   0 nofast_aload_0
   1 invokevirtual 478 <java/lang/String.isLatin1()Z> 
  0   bci: 1    VirtualCallData     count(5119) nonprofiled_count(0) entries(0)
I am not sure about what is the best solution to this, but ideally we would want to support the old and current indentation, and probably also fix and support the proper indentation of bytecode attributes in BytecodeTracer::print_method_codes().

A possible solution from IGV side is to rule out attribute lines, e.g. likes starting with a number then the bci keyword such as 56 bci: 4 BranchData taken(0) displacement(48) or lines starting with non-digits such as not taken(5119) And so on. Another possible solution is to actually parse these attributes and display them somehow (e.g. as bytecode sub-items), since they actually convey useful information. What do you think?

You are right. I changed it such that we now support the old and new format. Now we just skip lines with the bci: keyword.
Regarding parsing and displaying bci information - I think this should be done in a different RFE

navyxliu · 2023-02-15T18:12:29Z

0 bci: 1 VirtualCallData count(5119) nonprofiled_count(0) entries(0)

Those are methoddata. Should we add a leading character such as '#' to distinct them from real bytecode? I believe that would simplify matching logic in IGV.

tobiasholenstein · 2023-02-16T12:09:51Z

0 bci: 1 VirtualCallData count(5119) nonprofiled_count(0) entries(0)

Those are methoddata. Should we add a leading character such as '#' to distinct them from real bytecode? I believe that would simplify matching logic in IGV.

This would require a VM change for a printing flag. E.g. -XX:+CIPrintMethodCodes uses that code as well - I think we should leave it as it is because the main use is intended to be human-readable and it could break parsing of other uses (that we are not aware of)

robcasloz

Looks good otherwise!

robcasloz · 2023-02-16T12:36:57Z

src/utils/IdealGraphVisualizer/Data/src/main/java/com/sun/hotspot/igv/data/InputMethod.java

        String[] strings = text.split("\n");
        int oldBci = -1;
        for (String s : strings) {
-            if (s.startsWith(" ")) {
+            if (isInfo.matcher(s).matches()) {
                // indented lines are extra textual information


Maybe update this comment?

openjdk · 2023-02-16T12:39:02Z

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

8302335: IGV: Bytecode not showing

Reviewed-by: rcastanedalo, thartmann, 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 493 new commits pushed to the master branch:

4ce493f: 8302225: SunJCE Provider doesn't validate key sizes when using 'constrained' transforms for AES/KW and AES/KWP
a39cf2e: 8301753: AppendFile/WriteFile has differences between make 3.81 and 4+
de80dd9: 8296010: AssertionError in annotationTargetType
e5042dd: 8302671: libawt has a memmove decay error
574b48c: 8302678: validate_source fails after JDK-8293313
90e0922: 8293313: NMT: Rework MallocLimit
f558a6c: 8298276: JFR: Update NMT events to make use of common periodic timestamp feature
a58fa6e: 8302514: Misleading error generated when empty class file encountered
3cc459b: 8302595: use-after-free related to GraphKit::clone_map
2e3cea0: 8302594: use-after-free in Node::destruct
... and 483 more: https://git.openjdk.org/jdk/compare/85d8bacb0ff4015941db53305e6a0d5d28391e1f...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.

TobiHartmann · 2023-02-16T12:41:01Z

The method data still shows up for me:

tobiasholenstein · 2023-02-16T13:03:09Z

The method data still shows up for me:

Thanks for catching that!
This is actually a bug in printing the bytecodes on the VM side: JDK-8302656
I adjusted the PR to work even in the presents of the bug (and still work when it is fixed)

TobiHartmann

Looks good!

navyxliu

LGTM. I am not a reviewer.

tobiasholenstein · 2023-02-17T14:25:04Z

thanks @robcasloz , @TobiHartmann and @navyxliu for the reviews!

/integrate

openjdk · 2023-02-17T14:26:42Z

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

57fde75: 8302113: Improve CRC32 intrinsic with crypto pmull on AArch64
b8c9d6c: 8302158: PPC: test/jdk/jdk/internal/vm/Continuation/Fuzz.java: AssertionError: res: false shouldPin: false
dc55a7f: 8302202: Incorrect desugaring of null-allowed nested patterns
c4ffe4b: 8301494: Replace NULL with nullptr in cpu/arm
4f1cffd: 8302674: Parallel: Remove unused methods in MutableNUMASpace
c91cd28: 8301481: Replace NULL with nullptr in os/windows
47ca577: 8301491: C2: java.lang.StringUTF16::indexOfChar intrinsic called with negative character argument
49eb68b: 8296158: Refactor the verification of CDS region checksum
655a712: 8301444: Port fdlibm hyperbolic transcendental functions to Java
b242eef: 8280419: Remove dead code related to VerifyThread and verify_thread()
... and 493 more: https://git.openjdk.org/jdk/compare/85d8bacb0ff4015941db53305e6a0d5d28391e1f...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-02-17T14:26:54Z

@tobiasholenstein Pushed as commit 57c9bc3.

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

show the bytecode in window

1e1f8d2

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Feb 13, 2023

tobiasholenstein marked this pull request as ready for review February 13, 2023 15:34

openjdk bot added the rfr Pull request is ready for review label Feb 13, 2023

robcasloz suggested changes Feb 15, 2023

View reviewed changes

support old and new bytecode format

6510ea2

robcasloz approved these changes Feb 16, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Feb 16, 2023

print IGV bytecode correctly even in the presents of bug JDK-8302656

7f9cc1e

TobiHartmann approved these changes Feb 16, 2023

View reviewed changes

robcasloz approved these changes Feb 16, 2023

View reviewed changes

navyxliu approved these changes Feb 16, 2023

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Feb 17, 2023

openjdk bot closed this Feb 17, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Feb 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JDK-8302335: IGV: Bytecode not showing #12537

JDK-8302335: IGV: Bytecode not showing #12537

tobiasholenstein commented Feb 13, 2023 •

edited by openjdk bot

Loading

bridgekeeper bot commented Feb 13, 2023

openjdk bot commented Feb 13, 2023

mlbridge bot commented Feb 13, 2023 •

edited

Loading

robcasloz left a comment

tobiasholenstein commented Feb 15, 2023

navyxliu commented Feb 15, 2023

tobiasholenstein commented Feb 16, 2023

robcasloz left a comment

robcasloz Feb 16, 2023

openjdk bot commented Feb 16, 2023 •

edited

Loading

TobiHartmann commented Feb 16, 2023

tobiasholenstein commented Feb 16, 2023

TobiHartmann left a comment

navyxliu left a comment

tobiasholenstein commented Feb 17, 2023

openjdk bot commented Feb 17, 2023

openjdk bot commented Feb 17, 2023

JDK-8302335: IGV: Bytecode not showing #12537

JDK-8302335: IGV: Bytecode not showing #12537

Conversation

tobiasholenstein commented Feb 13, 2023 • edited by openjdk bot Loading

Solution

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Feb 13, 2023

openjdk bot commented Feb 13, 2023

mlbridge bot commented Feb 13, 2023 • edited Loading

Webrevs

robcasloz left a comment

Choose a reason for hiding this comment

tobiasholenstein commented Feb 15, 2023

navyxliu commented Feb 15, 2023

tobiasholenstein commented Feb 16, 2023

robcasloz left a comment

Choose a reason for hiding this comment

robcasloz Feb 16, 2023

Choose a reason for hiding this comment

openjdk bot commented Feb 16, 2023 • edited Loading

TobiHartmann commented Feb 16, 2023

tobiasholenstein commented Feb 16, 2023

TobiHartmann left a comment

Choose a reason for hiding this comment

navyxliu left a comment

Choose a reason for hiding this comment

tobiasholenstein commented Feb 17, 2023

openjdk bot commented Feb 17, 2023

openjdk bot commented Feb 17, 2023

tobiasholenstein commented Feb 13, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Feb 13, 2023 •

edited

Loading

openjdk bot commented Feb 16, 2023 •

edited

Loading