RFR: 8335334: Stress mode to randomly execute unstable if traps

[Vladimir Kozlov]

On Tue, 17 Sep 2024 11:01:27 GMT, Tobias Hartmann <thartmann at openjdk.org> wrote:

> Unstable if traps are supposed to be taken rarely. This patch introduces a `StressUnstableIfTraps` flag that forces unstable if traps to be taken randomly and thus potentially triggering intermittent bugs such as incorrect debug information. It works by adding another if before the unstable if that checks a "random" condition at runtime (a simple shared counter) and then either takes the trap or executes the original, unstable if.
> 
> This stress option also has the nice side effect of triggering re-compilation of methods that would otherwise not be re-compiled (see [JDK-8335843](https://bugs.openjdk.org/browse/JDK-8335843)).
> 
> I had to adjust a few tests that rely on methods being compiled / deoptimized because with the stress option enabled, deoptimization might unexpectedly (not) happen.
> 
> It reliably triggers [JDK-8335977](https://bugs.openjdk.org/browse/JDK-8335977), [JDK-8320308](https://bugs.openjdk.org/browse/JDK-8320308) and [JDK-8335843](https://bugs.openjdk.org/browse/JDK-8335843) in our testing.
> 
> Tested with multiple runs up to tier6. I'll integrate it into our (Oracle internal) testing and CTW [(JDK-8340302)](https://bugs.openjdk.org/browse/JDK-8340302) once all the bugs that it currently triggers are fixed.
> 
> Thanks,
> Tobias

Few comments.

src/hotspot/share/opto/parse2.cpp line 1385:

> 1383:   bool do_stress_trap = StressUnstableIfTraps && ((C->random() % 2) == 0);
> 1384:   if (do_stress_trap) {
> 1385:     Node* counter_addr = makecon(TypeRawPtr::make((address)&_trap_stress_counter));

Would it be easier if you use new Ideal macro node for this and expand it in macro expansion phase?

src/hotspot/share/opto/parse2.cpp line 1497:

> 1495:     incr_store = store_to_memory(control(), counter_addr, counter, T_INT, Compile::AliasIdxRaw, MemNode::unordered);
> 1496:   }
> 1497: 

>From the glance it looks like the code above. Should you put it into separate method to call it in both places?

src/hotspot/share/opto/parse2.cpp line 1589:

> 1587:   // Search for an unstable if trap
> 1588:   CallStaticJavaNode* trap = nullptr;
> 1589:   for (int i = 0; i <= 1; ++i) {

Should we check that it is `IfNode` and it has 2 output edges? May be assert?

src/hotspot/share/opto/parse2.cpp line 1590:

> 1588:   CallStaticJavaNode* trap = nullptr;
> 1589:   for (int i = 0; i <= 1; ++i) {
> 1590:     Node* out = orig_iff->raw_out(i)->find_out_with(Op_CallStaticJava);

Why not cast (CallStaticJava*) here?

src/hotspot/share/opto/parse2.cpp line 1591:

> 1589:   for (int i = 0; i <= 1; ++i) {
> 1590:     Node* out = orig_iff->raw_out(i)->find_out_with(Op_CallStaticJava);
> 1591:     if (out != nullptr && out->isa_CallStaticJava() && out->as_CallStaticJava()->is_uncommon_trap()) {

You don't need `out->isa_CallStaticJava()` because `find_out_with()` will return `nullptr` in other cases.

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

PR Review: https://git.openjdk.org/jdk/pull/21037#pullrequestreview-2310269564
PR Review Comment: https://git.openjdk.org/jdk/pull/21037#discussion_r1763603794
PR Review Comment: https://git.openjdk.org/jdk/pull/21037#discussion_r1763554614
PR Review Comment: https://git.openjdk.org/jdk/pull/21037#discussion_r1763612378
PR Review Comment: https://git.openjdk.org/jdk/pull/21037#discussion_r1763616657
PR Review Comment: https://git.openjdk.org/jdk/pull/21037#discussion_r1763614982