On Wed, 24 Jul 2024 10:18:03 GMT, Shaojin Wen <d...@openjdk.org> wrote:
>> The current implementation of StringConcat is to mix the coder and length >> into a long. This operation will have some overhead for int/long/boolean >> types. We can separate the calculation of the coder from the calculation of >> the length, which can improve the performance in the scenario of concat >> int/long/boolean. >> >> This idea comes from the suggestion of @l4es in the discussion of PR >> https://github.com/openjdk/jdk/pull/20253#issuecomment-2240412866 > > Shaojin Wen has updated the pull request incrementally with ten additional > commits since the last revision: > > - minor refactor > - minor refactor > - reduce change > - copyright > - reduce change > - refactor based on 8335182 > - use Float.toString & Double.toString > - remove ForceInline > - fix build error This is looking much better! An idea: evolve this PR to first and foremost be a replacement for the current `SimpleStringBuilderStrategy`. If you can prove the new strategy is always superior to the baseline `SimpleStringBuilderStrategy` (performance close to `generateMHInlineCopy` but without the problematic scaling JIT overheads for high-arity expressions) then we have a good, incremental improvement with relatively low risk. We can then follow-up with making this better for low-arity expressions by implementing a sharing strategy (need to build classes where we inject rather than embed constants and cache them, e.g. in a `ReferencedKeyMap<MethodType, Function<String[], MethodHandle>`) as a follow-up. One issue for both this and that future PR is that the hidden classes are generated into `java.lang` with `Lookup.ClassOption.STRONG`. This means such classes effectively can never be unloaded. That's a blocker. We need to get rid of that `STRONG` coupling, add tests to ensure generated classes can be unloaded and validate that this doesn't add too much overhead. For startup verification I added a `StringConcatStartup` JMH in #19927. You could extend that with a few tests that exercise high-arity expressions. src/java.base/share/classes/java/lang/invoke/StringConcatFactory.java line 153: > 151: @SuppressWarnings("removal") > 152: private static boolean isGenerateInlineCopy() { > 153: return GENERATE_INLINE_COPY && System.getSecurityManager() == > null; This security manager hack is unfortunate. Likely some bootstrapping issue due use of the classfile API, which might be pulling in lambdas in the paths travelled by this patch. src/java.base/share/classes/java/lang/invoke/StringConcatFactory.java line 1253: > 1251: } > 1252: > 1253: int lengthCoderSloat = paramSlotsTotalSize; Suggestion: int lengthCoderSlot = paramSlotsTotalSize; ------------- PR Review: https://git.openjdk.org/jdk/pull/20273#pullrequestreview-2196340289 PR Review Comment: https://git.openjdk.org/jdk/pull/20273#discussion_r1689583138 PR Review Comment: https://git.openjdk.org/jdk/pull/20273#discussion_r1689578966
