RFR: 8346989: Deoptimization and re-compilation cycle with C2 compiled code

[Emanuel Peter]

On Wed, 5 Mar 2025 12:56:48 GMT, Marc Chevalier <duke at openjdk.org> wrote:

> `Math.*Exact` intrinsics can cause many deopt when used repeatedly with problematic arguments.
> This fix proposes not to rely on intrinsics after `too_many_traps()` has been reached.
> 
> Benchmark show that this issue affects every Math.*Exact functions. And this fix improve them all.
> 
> tl;dr:
> - C1: no problem, no change
> - C2:
>   - with intrinsics:
>     - with overflow: clear improvement. Was way worse than C1, now is similar (~4s => ~600ms)
>     - without overflow: no problem, no change
>   - without intrinsics: no problem, no change
> 
> Before the fix:
> 
> Benchmark                                           (SIZE)  Mode  Cnt     Score      Error  Units
> MathExact.C1_1.loopAddIInBounds                    1000000  avgt    3     1.272 ±    0.048  ms/op
> MathExact.C1_1.loopAddIOverflow                    1000000  avgt    3   641.917 ±   58.238  ms/op
> MathExact.C1_1.loopAddLInBounds                    1000000  avgt    3     1.402 ±    0.842  ms/op
> MathExact.C1_1.loopAddLOverflow                    1000000  avgt    3   671.013 ±  229.425  ms/op
> MathExact.C1_1.loopDecrementIInBounds              1000000  avgt    3     3.722 ±   22.244  ms/op
> MathExact.C1_1.loopDecrementIOverflow              1000000  avgt    3   653.341 ±  279.003  ms/op
> MathExact.C1_1.loopDecrementLInBounds              1000000  avgt    3     2.525 ±    0.810  ms/op
> MathExact.C1_1.loopDecrementLOverflow              1000000  avgt    3   656.750 ±  141.792  ms/op
> MathExact.C1_1.loopIncrementIInBounds              1000000  avgt    3     4.621 ±   12.822  ms/op
> MathExact.C1_1.loopIncrementIOverflow              1000000  avgt    3   651.608 ±  274.396  ms/op
> MathExact.C1_1.loopIncrementLInBounds              1000000  avgt    3     2.576 ±    3.316  ms/op
> MathExact.C1_1.loopIncrementLOverflow              1000000  avgt    3   662.216 ±   71.879  ms/op
> MathExact.C1_1.loopMultiplyIInBounds               1000000  avgt    3     1.402 ±    0.587  ms/op
> MathExact.C1_1.loopMultiplyIOverflow               1000000  avgt    3   615.836 ±  252.137  ms/op
> MathExact.C1_1.loopMultiplyLInBounds               1000000  avgt    3     2.906 ±    5.718  ms/op
> MathExact.C1_1.loopMultiplyLOverflow               1000000  avgt    3   655.576 ±  147.432  ms/op
> MathExact.C1_1.loopNegateIInBounds                 1000000  avgt    3     2.023 ±    0.027  ms/op
> MathExact.C1_1.loopNegateIOverflow                 1000000  avgt    3   639.136 ±   30.841  ms/op
> MathExact.C1_1.loopNegateLInBounds                 1000000  avgt    3     2.422 ±    3.59...

The benchmark generally looks good to me, I only have some minor suggestions ;)

Ah. And is this only about `multiplyExact`, or are there other methods affected? Would be nice to extend the benchmark to those as well.

And yet another idea: you could probably write an IR test that checks that we at first have the compilation with the trap, and another test where we trap too much and then get a different compilation (without the intrinsic?).

Plus: the issue title is very generic. I think it should mention something about `Math.*Exact` as well ;)

test/micro/org/openjdk/bench/vm/compiler/MultiplyExact.java line 47:

> 45:         try {
> 46:             return square(i);
> 47:         } catch (Throwable e) {

Can you catch a more specific exception? Catching very general exceptions can often mask other bugs. I suppose this is only a benchmark, but it would still be good practice ;)

test/micro/org/openjdk/bench/vm/compiler/MultiplyExact.java line 62:

> 60: 
> 61:     @Fork(value = 1)
> 62:     public static class C2 extends MultiplyExact {}

What about a C2 version where you just disable the intrinsic?

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

PR Review: https://git.openjdk.org/jdk/pull/23916#pullrequestreview-2663529726
PR Comment: https://git.openjdk.org/jdk/pull/23916#issuecomment-2703023122
PR Review Comment: https://git.openjdk.org/jdk/pull/23916#discussion_r1982809388
PR Review Comment: https://git.openjdk.org/jdk/pull/23916#discussion_r1982808076