8254108: ciReplay: Support incremental inlining #6413

chhagedorn · 2021-11-16T15:45:15Z

This patch adds support to explicitly apply incremental inlining when replay compiling a method if the original compilation of the method was also incrementally inlined. We write a new value when dumping the inline tree to indicate if an inlinee was incrementally inlined (= 1) or not (= 0).

To implement this, I updated the REPLAY_VERSION to 2 and additionally added a test to verify that old replay file versions are still working. I added some support to modify/remove version numbers of generated replay files in tests. I also refactored the test added by JDK-8275868 to reuse some of the methods.

Thanks,
Christian

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

JDK-8254108: ciReplay: Support incremental inlining

Reviewers

Dean Long (@dean-long - Reviewer)
Tobias Hartmann (@TobiHartmann - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/6413/head:pull/6413
$ git checkout pull/6413

Update a local copy of the PR:
$ git checkout pull/6413
$ git pull https://git.openjdk.java.net/jdk pull/6413/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 6413

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/6413.diff

bridgekeeper · 2021-11-16T15:45:43Z

👋 Welcome back chagedorn! 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 · 2021-11-16T15:48:04Z

@chhagedorn 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 · 2021-11-16T15:51:57Z

Webrevs

dean-long · 2021-11-16T22:06:12Z

src/hotspot/share/ci/ciReplay.cpp

+            if (had_error()) {
+              // Pending exception?
+              break;
+            }


I don't see how a pending exception is possible here, given the check at L763, and parse_int() doesn't throw any.
What do you think about not calling parse_int() if _version < 2, that way there is no error to ignore?

I think that's a better idea to not parse it at all now that we have version numbers available.

dean-long · 2021-11-16T22:30:59Z

src/hotspot/share/opto/bytecodeInfo.cpp

-    build_inline_tree_for_callee(callee_method, jvms, caller_bci);
+    InlineTree* callee_tree = build_inline_tree_for_callee(callee_method, jvms, caller_bci);
+    if (should_delay || AlwaysIncrementalInline) {
+      callee_tree->set_late_inline();


It took me a while to figure out why this is needed: for replay. It bothers me a little that AlwaysIncrementalInline is check here and again in the caller. If the replay file sets should_delay to false, then we shouldn't let AlwaysIncrementalInline to force it to true, right? So I'm wondering if it would be better to pre-set should_delay to true in the caller if AlwaysIncrementalInline is true.

I've added a comment to make it more clear. So, this code is only to record the late inlining decision to later dump it to the replay file. I think initializing should_delay = AlwaysIncrementalInline is a good idea. should_delay can only become true but not false anymore during normal compilation.

But I think we need to leave || AlwaysIncrementalInline in here https://github.com/openjdk/jdk/blob/master/src/hotspot/share/opto/doCall.cpp#L192 in case someone wants to replay compile with that flag even though the replay file recorded a different late inlining decision.

I also fixed the test if run with -XX:+AlwaysIncrementalInline.

Shouldn't the recorded inlining decision always override flags like -XX:+AlwaysIncrementalInline?
This brings up the question of how to handle flags. If we stored them in the replay file, then the replay compile could compare those to the current flags and if they don't match:

give a warning and continue, correct replay not guaranteed

give an error and refuse to continue

override current flags with saved flags (this could be implemented by having the "ci" layer cache flags settings for each compile)

As we have discussed offline, it's best not to treat AlwaysIncrementalInline specially given that we are already enforcing the general inlining decisions based on the replay data. I will therefore remove || AlwaysIncrementalInline from L192.

About the flag handling in general, as you have suggested offline, I also think it's a good option to put the used flags in the replay file in the future and enforce them. If the user wants to run a different set of flags for any reasons then the replay file can be adapted accordingly.

dean-long · 2021-11-16T22:31:38Z

src/hotspot/share/opto/bytecodeInfo.cpp

    return false;
  }
-  if (should_not_inline(callee_method, caller_method, caller_bci, profile)) {
+  if (should_not_inline(callee_method, caller_method, caller_bci, NOT_PRODUCT_ARG(should_delay) profile)) {


How about a comment for these two calls saying replay may override "should_delay"?

…n with -XX:+AlwaysIncrementalInline

dean-long · 2021-11-17T20:41:10Z

src/hotspot/share/opto/bytecodeInfo.cpp

-    build_inline_tree_for_callee(callee_method, jvms, caller_bci);
+    InlineTree* callee_tree = build_inline_tree_for_callee(callee_method, jvms, caller_bci);
+    if (should_delay || AlwaysIncrementalInline) {
+      callee_tree->set_late_inline();


Shouldn't the recorded inlining decision always override flags like -XX:+AlwaysIncrementalInline?
This brings up the question of how to handle flags. If we stored them in the replay file, then the replay compile could compare those to the current flags and if they don't match:

give a warning and continue, correct replay not guaranteed

give an error and refuse to continue

override current flags with saved flags (this could be implemented by having the "ci" layer cache flags settings for each compile)

dean-long · 2021-11-18T08:56:45Z

src/hotspot/share/opto/bytecodeInfo.cpp

                              bool& should_delay) {
  assert(callee_method != NULL, "caller checks for optimized virtual!");
-  assert(!should_delay, "should be initialized to false");
+  assert(!should_delay || AlwaysIncrementalInline, "should be initialized to false");


I'm not sure how useful this assert is now. It could be changed to should_delay == AlwaysIncrementalInline, or maybe just removed?

Agreed, removed.

openjdk · 2021-11-18T20:21:41Z

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

8254108: ciReplay: Support incremental inlining

Reviewed-by: dlong, 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 88 new commits pushed to the master branch:

d427c79: 8277428: G1: Move and inline G1STWIsAliveClosure::do_object_b
32839ba: 8266593: vmTestbase/nsk/jvmti/PopFrame/popframe011 fails with "assert(java_thread == _state->get_thread()) failed: Must be"
8051041: 8277534: Remove unused ReferenceProcessor::has_discovered_references
3f847fe: 8277385: Zero: Enable CompactStrings support
ca31ed5: 8275448: [REDO] AArch64: Implement string_compare intrinsic in SVE
4ff4301: 8224922: Access JavaFileObject from Element(s)
0a9e76c: 8277485: Zero: Fix fast{i,f}access_0 bytecodes handling
1c215f3: 8272773: Configurable card table card size
1d7cef3: 8276662: Scalability bottleneck in SymbolTable::lookup_common()
c79a485: 8277494: [BACKOUT] JDK-8276150 Quarantined jpackage apps are labeled as "damaged"
... and 78 more: https://git.openjdk.java.net/jdk/compare/b8d33a2a4e4ac1be322644102e8f09ce1435b4fb...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.

chhagedorn · 2021-11-19T08:20:55Z

Thanks Dean for the careful review!

TobiHartmann

The changes look good to me. Nice test!

chhagedorn · 2021-11-22T15:59:22Z

Thanks Tobias for your review!

chhagedorn · 2021-11-23T15:20:33Z

/integrate

openjdk · 2021-11-23T15:22:25Z

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

64bdc84: 8277649: [BACKOUT] JDK-8277507 Add jlink.debug system property while launching jpackage tests to help diagonize recent intermittent failures
5e7e2da: 8272042: java.util.ImmutableCollections$Map1 and MapN should not be @valuebased
f4dc03e: 8276696: ParallelObjectIterator freed at the wrong time in VM_HeapDumper
90f96fb: 8277441: CompileQueue::add fails with assert(_last->next() == __null) failed: not last
66eaf65: 8277413: Remove unused local variables in jdk.hotspot.agent
36b887a: 8277560: Remove WorkerDataArray::_is_serial
017df14: 8277556: Call ReferenceProcessorPhaseTimes::set_processing_is_mt once
79350b9: 8276216: Negated character classes performance regression in Pattern
e3243ee: 8277087: ZipException: zip END header not found at ZipFile#Source.findEND
12f08ba: 8277507: Add jlink.debug system property while launching jpackage tests to help diagonize recent intermittent failures
... and 100 more: https://git.openjdk.java.net/jdk/compare/b8d33a2a4e4ac1be322644102e8f09ce1435b4fb...master

Your commit was automatically rebased without conflicts.

openjdk · 2021-11-23T15:22:44Z

@chhagedorn Pushed as commit 38802ad.

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

chhagedorn added 2 commits November 16, 2021 11:45

8254108: ciReplay: Support incremental inlining

fc88420

adapt patch to use new replay file version number

ea35d31

openjdk bot added the rfr Pull request is ready for review label Nov 16, 2021

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Nov 16, 2021

dean-long suggested changes Nov 16, 2021

View reviewed changes

update comments, should_delay and parsing inline_late, fix test if ru…

0f6b204

…n with -XX:+AlwaysIncrementalInline

dean-long suggested changes Nov 17, 2021

View reviewed changes

dean-long reviewed Nov 18, 2021

View reviewed changes

update AlwaysIncrementalInline and assert

ab63bcb

dean-long approved these changes Nov 18, 2021

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Nov 18, 2021

TobiHartmann approved these changes Nov 22, 2021

View reviewed changes

openjdk bot closed this Nov 23, 2021

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Nov 23, 2021

8254108: ciReplay: Support incremental inlining #6413

8254108: ciReplay: Support incremental inlining #6413

Uh oh!

Conversation

chhagedorn commented Nov 16, 2021 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Nov 16, 2021

Uh oh!

openjdk bot commented Nov 16, 2021

Uh oh!

mlbridge bot commented Nov 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Nov 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chhagedorn commented Nov 19, 2021

Uh oh!

TobiHartmann left a comment

Choose a reason for hiding this comment

Uh oh!

chhagedorn commented Nov 22, 2021

Uh oh!

chhagedorn commented Nov 23, 2021

Uh oh!

openjdk bot commented Nov 23, 2021

Uh oh!

openjdk bot commented Nov 23, 2021

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

chhagedorn commented Nov 16, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented Nov 16, 2021 •

edited

Loading

openjdk bot commented Nov 18, 2021 •

edited

Loading