[fandreuz]

This PR sorts the includes in hotspot/share/jfr using SortIncludes.java. I'm also adding the directory to TestIncludesAreSorted.

Testing:

• tier1
• tier2

---

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-8365610: Sort share/jfr includes (Enhancement - P4)

Reviewers

• Markus Grönlund (@mgronlun - Reviewer) Review applies to 3bfcf135
• Aleksey Shipilev (@shipilev - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 26800

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[bridgekeeper]

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

[fandreuz]

Let me know if the comment should stay

[openjdk]

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

8365610: Sort share/jfr includes

Reviewed-by: shade, mgronlun

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

• 4ed268f: 8362304: Fix JDWP spec w.r.t. OPAQUE_FRAME and INVALID_SLOT errors
• 0b2d081: 8365636: JFR: Minor cleanup

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@mgronlun, @shipilev) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

[openjdk]

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

• hotspot-jfr

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]

Webrevs

• 01: Full - Incremental (f734135f)
• 00: Full (3bfcf135)

[mgronlun]

This creates a dependency on a test that is not normally run when we develop JFR.

What tier is the test located?

[fandreuz]

This creates a dependency on a test that is not normally run when we develop JFR.

What tier is the test located?

TestIncludesAreSorted.java is in tier1.

[mgronlun]

A blanket HotSpot change that does not add much value, but instead creates an unnecessary dependency that will make development of JFR more problematic.

[shipilev]

A blanket HotSpot change that does not add much value, but instead creates an unnecessary dependency that will make development of JFR more problematic.

It is not "blanket", is it? The includes get sorted component by component, and many components in Hotspot have already been processed, with a long view of entire Hotspot being covered. Overall, while I agree maintaining the include order hygiene would require additional work, that is true for any hygiene work. It does not mean we refrain from it. What we do is to make sure the detection is as prompt and the fixes are as easy as practically possible.

Also, there is already a "dependency" on source tests. For example, if you write NULL anywhere in JFR code, this test would fail: https://github.com/openjdk/jdk/blob/master/test/hotspot/jtreg/sources/TestNoNULL.java. You'll get a failure with any tier1 testing, including GHA checks. The include order test is not that different.

[mgronlun]

That is a blanket change, yes - it means all of HotSpot being covered.

In my opinion, it adds little value and instead creates more work for us that develop JFR.

[mgronlun]

That is a blanket change, yes - it means all of HotSpot being covered.

It adds little value and instead creates more work for us who develop JFR.

But if "someone" has said this is good to do for all of HotSpot (who?), we can accommodate the general will.

[shipilev]

In my opinion, it adds little value and instead creates more work for us that develop JFR.

Noted, but I think this overestimates the amount of work. If the test would fail in tier1 (which the required pre-integration testing, runs even in GHA), in 99% of cases you either sort the includes by hand, or run the tool in the way that test failure itself would guide you through:

jdk/test/hotspot/jtreg/sources/TestIncludesAreSorted.java

Lines 91 to 93 in c1198bb

	This should be fixable by running:
	java %s.java --update %s

But if "someone" has said this is good to do for all of HotSpot (who?) we can accommodate to the general will.

Take a look at Hotspot Style Guide: "Keep the include lines within a section alphabetically sorted by their lowercase value. If an include must be out of order for correctness, suffix with it a comment such as // do not reorder. Source code processing tools can also use this hint."

Both the original JDK-8352645, and the related tasks from it maintain the same consensus: sort the includes, make sure the test would keep them sorted.

[mgronlun]

That the test is integrated with GHA is the good part.

[fandreuz]

/integrate

[openjdk]

@fandreuz
Your change (at version 3bfcf13) is now ready to be sponsored by a Committer.

[shipilev]

For Hotspot changes, you need to wait 24 hours, so people around the globe could take a look.

[fandreuz]

For Hotspot changes, you need to wait 24 hours, so people around the globe could take a look.

Sure, so is it 24 hours after getting approval? I thought it's 24 hours after opening the PR.

[shipilev]

For Hotspot changes, you need to wait 24 hours, so people around the globe could take a look.

Sure, so is it 24 hours after getting approval? I thought it's 24 hours after opening the PR.

Ah no, you're right: https://openjdk.org/guide/#life-of-a-pr -- 24 hours is the bare minimum. It gets a bit murky around holidays/weekends, as people take time off or checkout on Friday earlier. But this looks fine.

You also need a Review from one area expert (for JFR, Markus definitely qualifies), and another Review from any other reviewer. I'll do it shortly.

[shipilev]

Looks fine, but I think we should refrain from deleting the includes here.

[shipilev]

Looks fine, thanks.

[fandreuz]

Thanks for your reviews @shipilev and @mgronlun !

[fandreuz]

/integrate

[openjdk]

@fandreuz
Your change (at version f734135) is now ready to be sponsored by a Committer.

[simonis]

/sponsor

[openjdk]

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

• ed7d5fe: 8360304: Redundant condition in LibraryCallKit::inline_vector_nary_operation
• be6c15e: 8365671: Typo in Joiner.allUntil example
• 9041f4c: 8309400: JDI spec needs to clarify when OpaqueFrameException and NativeMethodException are thrown
• ... and 27 more: https://git.openjdk.org/jdk/compare/999761d0f6d37c9cd6ec482620800b694c5fb9ad...master

Your commit was automatically rebased without conflicts.

[openjdk]

@simonis @fandreuz Pushed as commit ecab52c.

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