Looks good. —
Separately, not for this issue, the code changes you made impact what is mostly unused code. I don’t recall us ever changing the default strategy, nor I am aware of others changing it via a system property (except for our tests), are you? At the time when we introduced this feature we were not sure what might play out, now the dust has settled and we are more informed shall we consider removing the code for the other non-default strategies, thereby reducing the maintenance burden? Paul. > On May 14, 2020, at 7:39 AM, Claes Redestad <claes.redes...@oracle.com> wrote: > > Hi, > > JDK-8155090 added some measures to StringConcatFactory to prevent > certain bootstrapping issues. I think we should consider eagerly > initializing the StringConcatFactory base class during bootstrap > instead. > > Doing so will add a tiny bit of class loading overhead to bootstrap > (which of course will be amortized on first use of SCF) but still this > would also be a good opportunity to clean up a little. > > One straightforward means to do so is to move the CACHE, DUMPER and > CACHE_ENABLE fields to BytecodeStringBuilderStrategy, since it's the > only place where they are used (the MethodHandle based strategies don't > manually spin classes and rely on LF sharing to de-duplicate shared > components). > > Webrev: http://cr.openjdk.java.net/~redestad/8245024/open.00/ > Bug: https://bugs.openjdk.java.net/browse/JDK-8245024 > > This reduces initialization overhead and simplifices bootstraps for the > default case, while still passing relevant test cases such as test/jdk > /java/lang/String/concat/WithSecurityManager.java for all strategies. > > Testing: tier1-2 > > Thanks! > > /Claes
