8342469: Improve API documentation for java.lang.classfile.instruction #21625

liach · 2024-10-21T22:43:42Z

Improve the documentation for classfile instructions. Includes links to all opcodes, usage notes for instructions, and other various fixes.

API Diff: https://cr.openjdk.org/~liach/apidiff/cf-instr/java.base/module-summary.html
Javadoc: https://cr.openjdk.org/~liach/javadoc/cf-instr/java.base/java/lang/classfile/instruction/package-summary.html

Please review the associated CSR as well.

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
Change requires CSR request JDK-8344373 to be approved

Issues

JDK-8342469: Improve API documentation for java.lang.classfile.instruction (Sub-task - P4)
JDK-8344373: Improve API documentation for java.lang.classfile.instruction (CSR)

Reviewers

Adam Sotona (@asotona - Reviewer)
Joe Darcy (@jddarcy - Reviewer) Review applies to e8334c97

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 21625

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2024-10-21T22:44:32Z

👋 Welcome back liach! 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 · 2024-10-21T22:44:54Z

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

8342469: Improve API documentation for java.lang.classfile.instruction

Reviewed-by: asotona, darcy

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

9bd70ec: 8345888: Broken links in the JDK 24 JavaDoc API documentation, build 27
1def2d8: 8345700: tier{1,2,3}_compiler don't cover all compiler tests
d6b5264: 8345746: Remove :resourcehogs/compiler from :hotspot_slow_compiler
4f855d1: 8345818: Fix SM cleanup of parsing of System property resource.bundle.debug
a24b08f: 8345669: RISC-V: fix client build failure due to AlignVector after JDK-8343827
25d9deb: 8335367: [s390] Add support for load immediate on condition instructions.
a6277bb: 8340212: -Xshare:off -XX:CompressedClassSpaceBaseAddress=0x40001000000 crashes on macos-aarch64
165fd84: 8342987: Update --release 24 symbol information for JDK 24 build 27
06c44dd: 8345465: Fix performance regression on x64 after JDK-8345120
2979806: 8345795: Update copyright year to 2024 for hotspot in files where it was missed
... and 89 more: https://git.openjdk.org/jdk/compare/79eb77b782bd0c3cecee6c66b86f6f3e17054498...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 · 2024-10-21T22:45:22Z

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

core-libs

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.

liach · 2024-10-22T04:25:10Z

/csr needed

Proofread a little bit. Since this is huge and there may be mistakes, I will only create a CSR after some typos and problems have been fixed to reduce repeated updates to the CSR.

openjdk · 2024-10-22T04:25:53Z

@liach has indicated that a compatibility and specification (CSR) request is needed for this pull request.

@liach please create a CSR request for issue JDK-8342469 with the correct fix version. This pull request cannot be integrated until the CSR request is approved.

mlbridge · 2024-10-22T04:28:44Z

Webrevs

asotona

Great job done!
It would be good to have another pair of eyes to read the new doc, however it looks good to me.

…instruction

jddarcy · 2024-11-15T19:39:22Z

src/java.base/share/classes/java/lang/classfile/Opcode.java

    LCONST_1(RawBytecodeHelper.LCONST_1, 1, Kind.CONSTANT),

