RFR: 8245024: Simplify and eagerly initialize StringConcatFactory

[Claes Redestad]

On 2020-05-14 17:28, Paul Sandoz wrote:
> Looks good.

Thanks!

> 
> —
> 
> 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?

It has been argued the alternative strategies might have a role to play
as a fallback if the default strategy ever fails. So far we've not seen
any reports to that effect, and the default strategy has been optimized
substantially since inception, while alternatives have stayed more or
less the same since 9 (barring some adjustment to work with hidden
classes recently).

I think a more effective fallback in face of any issues would be to
desugar ISC call-sites to classic StringBuilder chains at link-time[1],
like one would do at (javac) compile-time with -XDconcat=inline.

But yeah, another issue.

/Claes

[1] "easy" to do with jlink, slightly tricker - but possible? - to do so
as a linking step when loading the class into the VM

> 
> Paul.
> 
>> On May 14, 2020, at 7:39 AM, Claes Redestad <claes.redestad at 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
>