On Tue, 19 Aug 2025 10:25:34 GMT, Aleksey Shipilev <shade@openjdk.org> wrote:

...

src/hotspot/share/jfr/periodic/jfrOSInterface.cpp line 35:

...

33: #include "utilities/ostream.hpp" 34: 35: #include <stdlib.h> // for environment variables

I couldn't find any usage for this include, let me know if it should stay. It's been here for a long time: https://github.com/openjdk/jdk/commit/a060be188df894ed5c26fc12fc9e902f9af32b...

I suspect it has to do with `generate_environment_variables_events()` here: https://github.com/openjdk/jdk/pull/26800/files#diff-2d77a162387fd8e8a57c631...

Which does not have any implementation. I cannot find any code history about this as well; looks like it was unimplemented from day 1:

% git log -p --follow src/hotspot/ | grep generate_environment_variables_events + void generate_environment_variables_events();

So maybe keep this one intact, and submit another quick RFE to clean up both the header inclusion and the dead method declarations? It would also match the synopsis for current PR ("sort") better.

Sure: f734135f2ecc399ef2aa2a2d150ebba454e5776d https://bugs.openjdk.org/browse/JDK-8365782 ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/26800#discussion_r2284870134

On Mon, 18 Aug 2025 09:56:32 GMT, Markus Grönlund <mgronlun@openjdk.org> wrote:

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`](https://github.com/openjdk/jdk/blob/master/test/hotspot/jtreg/TEST.groups#L1...). ------------- PR Comment: https://git.openjdk.org/jdk/pull/26800#issuecomment-3196013395

On Mon, 18 Aug 2025 10:43:38 GMT, Markus Grönlund <mgronlun@openjdk.org> wrote:

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/TestNo.... You'll get a failure with any `tier1` testing, including GHA checks. The include order test is not that different. ------------- PR Comment: https://git.openjdk.org/jdk/pull/26800#issuecomment-3196274329

On Mon, 18 Aug 2025 11:44:38 GMT, Markus Grönlund <mgronlun@openjdk.org> wrote:

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.

But if "someone" has said this is good to do for all of HotSpot (who?) we can accommodate to the general will. ------------- PR Comment: https://git.openjdk.org/jdk/pull/26800#issuecomment-3196327634

On Mon, 18 Aug 2025 15:46:09 GMT, Aleksey Shipilev <shade@openjdk.org> wrote:

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. ------------- PR Comment: https://git.openjdk.org/jdk/pull/26800#issuecomment-3197491190

On Tue, 19 Aug 2025 10:10:56 GMT, Aleksey Shipilev <shade@openjdk.org> wrote:

...

...

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.

...

...

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.

Thanks for your reviews @shipilev and @mgronlun ! ------------- PR Comment: https://git.openjdk.org/jdk/pull/26800#issuecomment-3201175425

On Tue, 19 Aug 2025 10:53:25 GMT, Francesco Andreuzzi <duke@openjdk.org> wrote:

...

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

Testing: - [x] tier1 - [x] tier2

Francesco Andreuzzi has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision:

- revert - Merge branch 'master' into JDK-8365610 - reorder

@fandreuz Your change (at version f734135f2ecc399ef2aa2a2d150ebba454e5776d) is now ready to be sponsored by a Committer. ------------- PR Comment: https://git.openjdk.org/jdk/pull/26800#issuecomment-3201182213