On Mon, 26 Aug 2024 22:25:49 GMT, Shaojin Wen <d...@openjdk.org> wrote:
>> This is a follow-up to PR #20273, which improves performance when the number >> of parameters exceeds 20. >> >> When the number of parameters is large, the possibility of reuse will be >> lower, so we can use the static concat method and write the length and coder >> directly into the bytecode to solve the performance regression problem. > > Shaojin Wen has updated the pull request incrementally with two additional > commits since the last revision: > > - reuseThreshold -> cacheThreshold > - Revert "optimize for CompactStrings is off" > > This reverts commit a9fa264afd9fa625ef29357a7ca8559ce9c5fea4. So the code shape from inlining hard-coded constants is smaller thus there might be a warmup benefit? OK, looks somewhat insignificant. What happens if you run something more realistic at scale with many string concats expressions across a larger application - what happens then? You'd be generating a new class at every call site above `cacheThreshold`, so every time we encounter an expression that would have been a cache hit we instead increase application footprint slightly. And each freshly spun class will have to be interpreted many times before being JIT compiled. So that potential warmup win you notice on the `StringConcat` might quickly turn into a loss, since we'd do more or less the same work over and over. I remain unconvinced that this `cacheThreshold` should be anything but an opt-in feature. It might be great for many smaller apps that have a known small number of concatenations and want to reach for that potential win. But in the meantime I think we should treat the performance difference between the now-default translation and this static hard-coded translation as a bug and work towards getting the same performance out of the former instead of selecting a default that might have severe downsides at scale. ------------- PR Comment: https://git.openjdk.org/jdk/pull/20675#issuecomment-2311358786
