RFR: 8330157: C2: Add a stress flag for bailouts [v6]

[Emanuel Peter]

On Thu, 20 Jun 2024 16:04:39 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.
>
> Daniel Skantz has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - tweak probability in CtwRunner, add CaptureBailoutInformation
>  - remove once-used method arg

src/hotspot/share/opto/compile.cpp line 1024:

> 1022: #ifdef ASSERT
> 1023:   _phase_optimize_finished = false;
> 1024:   _phase_verify_ideal_loop = false;

What is this for?

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

> 839:   const CompilationFailureInfo* first_failure_details() const { return _first_failure_details; }
> 840: 
> 841:   bool failing(DEBUG_ONLY(bool no_stress_bailout=false)) {

Suggestion:

  bool failing(DEBUG_ONLY(bool no_stress_bailout = false)) {

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

> 857: #ifdef ASSERT
> 858:   bool fail_randomly() {
> 859:     if (!_stress_seed_is_initialized || (random() % StressBailoutProbability)) {

Suggestion:

    if (!_stress_seed_is_initialized || (random() % StressBailoutProbability) != 0) {

I think the new style is to never let values (ints, pointers, etc, only booleans are ok) be considered as conditions directly. So you would need a comparison.

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

> 878: 
> 879:   void record_failure(const char* reason DEBUG_ONLY(COMMA bool allow_multiple_failures=false));
> 880:   void record_method_not_compilable(const char* reason DEBUG_ONLY(COMMA bool allow_multiple_failures=false)) {

Suggestion:

  void record_failure(const char* reason DEBUG_ONLY(COMMA bool allow_multiple_failures = false));
  void record_method_not_compilable(const char* reason DEBUG_ONLY(COMMA bool allow_multiple_failures = false)) {

src/hotspot/share/opto/graphKit.hpp line 184:

> 182: 
> 183:   // Tell if the compilation is failing.
> 184:   bool failing(DEBUG_ONLY(bool no_stress_bailout=false)) const { return C->failing(DEBUG_ONLY(no_stress_bailout)); }

Suggestion:

  bool failing(DEBUG_ONLY(bool no_stress_bailout = false)) const { return C->failing(DEBUG_ONLY(no_stress_bailout)); }

test/hotspot/jtreg/compiler/debug/TestStressBailout.java line 39:

> 37:  * @summary Basic tests for bailout stress flag.
> 38:  * @library /test/lib /
> 39:  * @run driver compiler.debug.TestStressBailout

Are external flags passed through to your test-vm? Not sure what the impact of `driver` vs `main` is here, or if you would need to add the external flags explicitly when you create the child process.

test/hotspot/jtreg/testlibrary/ctw/src/sun/hotspot/tools/ctw/CtwRunner.java line 307:

> 305:                 "-XX:+StressIncrementalInlining",
> 306:                 "-XX:+StressBailout",
> 307:                 "-XX:StressBailoutProbability=100_000_000",

Hmm, does this very high mean-to-bailout imply that we rarely bailout? I guess it is a delicate line between bailing out too much and then all the compilations do not complete, and not bailing out enough, and not testing the bailout in combination with the stress flags enough. How do you know that this value is a good choice?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/19646#discussion_r1660536255
PR Review Comment: https://git.openjdk.org/jdk/pull/19646#discussion_r1660540368
PR Review Comment: https://git.openjdk.org/jdk/pull/19646#discussion_r1660542954
PR Review Comment: https://git.openjdk.org/jdk/pull/19646#discussion_r1660557104
PR Review Comment: https://git.openjdk.org/jdk/pull/19646#discussion_r1660557824
PR Review Comment: https://git.openjdk.org/jdk/pull/19646#discussion_r1660560735
PR Review Comment: https://git.openjdk.org/jdk/pull/19646#discussion_r1660565668