Use StringConcatFactory for string concatenation on JDK 9+

[harpocrates]

JEP 280, released in JDK 9, proposes a new way to compile string concatenation using invokedynamic and StringConcatFactory. This new approach generates less bytecode, doesn't incur the overhead of StringBuilder allocations, and allows users to pick swap the concatenation technique at runtime.

This changes the codegen when the target is at least Java 9 to leverage invokedynamic and StringConcatFactory. On Java 8, the old StringBuilder approach is still used.

[harpocrates]

I've unit tested this and it seems to work well. I'd like to know if there is a chance this could be merged before I put more effort into testing and benchmarking though.

[harpocrates]

@harpocrates given the java range thing is now merged, do you want to add a test for this now (so hopefully we can get it merged this week).

I'll add some tests today. Still interested in feedback for whether we want this to be configurable.

[lrytz]

The change LGTM. How do we test it (not only once, but continuously)? We could switch our JDK 16 nightly build on travis to use -target:11 everywhere.

This would be the first feature in the compiler where -target actually has an effect (besides the classfile version).

I'm wondering if we can make this more dynamic so that StringConcatFactory is used when available at runtime. Would that be possible by writing a custom indy bootstrap method? Maybe it would be simpler to rewrite our own StringConcatFactory and ship that in the Scala library.

I think licensing wouldn't allow us to use or look at the implementation in the JDK (@sjrd has experience with this question).

[sjrd]

I think licensing wouldn't allow us to use or look at the implementation in the JDK (@sjrd has experience with this question).

Yeah, as soon as you look at the OpenJDK source code or even its tests, you're out. You can't contribute anything remotely similar to the feature you looked at in a Scala project. That's because OpenJDK is GPL and Scala is Apache.

The only things you can do are to look at the JavaDocs and the specs, and experiment with the behavior on an OpenJDK build (avoid OracleJDK builds, because they have a clause against reverse engineering, and I don't want a lawyer to tell me that poking at the behavior constitutes reverse engineering).

[dwijnand]

We could switch our JDK 16 nightly build on travis to use -target:11 everywhere.

If we were to ever make a release of Scala 2 not building on JDK 8 it would target at least JDK 11, so I would be happy with that change.

Making the compiler emit a fancy indy bootstrap method based on the runtime availability of StringConcatFactory would be wonderful (nerdsnipe) but even without that this would be a great addition.

[harpocrates]

Sorry for dropping the ball on this PR. I'm back and I'll add some tests that basically just test the different code paths (eg. spilling slots, strings that involve some of the magical tags). Those tests will only be actually useful if/when some CI uses -target:11 (the JDK 16 nightly build sounds like a great idea).

I'm wondering if we can make this more dynamic so that StringConcatFactory is used when available at runtime. Would that be possible by writing a custom indy bootstrap method? Maybe it would be simpler to rewrite our own StringConcatFactory and ship that in the Scala library.

Whenever possible, I think we should prefer to use StringConcatFactory over some custom re-implementation:

• we benefit from existing (and future) optimization work that happens in the JDK
• the java.lang.invoke.stringConcat runtime option works uniformly across Java/Scala

Making the compiler emit a fancy indy bootstrap method based on the runtime availability of StringConcatFactory would be wonderful (nerdsnipe) but even without that this would be a great addition.

Agreed! Even in this case though, I would expect an indy call to such a custom BM to only be emitted for -target:8, since the runtime check is pointless for newer targets.

[harpocrates]

I added a test. It just tests string concatenation in general though, so on -target:8 it'll use the old StringBuilder and on -target:9+ it'll use indy. I manually checked that both work (but not through partest, since I can't get the stack configuration to work).

[harpocrates]

Thanks @lrytz for the help working around the stack overflow in the test! Do we need more tests, or is this enough? If everyone is satisfied, I'll squash the history.

[SethTisue]

ping @lrytz (but if you've got too much else to do and you want to hand this over to me, that would be fine)

[lrytz]

LGTM, squashed!