-    /** Push float constant 0 */
+    /**
+     * Push {@link TypeKind#FLOAT float} constant {@code 0}.


For the float and double constants, it would be a small improvement to write them out as floating-point literals, "0.0f" for a float zero, etc.

…instruction

liach · 2024-11-18T04:08:18Z

After offline discussions, I believe it is necessary to also include the data models of instructions in the javadoc; they will be removed from the java.lang.classfile top-level documentation later.

I am pleased with the current state of the documentation, and have submitted a CSR.
Please review the renderings:

API Diff: https://cr.openjdk.org/~liach/apidiff/cf-instr/java.base/module-summary.html
Javadoc: https://cr.openjdk.org/~liach/javadoc/cf-instr/java.base/java/lang/classfile/instruction/package-summary.html

(Omitted the constant rendering changes; committed locally, will push with other review fixes later)

asotona · 2024-11-19T09:40:25Z

src/java.base/share/classes/java/lang/classfile/instruction/CharacterRange.java

+ * }
+ * <p>
+ * Physically, a character range has the same structure; it is modeled by a
+ * {@link CharacterRangeInfo}.


I'm not sure how to explain "Physically" and "it is modeled by".
If "Physically" means specific byte serialization, then it is an entry in a CRT attribute with BCIs instead of labels.

"it is modeled by a CharacterRangeInfo" seems to me very confusing as this class (CharacterRange) is the primary model class.

Maybe we should explain the duplicity of some classfile structures accessible as attributes internal raw structures (CharacterRangeInfo) versus preferred primary models as labeled pseudo instructions (CharacterRange).

Done. I have simplified the record model and added dedicated info about CRI being independent from Code so it's used in the attribute. Same for LineNumber etc.

asotona · 2024-11-19T09:45:53Z

src/java.base/share/classes/java/lang/classfile/instruction/ConstantInstruction.java

+ * ConstantDesc}.
+ * <p>
+ * Physically, a constant-load instruction is polymorphic; nested interfaces in
+ * this interface model different constant instructions as records.


I'm not sure what the sentence "nested interfaces in this interface model different constant instructions as records" gives to users and on the other side what obligation does it mean from the JCK perspective?

Removed this confusing and meaningless sentence.

asotona · 2024-11-19T10:08:29Z

My personal feeling is that sentences like "Conceptually, Xy is a record..." and "Physically, Xy is a record..." are more confusing than explaining. My understanding of "Conceptually" is that the particular sealed interface xyInstruction represents the model concept. My understanding of "Physically" is that the particular chapter 6. paragraph of the JVMS describes the mechanics. And I don't see there any records (in particular Java meaning) .

… also the labelless info models

liach · 2024-11-20T23:04:09Z

I used the term "record" because they are something that may be pattern-matchable in the future; they are plain data objects. Similar representation is used in data model section of the java.lang.classfile package-level documentation.

Pushed another iteration (also updated CR) that solved some of the issues, including the constant format problems. I still think it is meaningful to move the huge data model from root package doc to individual model classes.

asotona · 2024-11-21T10:52:56Z

src/java.base/share/classes/java/lang/classfile/instruction/DiscontinuedInstruction.java

+     * the {@link ClassFile.ShortJumpsOption} controls how an invalid {@code jsr}
+     * instruction model is written by a {@link CodeBuilder}.
+     * <p>
+     * Jump subroutine instructions push a {@link TypeKind##returnAddress


I don't think we have a TypeKind##returnAddress.

This links to the index tag on returnAddress.

asotona · 2024-11-21T10:53:20Z

src/java.base/share/classes/java/lang/classfile/instruction/DiscontinuedInstruction.java

+     * where {@code slot} must be within {@code [0, 65535]}.
+     * <p>
+     * {@link StoreInstruction astore} series of instructions store a {@link
+     * TypeKind##returnAddress returnAddress} value to a local variable slot,


And the same here.

asotona · 2024-11-21T10:54:47Z

src/java.base/share/classes/java/lang/classfile/instruction/ConvertInstruction.java

+ * have a {@linkplain Opcode#kind() kind} of {@link Opcode.Kind#CONVERT}.
+ * Delivered as a {@link CodeElement} when traversing the elements of a {@link CodeModel}.
+ * <p>
+ * Conceptually, a primitive conversion instruction is a record:


Here remains "Conceptually, ... is a record"

asotona · 2024-11-21T10:56:23Z

src/java.base/share/classes/java/lang/classfile/instruction/InvokeInstruction.java

+ * Corresponding opcodes have a {@linkplain Opcode#kind() kind} of {@link Opcode.Kind#INVOKE}.
+ * Delivered as a {@link CodeElement} when traversing the elements of a {@link CodeModel}.
+ * <p>
+ * Conceptually, a method invocation instruction is a record:


asotona · 2024-11-21T11:13:48Z

src/java.base/share/classes/java/lang/classfile/instruction/StoreInstruction.java

-     * {@return the type of the value to be stored}
+     * {@return the {@linkplain TypeKind##computational-type computational type}
+     * of the value to be stored}  The {@link TypeKind#REFERENCE reference}
+     * type store instructions also operate on the {@code returnAddress} type,


Where do we expose a TypeKind related to a return address?

We do not. This just notes astore can store jsr addresses (see jvms). This is intended to be similar to the array load/store boolean note.

asotona · 2024-11-21T11:18:56Z

It looks better to me now, much easier to understand.

@jddarcy what do you think about general formulations "Xy instruction can be viewed as a record..." describing code elements exposed as sealed interfaces in the API?

…instruction

asotona

Looks good to me, thanks.

liach · 2024-12-10T20:32:33Z

Thanks for the reviews!

/integrate

openjdk · 2024-12-10T20:34:13Z

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

9bd70ec: 8345888: Broken links in the JDK 24 JavaDoc API documentation, build 27
1def2d8: 8345700: tier{1,2,3}_compiler don't cover all compiler tests
d6b5264: 8345746: Remove :resourcehogs/compiler from :hotspot_slow_compiler
4f855d1: 8345818: Fix SM cleanup of parsing of System property resource.bundle.debug
a24b08f: 8345669: RISC-V: fix client build failure due to AlignVector after JDK-8343827
25d9deb: 8335367: [s390] Add support for load immediate on condition instructions.
a6277bb: 8340212: -Xshare:off -XX:CompressedClassSpaceBaseAddress=0x40001000000 crashes on macos-aarch64
165fd84: 8342987: Update --release 24 symbol information for JDK 24 build 27
06c44dd: 8345465: Fix performance regression on x64 after JDK-8345120
2979806: 8345795: Update copyright year to 2024 for hotspot in files where it was missed
... and 89 more: https://git.openjdk.org/jdk/compare/79eb77b782bd0c3cecee6c66b86f6f3e17054498...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-12-10T20:34:22Z

@liach Pushed as commit 0f03554.

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

liach · 2024-12-10T20:40:12Z

/backport :jdk24

openjdk · 2024-12-10T20:41:31Z

@liach the backport was successfully created on the branch backport-liach-0f035545-jdk24 in my personal fork of openjdk/jdk. To create a pull request with this backport targeting openjdk/jdk:jdk24, just click the following link:

➡️ Create pull request

The title of the pull request is automatically filled in correctly and below you find a suggestion for the pull request body:

Hi all,

This pull request contains a backport of commit 0f035545 from the openjdk/jdk repository.

The commit being backported was authored by Chen Liang on 10 Dec 2024 and was reviewed by Adam Sotona and Joe Darcy.

Thanks!

If you need to update the source branch of the pull then run the following commands in a local clone of your personal fork of openjdk/jdk:

$ git fetch https://github.com/openjdk-bots/jdk.git backport-liach-0f035545-jdk24:backport-liach-0f035545-jdk24
$ git checkout backport-liach-0f035545-jdk24
# make changes
$ git add paths/to/changed/files
$ git commit --message 'Describe additional changes made'
$ git push https://github.com/openjdk-bots/jdk.git backport-liach-0f035545-jdk24

liach added 2 commits October 21, 2024 17:37

Wip instr

02d8412

Labels

a9c6c92

openjdk bot added the core-libs core-libs-dev@openjdk.org label Oct 21, 2024

Typos, long lines

96a4a3d

liach marked this pull request as ready for review October 22, 2024 04:25

openjdk bot added rfr Pull request is ready for review csr Pull request needs approved CSR before integration labels Oct 22, 2024

asotona approved these changes Oct 23, 2024

View reviewed changes

Merge branch 'master' of https://github.com/openjdk/jdk into docs/cf-…

e8334c9

…instruction

jddarcy reviewed Nov 15, 2024

View reviewed changes

jddarcy approved these changes Nov 15, 2024

View reviewed changes

liach added 2 commits November 17, 2024 19:58

Merge branch 'master' of https://github.com/openjdk/jdk into docs/cf-…

bc0fdda

…instruction

Further facelift

4a92d72

Non-int constant values

7ee26ee

liach mentioned this pull request Nov 18, 2024

8342468: Improve API documentation for java.lang.classfile.constantpool #22217

Closed

4 tasks

asotona reviewed Nov 19, 2024

View reviewed changes

liach added 3 commits November 20, 2024 14:26

Remove a few confusions around bad modeling, some missing exceptions,…

85f5ed0

… also the labelless info models

More cleanup

9f08019

typo

2dab7a2

asotona reviewed Nov 21, 2024

View reviewed changes

liach added 4 commits November 21, 2024 10:09

Leftover problems

56683d8

Merge branch 'master' of https://github.com/openjdk/jdk into docs/cf-…

ecda715

…instruction

Merge branch 'master' of https://github.com/openjdk/jdk into docs/cf-…

cda30b1

…instruction

Avoid using "record", other cleans

c151792

asotona approved these changes Dec 10, 2024

View reviewed changes

openjdk bot added ready Pull request is ready to be integrated and removed csr Pull request needs approved CSR before integration labels Dec 10, 2024

openjdk bot added the integrated Pull request has been integrated label Dec 10, 2024

openjdk bot closed this Dec 10, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Dec 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8342469: Improve API documentation for java.lang.classfile.instruction #21625

8342469: Improve API documentation for java.lang.classfile.instruction #21625

liach commented Oct 21, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Oct 21, 2024

openjdk bot commented Oct 21, 2024 •

edited

Loading

openjdk bot commented Oct 21, 2024

liach commented Oct 22, 2024

openjdk bot commented Oct 22, 2024

mlbridge bot commented Oct 22, 2024 •

edited

Loading

asotona left a comment

jddarcy Nov 15, 2024

liach commented Nov 18, 2024 •

edited

Loading

asotona Nov 19, 2024

liach Nov 20, 2024

asotona Nov 19, 2024

liach Nov 20, 2024

asotona commented Nov 19, 2024

liach commented Nov 20, 2024 •

edited

Loading

asotona Nov 21, 2024

liach Nov 21, 2024

asotona Nov 21, 2024

asotona Nov 21, 2024

asotona Nov 21, 2024

asotona Nov 21, 2024

liach Nov 21, 2024

asotona commented Nov 21, 2024

asotona left a comment

liach commented Dec 10, 2024

openjdk bot commented Dec 10, 2024

openjdk bot commented Dec 10, 2024

liach commented Dec 10, 2024

openjdk bot commented Dec 10, 2024

8342469: Improve API documentation for java.lang.classfile.instruction #21625

8342469: Improve API documentation for java.lang.classfile.instruction #21625

Conversation

liach commented Oct 21, 2024 • edited by openjdk bot Loading

Progress

Issues

Reviewers

Reviewing

bridgekeeper bot commented Oct 21, 2024

openjdk bot commented Oct 21, 2024 • edited Loading

openjdk bot commented Oct 21, 2024

liach commented Oct 22, 2024

openjdk bot commented Oct 22, 2024

mlbridge bot commented Oct 22, 2024 • edited Loading

Webrevs

asotona left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liach commented Nov 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asotona commented Nov 19, 2024

liach commented Nov 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asotona commented Nov 21, 2024

asotona left a comment

Choose a reason for hiding this comment

liach commented Dec 10, 2024

openjdk bot commented Dec 10, 2024

openjdk bot commented Dec 10, 2024

liach commented Dec 10, 2024

openjdk bot commented Dec 10, 2024

liach commented Oct 21, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Oct 21, 2024 •

edited

Loading

mlbridge bot commented Oct 22, 2024 •

edited

Loading

liach commented Nov 18, 2024 •

edited

Loading

liach commented Nov 20, 2024 •

edited

Loading