8374622: StressIncrementalInlining should also randomize the processing order

[marc-chevalier]

As it says: randomize the order the late inlines are processed, and slightly factor it with macro nodes.

I didn't add shuffle to the GrowableArray class since it seems a bit method specialized method (and it seems it'd be a controversial change to a widely use class), it would make GrowableArray depends on Compile for random number generation (or require a callback, for instance, giving a non-trivial signature and usage), I couldn't find other shuffling of such an object.
There is also shuffling of UniqueNodeList (for StressIGVN), but it seems hard to unify: access and length are not written quite the same, it would probably be not simpler than duplicating the implem (which is simple, so it's fine in my opinion).

Thanks,
Marc

---

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-8374622: StressIncrementalInlining should also randomize the processing order (Enhancement - P4)

Reviewers

• Tobias Hartmann (@TobiHartmann - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 29110

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back mchevalier! 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]

@marc-chevalier 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:

8374622: StressIncrementalInlining should also randomize the processing order

Reviewed-by: thartmann

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

• c5159fc: 8374328: Convert simple AtomicAccess uses in gc/shared to use Atomic
• 904ba5f: 8374718: Generation of CompilerProperties can fail in subtle ways
• 067fd3c: 8374768: S390X builds are failing after JDK-8372754
• ... and 13 more: https://git.openjdk.org/jdk/compare/929864b1a40eb222d3b7b3451fc6d4e5316a7cc8...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]

@marc-chevalier 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]

Webrevs

• 00: Full (38569db4)

[TobiHartmann]

Looks good to me. Please make sure that this doesn't immediately trigger bugs in our testing 🙂

[marc-chevalier]

Right, I didn't write there, but I've tested with tier 1-3 and internal tests. All good. I'll test with more flags, tho.

[TobiHartmann]

Sounds good, thanks!

[iwanowww]

It shuffles initial list, but doesn't have any effects on elements added during incremental inlining. Do we want to shuffle them as well?

[marc-chevalier]

Good point. I don't see why we wouldn't want that.

[marc-chevalier]

Actually, I'm not sure. I see that

jdk/src/hotspot/share/opto/compile.hpp

Line 480 in 8737a8c

int _late_inlines_pos; // Where in the queue should the next late inlining candidate go (emulate depth first inlining)

and

jdk/src/hotspot/share/opto/compile.hpp

Lines 1059 to 1062 in 8737a8c

	void add_late_inline(CallGenerator* cg) {
	_late_inlines.insert_before(_late_inlines_pos, cg);
	_late_inlines_pos++;
	}

But reading the code, I don't understand why it's important to insert just at this position, and not simply at this position or after (and then, why not at the end, to avoid shifting?). In inline_incrementally_one

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

Lines 2123 to 2126 in 8737a8c

	} else if (inlining_progress()) {
	_late_inlines_pos = i+1; // restore the position in case new elements were inserted
	print_method(PHASE_INCREMENTAL_INLINE_STEP, 3, cg->call_node());
	break; // process one call site at a time

We seem to stop as soon as something happens, so we wouldn't really use the fact that the coming elements in _late_inlines are related. Overall, I don't see where this assumption of depth-first is used.

A little bit of testing doesn't catch fire when inserting potentially after _late_inlines_pos. I'll read and test more, but maybe someone already has more context? Maybe @iwanowww or @rwestrel from looking at git blame?

[iwanowww]

During late inlining we want to preserve relative order or candidate sites for late inlining. It matters when compiler runs out of inlining budget. If it appends candidates instead, it turns DFS call site traversal into BFS.