8322636: [JVMCI] HotSpotSpeculationLog can be inconsistent across a single compile

[davleopo]

This PR fixes a subtle inconsistency in HotSpotSpeculationLog .

Normal uses of HotSpotSpeculationLog work by using a SpeculationReason and asking the speculation log via maySpeculate if the speculation can be performed, i.e., if it failed before for the given method. An example for this can be seen in Graal https://github.com/oracle/graal/blob/master/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/nodes/loop/CountedLoopInfo.java#L591C15-L591C15
The implicit assumption is that the speculation log, HotSpotSpeculationLog in particular collects failed speculations at the beginning of a compile and then stays consistent during the compile. Why is that? - Because if there are new failed speculations added to the failed speculations during the compile - the compiler would speculate again on those in an inconsistent way. E.g. at the beginning of a compile a certain speculation has not failed yet and the compiler thinks it can do optimization xyz using a speculation - later during the compilation process it consults the speculation log but gets a different answer. All those inconsistent speculations that already failed will anyway later fail code installation in jvmci (they will throw a bailout during HotSpotCodeCacheProvider#installCode https://github.com/openjdk/jdk/blob/master/src/jdk.internal.vm.ci/share/classes/jdk/vm/ci/hotspot/HotSpotSpeculationLog.java#L192 ). Thus, we should at least return a consistent result during a compile.
The problem for consistency here, that also makes troubles on the graal side, is that maySpeculate itself can collect failed speculations if there have not been any previously, i.e., failedSpeculations == null.
In order to make the speculation log consistent across an entire JVMCI compile this PR removes the collection of failed speculations in maySpeculate.

---

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-8322636: [JVMCI] HotSpotSpeculationLog can be inconsistent across a single compile (Bug - P4)

Reviewers

• Doug Simon (@dougxc - Reviewer) ⚠️ Review applies to 810e42ad
• Tom Rodriguez (@tkrodriguez - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 17183

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

Using diff file

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

Webrev

Link to Webrev Comment

[bridgekeeper]

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

@davleopo The following labels will be automatically applied to this pull request:

• graal
• hotspot-compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

[davleopo]

cc @dougxc @tkrodriguez can you have a look

[mlbridge]

Webrevs

• 01: Full - Incremental (ef026267)
• 00: Full (810e42ad)

[dougxc]

I think it's worth updating the javadoc for maySpeculate to clarify that it returns consistent results for any given speculation for the lifetime of a SpeculationLog object.

[openjdk]

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

8322636: [JVMCI] HotSpotSpeculationLog can be inconsistent across a single compile

Reviewed-by: dnsimon, never

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

• 2a9c358: 8322725: (tz) Update Timezone Data to 2023d
• 5235cc9: 8322583: RISC-V: Enable fast class initialization checks
• 3b1e56a: 8322322: Support archived full module graph when -Xbootclasspath/a is used
• 3fbccb0: 8322978: Remove debug agent debugMonitorTimedWait() function. It is no longer used.
• ad31ec5: 8322647: Short name for the Europe/Lisbon time zone is incorrect
• 15cf8f8: 8319626: Override toString() for ZipFile
• ade4074: 8322976: Remove reference to transform_no_reclaim
• ea19e9c: 8323011: ProblemList serviceability/HeapDump/FullGCHeapDumpLimitTest.java
• d33dfe5: 8323002: test/jdk/java/lang/Thread/virtual/stress/GetStackTraceALotWhenPinned.java times out on macosx-x64
• 27d5f5c: 8322781: C1: Debug build crash in GraphBuilder::vmap() when print stats
• ... and 57 more: https://git.openjdk.org/jdk/compare/84c23792856c5c2374963d78a7a734a467bbb79b...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.

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 (@dougxc, @tkrodriguez) 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).

[davleopo]

I think it's worth updating the javadoc for maySpeculate to clarify that it returns consistent results for any given speculation for the lifetime of a SpeculationLog object.

@dougxc done - please check

[tkrodriguez]

More specifically it only validates against the speculations that failed before the last call to collectFailedSpeculations which must always be called explicitly. And we should point out somewhere that installCode will call collectFailedSpeculations before installation and revalidate the current set of speculations, bailing out if any were violated during compilation. This doesn't seem to be documented anywhere.

[davleopo]

@tkrodriguez where do you want to put it?

Id suggest to add some additional javadoc to maySpeculate so we end up with something like

    /**
     * @return {@code true} if the given speculation can be performed, i.e., it never failed so far, otherwise
     * return {@code false}. Note, that this method returns consistent results for any given speculation for the
     * entire lifetime of the enclosing SpeculationLog object. This means that speculations failed during a
     * compilation will not be updated. Validation of speculations only considers those failed since the last
     * call to {@link #collectFailedSpeculations()}. 
     * 
     * Users of {@link SpeculationLog} must explicitly call {@link #collectFailedSpeculations()} to collect
     * failed speculations. This should be done before starting a compile.
     * 
     * Code installation performs a revalidation of the current set of speculations. If this fails, i.e. since the
     * start of the compile new speculations failed, the compilation is aborted with a bailout. This is done in
     * {@link #getFlattenedSpeculations(boolean)}.
     */

[tkrodriguez]

So I looked more closely the HotSpot and substrate implementations and I'm not sure we can currently align the implementation and the javadoc. In the HotSpot world, HotSpotSpeculationLog is a compiler local object that reads data from the real speculation data that's kept in the MDO. This means that it has full control over when collectFailedSpeculations is called. SubstrateSpeculationLog is the actual log so if two threads are operating on the same log then one of them could see the effects of a call to collectFailedSpeculations by the other thread. Maybe in practice 2 threads never do this because it would mean they are compiling the same root method but it doesn't seem guaranteed. installCode on substrate also doesn't perform the speculation log check that HotSpot does. So maybe we punt on javadoc updates for now.

[davleopo]

I did not consider the substrate runtime compilation use case - that may actually lead to the same error of inconsistency we have seen as here. Probably not relevant now but if it ever pops up we need to relax the invariant on the graal side then.

Regarding doc changes - what is our final call now ? (a) drop all new doc again or (b) keep (whatever) form of the new doc I added? - Its hotspot specific so not wrong.

[tkrodriguez]

I think for now lets just stick with your updates. It does seem like the substrate runtime compilation case is potentially exposed to the original problem but we should address that separately.

[davleopo]

/integrate

[openjdk]

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

[davleopo]

@tkrodriguez please sponsor

[tkrodriguez]

/sponsor

[openjdk]

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

• 46965a0: 8322981: Fix 2 locations in JDI that throw IOException without using the "Caused by" exception
• 700c25f: 8322954: Shenandoah: Convert evac-update closures asserts to rich asserts
• 631a9f6: 8323073: ProblemList gc/g1/TestSkipRebuildRemsetPhase.java on linux-aarch64
• ed9f324: 8322985: [BACKOUT] 8318562: Computational test more than 2x slower when AVX instructions are used
• ade21a9: 8310844: [AArch64] C1 compilation fails because monitor offset in OSR buffer is too large for immediate
• f0cfd36: 8322532: JShell : Unnamed variable issue
• 78623c9: 8323012: C2 fails with fatal error: no reachable node should have no use
• f0e2e43: 8323021: Shenandoah: Encountered reference count always attributed to first worker thread
• 3dc4bd8: 8322989: New test serviceability/HeapDump/FullGCHeapDumpLimitTest.java fails
• 1d1cd32: 8321812: Update GC tests to use execute[Limited]TestJava
• ... and 68 more: https://git.openjdk.org/jdk/compare/84c23792856c5c2374963d78a7a734a467bbb79b...master

Your commit was automatically rebased without conflicts.

[openjdk]

@tkrodriguez @davleopo Pushed as commit 35a1b77.

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