RFR: 8330157: C2: Add a stress flag for bailouts

[Thomas Stuefe]

On Tue, 11 Jun 2024 07:14:20 GMT, Daniel Skantz <duke at openjdk.org> wrote:

> This patch adds a diagnostic/stress flag for C2 bailouts. It can be used to support testing of existing bailouts to prevent issues like [JDK-8318445](https://bugs.openjdk.org/browse/JDK-8318445), and can test for issues only seen at runtime such as [JDK-8326376](https://bugs.openjdk.org/browse/JDK-8326376). It can also be useful if we want to add more bailouts ([JDK-8318900](https://bugs.openjdk.org/browse/JDK-8318900)).
> 
> We check two invariants.
> a) Bailouts should be successful starting from any given `failing()` check.
> b) The VM should not record a bailout when one is pending (in which case we have continued to optimize for too long).
> 
> a), b) are checked by randomly starting a bailout at calls to `failing()` with a user-given probability.
> 
> The added flag should not have any effect in debug mode.
> 
> Testing:
> 
> T1-5, with flag and without it. We want to check that this does not cause any test failures without the flag set, and no unexpected failures with it. Tests failing because of timeout or because an error is printed to output when compilation fails can be expected in some cases.

This looks useful.

Is it planned to also test bailouts for C1 with the same switch?

src/hotspot/share/opto/c2_globals.hpp line 76:

> 74:           "Stress bailout every n:th time on average")                      \
> 75:           range(1, max_juint)                                               \
> 76:                                                                             \

"Interval" is usually a time period. Maybe `StressBailoutProbability` ?

src/hotspot/share/opto/compile.hpp line 831:

> 829: 
> 830:   const CompilationFailureInfo* first_failure_details() const { return _first_failure_details; }
> 831: 

- Why guarantees? Why not assert? Do we really want this stress code in release builds? 
- If assert, the `skip` parameter can be DEBUG_ONLY. 
- In any case, I would rename it since is not very clear without examining the source code now. We don't skip the error check, we override the random stress bailout check. E.g. something like `DEBUG_ONLY(bool no_stress_bailout)` ?

src/hotspot/share/opto/compile.hpp line 839:

> 837:       return false;
> 838:     }
> 839:     return fail_randomly(StressBailoutInterval);

To make this easier to understand, I'd invert the logic to


if (StressBailout && !skip) {
  return fail_randomly
}

src/hotspot/share/opto/compile.hpp line 843:

> 841: 
> 842:   bool fail_randomly(uint invprob) {
> 843:     guarantee(0 < invprob, "domain error");

- invprob depends on StressBailoutInterval. It never is anything different. Why not hard-code it right here for simplicity?
- Does this really need guarantee? Maybe assert, or potentially remove completely? `StressBailoutInterval` is a range-checked option >=1, and if range-checks don't work we notice. Also, since we use it for division, we would get a clear error immediately.

src/hotspot/share/opto/compile.hpp line 847:

> 845:     return false; // debug builds assert on bailouts.
> 846: #endif
> 847:     if (!_stress_seed || (random() % invprob)) {

- nit, unless comparing boolean expressions, we use explicit comparisons, not !
- but the _stress_seed != 0 exclusion is somewhat odd. We allow the seed to be 0 - since `-XX:StressSeed=0` is valid. So, either we should make sure `StressSeed` can never be 0 by restricting its valid range to >=1. Then, here (or better, inside Compile::random()) we should assert that seed != 0. Alternatively, if StressSeed=0 is supposed to be valid, this exclusion here can be removed. Note that it does not get mirrored by other uses of `Compile::random`.

src/hotspot/share/opto/compile.hpp line 853:

> 851:     return true;
> 852:   }
> 853: 

Does this function really have to be inline? We only exercise it for StressBailout=true

-------------

PR Review: https://git.openjdk.org/jdk/pull/19646#pullrequestreview-2109947885
PR Review Comment: https://git.openjdk.org/jdk/pull/19646#discussion_r1634577626
PR Review Comment: https://git.openjdk.org/jdk/pull/19646#discussion_r1634562062
PR Review Comment: https://git.openjdk.org/jdk/pull/19646#discussion_r1634578749
PR Review Comment: https://git.openjdk.org/jdk/pull/19646#discussion_r1634570032
PR Review Comment: https://git.openjdk.org/jdk/pull/19646#discussion_r1634583753
PR Review Comment: https://git.openjdk.org/jdk/pull/19646#discussion_r1634574933