RFR: 8316180: Thread-local backoff for secondary_super_cache updates [v13]
Luis Barreiro
duke at openjdk.org
Wed Nov 15 13:58:41 UTC 2023
On Thu, 2 Nov 2023 18:13:38 GMT, Aleksey Shipilev <shade at openjdk.org> wrote:
>> See more details in the bug and related issues.
>>
>> This is the attempt to mitigate [JDK-8180450](https://bugs.openjdk.org/browse/JDK-8180450), while the more complex fix that would obviate the need for `secondary_super_cache` is being worked out. The goal for this fix is to improve performance in pathological cases, while keeping non-pathological cases out of extra risk, *and* staying simple enough and reliable for backports to currently supported JDK releases.
>>
>> This implements mitigation on most current architectures:
>> - ✅ x86_64: implemented
>> - 🔴 x86_32: considered, abandoned; cannot be easily done without blowing up code size
>> - ✅ AArch64: implemented
>> - 🔴 ARM32: considered, abandoned; needs cleanups and testing; see [JDK-8318414](https://bugs.openjdk.org/browse/JDK-8318414)
>> - ✅ PPC64: implemented, thanks @TheRealMDoerr
>> - ✅ S390: implemented, thanks @offamitkumar
>> - ✅ RISC-V: implemented, thanks @RealFYang
>> - ✅ Zero: does not need implementation
>>
>> Note that the code is supposed to be rather compact, because it is inlined in generated code. That is why, for example, we cannot easily do x86_32 version: we need a thread, so the easiest way would be to call into VM. But we cannot that easily: the code blowout would make some forward branches in external code non-short. I think we we cannot implement this mitigation on some architectures, so be it, it would be a sensible tradeoff for simplicity.
>>
>> Setting backoff at `0` effectively disables the mitigation, and gives us safety hatch if something goes wrong.
>>
>> I believe we can go in with `1000` as the default, given the experimental results mentioned in this PR.
>>
>> Additional testing:
>> - [x] Linux x86_64 fastdebug, `tier1 tier2 tier3`
>> - [x] Linux AArch64 fastdebug, `tier1 tier2 tier3`
>
> Aleksey Shipilev has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 29 additional commits since the last revision:
>
> - Merge branch 'master' into JDK-8316180-backoff-secondary-super
> - Improve benchmarks
> - Merge branch 'master' into JDK-8316180-backoff-secondary-super
> - Editorial cleanups
> - RISC-V implementation
> - Mention ARM32 bug
> - Make sure benchmark runs with C1
> - Merge branch 'master' into JDK-8316180-backoff-secondary-super
> - Touchup benchmark metadata
> - S390 implementation
> - ... and 19 more: https://git.openjdk.org/jdk/compare/fcf8687c...74921ea9
I have tested this patch in isolation, by running the techempower benchmark on a quarkus version that is heavily impacted by the type pollution issue (`JDK-8316180`). See [1] for the exact commit used for the benchmark.
The procedure has been to back-port the patch to the latest JDK 21 build (`jdk-21+35`) (that is the branch in [2]). 6 values for the backoff (-XX:SecondarySuperMissBackoff property) were used: `0`, `1`, `10`, `100`, `1000`, `10000`.
The results for the `plaintext` test can be summarized in the graphs below:
None of the test of the benchmark showed any regression.
When comparing with JDK 22 (`jdk-22+21`), this patch can still offer a significant improvement.
The improved performance of JDK22 can be attributed, but not limited, to `JDK-8308869` that improves C2 type awareness and that is effective for this use case. Other optimizations that have since been merged can impact this comparison between results as well.
[1] - https://github.com/TechEmpower/FrameworkBenchmarks/tree/0db323061e4e258d1ce66a34ea2132f8beef5cc8
[2] - https://github.com/barreiro/jdk/tree/shipilev_patch
-------------
PR Comment: https://git.openjdk.org/jdk/pull/15718#issuecomment-1812570392
More information about the hotspot-dev
mailing list