Code (Pre-)Review for JEP 280: Indify String Concat

[Paul Sandoz]

> On 19 Nov 2015, at 21:58, Aleksey Shipilev <aleksey.shipilev at oracle.com> wrote:
> 
> Hi,
> 
> I would like to have a code pre-review for Indify String Concat changes.
> For the reference, the rationale, design constraints, etc. are outlined
> in the JEP itself:
>  https://bugs.openjdk.java.net/browse/JDK-8085796
> 
> The experimental notes, including the bytecode shapes, benchmark data,
> and benchmark analysis, interaction with Compact Strings are here:
>  http://cr.openjdk.java.net/~shade/8085796/notes.txt
> 
> Javadoc (CCC is in progress):
> 
> http://cr.openjdk.java.net/~shade/8085796/javadocs/StringConcatFactory.html
> 
> 
> === javac change:
> 
> Webrev:
> http://cr.openjdk.java.net/~shade/8085796/webrev.langtools.00/
> 

I am not a langtools expert but the code made sense to me (as i have dabbled in hacking javac to generate indy)


Gen
—

 134                 if ("inline".equals(concat)) {
 135                     allowIndyStringConcat = false;
 136                     indyStringConcatConstants = false;
 137                 } else if ("indy".equals(concat)) {
 138                     allowIndyStringConcat = true;
 139                     indyStringConcatConstants = false;
 140                 } else if ("indyWithConstants".equals(concat)) {
 141                     allowIndyStringConcat = true;
 142                     indyStringConcatConstants = true;
 143                 } else {
 144                     Assert.error("Unknown stringConcat: " + concat);
 145                     throw new IllegalStateException("Unknown stringConcat: " + concat);
 146                 }

If you are in anyway inclined you could use a switch on concat.

I have a marginal preference for an enum StringConcatStrategy with StringBuilder, Indy, IndyWithConstants values. But i will defer to other javac reviewers.



> A little guide:
> 
> * We cut in at the same two points current javac does the String
> concat. But instead of emitting the StringBuilder append chains, we
> collect all arguments and hand them over to JDK's
> java.lang.invoke.StringConcatFactory.
> 
> * The bytecode flavor is guarded by hidden -XDstringConcat option, with
> three possible values: inline, indy, indyWithConstants.
> "indyWithConstants" is selected as the default bytecode flavor.
> 
> * Since a String concat expression may contain more operands than any
> Java call can manage, we sometimes have to peel the concat in several
> back-to-back calls, and concat-ting the results. That peeling is
> one-level, and can accommodate 200*200 = 40000 arguments, well beyond
> what can be achieved for a singular String concat expression (limited
> either by constant pool size, or by method code size).
> 
> 
> === JDK change:
> 
> Webrev:
> http://cr.openjdk.java.net/~shade/8085796/webrev.jdk.00/
> 

StringConcatFactory
—

 187         final String key = "java.lang.invoke.stringConcat.debug";
 188         String str = AccessController.doPrivileged(
 189                 new GetPropertyAction(key), null,
 190                 new PropertyPermission(key, "read"));
 191         DEBUG = Boolean.valueOf(str);

See also Boolean.getBoolean

My inclination would be to consolidate all the sys props under one doPriv block.


 200     private static final Map<Key, MethodHandle> CACHE;

ConcurrentMap? To make it clearer that this is operated on concurrently.


 287             elements = new ArrayList<>(el);
 288             Collections.reverse(el);
 289             elementsRev = new ArrayList<>(el);

elementsRev = e1?

It’s mildly surprising that there is no supported way to create a reverse view of a ListIterator.


 408         if (DEBUG) {
 409             System.out.println("StringConcatFactory " + STRATEGY + " is here for " + argTypes);
 410         }

No recursion! :-)


 530         if (caller == null) {
 531             throw new StringConcatException("Lookup is null");
 532         }
 533
 534         if (name == null) {
 535             throw new StringConcatException("Name is null");
 536         }
 537
 538         if (argTypes == null) {
 539             throw new StringConcatException("Argument types are null");
 540         }

It’s odd to not see NPEs being used instead.


Can the generated class extend from an existing compiled class or reuse static method calls of another class? then it might be possible to move ASM generation of debug code into java source code.

Here is a wild thought, i dunno if it makes any difference. U.defineAnonymousClass allows one to patch the constant pool, so the string constants could be patched in.

You seem to be hedging your bets about what static strategy to choose. Do you intend to whittle down the strategies over time?
Same argument applies to caching. Some time before the JDK 9 release it would be good to reduce the scope.

Do you anticipate it might be possible to reduce the scope of the existing JIT optimisations?

Paul.

> A little guide:
> 
> * The entry point is java.lang.invoke.StringConcatFactory. Its methods
> would be used as invokedynamic bootstrap methods, at least in javac.
> 
> * There are multiple concatenation strategies supported by SCF. We
> explored the performance characteristics of all factories, chosen one
> performing good throughput- and startup-wise. All strategies are fully
> functional, covered by tests, and kept as the base for the future work
> and fine tuning.
> 
> * Most strategies delegate to public StringBuilder API, which makes
> them quite simple, since they are not forced to deal with actual storage
> (this got complicated with Compact Strings).
> 
> * The only strategy that builds the String directly is
> MH_INLINE_SIZED_EXACT strategy, and thus it is the most complicated of
> all. It uses private java.lang.StringConcatHelper class to get access to
> package-private (Integer|Long).(stringSize|getChar*) methods; the access
> to j.l.SCH is granted by a private lookup.
> 
> * Some tests assume the particular code shape and/or invokedynamic
> counts, needed to fix those as well.
> 
> === Build change
> 
> (I don't copy build-dev@ to avoid spamming that list with langtools/jdk
> reviews; this section is FYI)
> 
> Webrev:
> http://cr.openjdk.java.net/~shade/8085796/webrev.root.00/
> 
> * This one is simple: we exempt java.base and interim classes from Indy
> String Concat to avoid bootstrapping issues.
> 
> 
> Thanks,
> -Aleksey
>