RFR: 8352316: More MergeStoreBench [v7]

[Emanuel Peter]

On Sat, 29 Mar 2025 07:27:24 GMT, Shaojin Wen <swen at openjdk.org> wrote:

>> Added performance tests related to String.getBytes/String.getChars/StringBuilder.append/System.arraycopy in constant scenarios to verify whether MergeStore works
>
> Shaojin Wen has updated the pull request incrementally with one additional commit since the last revision:
> 
>   add StringBuilderUnsafePut

@wenshao @iwanowww  I have a few concerns about this PR.

Your current PR description says this:

> Added performance tests related to String.getBytes/String.getChars/StringBuilder.append/System.arraycopy in constant scenarios to verify whether MergeStore works

First: a benchmark is not the best way `to verify whether MergeStore works`. An IR test would be more helpful, as it could check reliably what IR is generated, and hence if MergeStores actually optimized anything.

Second: A JMH benchmark could also be helpful, but only if you run it with and without MergeStores enabled. Otherwise how would you know if it was MergeStores or another optimization that is relevant here?

Third: `getBytes` / `arraycopy` is **NOT** a MergeStores pattern. These are **COPY** patterns. So they probably should go to a separate benchmark file. I don't want the MergeStores benchmark polluted with unrelated cases. I could be wrong here, and just not see how these cases are MergeStore cases, but you need to show the details here.

I put some time in understanding your PR and asking you a list of questions. You did not really respond to them, and that is frustrating to me and makes me feel like my time is not valued: https://github.com/openjdk/jdk/pull/24108#issuecomment-2762946069

You say this:
> By default, in OpenJDK, COMPACT_STRINGS = true, and the String coder without UTF16 characters is LATIN1, which is implemented using System.arraycopy. However, since String is immutable and System.arraycopy is directly performed on byte[], C2 should have more opportunities for optimization.

Maybe the `System.arraycopy` can be optimized. But I don't think it is the MergeStores optimization that would do that. This is really a **Copy** pattern and not a `MergeStores` pattern. Please read the PRs on MergeStores to see what patterns are covered.

And like I asked in previously:
> Can you investigate what code it generates, and what kinds of optimizations are missing to make it close in performance to the Unsafe benchmark?
> I don't have time to do all the deep investigations myself. But feel free to ask me if you have more questions.

To me, benchmarks are only helpful and worth integrating if there is some clear and documented purpose. It would be really nice if you could invest some time into that :)

test/micro/org/openjdk/bench/vm/compiler/MergeStoreBench.java line 693:

> 691:         }
> 692:         BH.consume(off);
> 693:     }

This is a copy pattern, not MergeStores.

test/micro/org/openjdk/bench/vm/compiler/MergeStoreBench.java line 735:

> 733:         }
> 734:         BH.consume(off);
> 735:     }

@wenshao This is a copy pattern. Not a MergeStore pattern. So I can tell you already now that it will not be optimized by MergeStores ;)

test/micro/org/openjdk/bench/vm/compiler/MergeStoreBench.java line 799:

> 797:         }
> 798:         BH.consume(off);
> 799:     }

@wenshao Why would MergeStores work here? This is is a copy pattern. That is not at all covered by MergeStores.

test/micro/org/openjdk/bench/vm/compiler/MergeStoreBench.java line 856:

> 854:         }
> 855:         BH.consume(sb.length());
> 856:     }

Why would you expect MergeStores to work here?

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

Changes requested by epeter (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/24108#pullrequestreview-2734816014
PR Review Comment: https://git.openjdk.org/jdk/pull/24108#discussion_r2024171061
PR Review Comment: https://git.openjdk.org/jdk/pull/24108#discussion_r2024170015
PR Review Comment: https://git.openjdk.org/jdk/pull/24108#discussion_r2024169285
PR Review Comment: https://git.openjdk.org/jdk/pull/24108#discussion_r2024172517