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

[Maurizio Cimadamore]

Hi Alex,
some comments on the langtools code below:

* please remove the new voidClassType constant from Symtab - and use 
this idiom instead:

types.boxedClass(syms.voidType).type;

Note that j.l.Void is always created (see call to 
synthetizeBoxTypeIfMissing(voidType) - but there are (or used to be) 
corner cases where j.l.Void is not available on certain platforms (i.e. 
mobile) so we can't rely on it being always there (as it will lead to 
crashes). The pattern above is safer, as it relies on javac ability to 
fake a box type if it's missing from the platform.

* The code for initializing options in Gen is what we would normally 
model with an enum. I.e. you need something like:

enum StringConcatMode {
    INLINE,
    INDY_PLAIN,
    INDY_CONSTANTS;
}

And then put some logic inside the enum class to parse the option and to 
return the right enum constant. The code will get better as you can 
model two constants (allowIndyStringConcat and 
indyStringConcatConstants) with a single enum value.

* From a design point of view, I see the logic for generating the string 
concat indy calls as split into two components - one that collects 
arguments, iterates through the peels and the merges them together; this 
part is independent from the particular indy strategy used. There's also 
a second part - the one that generates the list of static/dynamic args 
(given the flattened args to the string concat) and then generates the 
final indy call. This latter part is variable, i.e. you get different 
logic for different indy strategies - which means the code ends up with 
many if/else blocks. A possible suggestioin is to make the code more 
object oriented - keep the first part as is, but have an helper class to 
do the job of mapping an argument into a static arg/recipe and then 
generate the final indy call. In other wordas, what you need is a 
builder pattern - you need an object that you can append args to and, 
when finished, you can call a method to generate the resulting indy 
call. There will be a builder for each indy strategy. This is just a 
suggestion on how I would organize the code - it's not a blocking 
comment - but I see the current strategy not scaling very well if e.g. 
you start adding more strategies.

* I need some confirmation here

// Concat the String representation of the constant, except
+ // for the case it contains special tags, which requires us
+ // to expose it as detached constant.
+ String a = arg.type.stringValue();
+ if (a.contains(TAG_CONST) || a.contains(TAG_ARG)) {
+ recipe.append(TAG_CONST);
+ staticArgs.add(a);
+ } else {
+ recipe.append(a);
+ }


My understanding is that your strategy builds a 'recipe' which is 
basically the result of the concat. Since some of the arguments are 
dynamic, you need to put special markers in the recipe, to tell the BSM 
to fetch the dynamic argument and stick it in there. Correct? This in 
turn creates a problem, as it's possible for a concat _constant_ 
argument to contain the special values themselves - if that's the case, 
you use static arguments essentially as an escaping mechanism, right?

* I would expect more of a combinatorial test where you check i.e. 
string concat of different sizes and args - i.e. the dimension of the 
combinatorial spaces are:

- size - how many args to the string concat? This as non obvious 
consequences in terms of peeling
- arg types: constants, dynamic, special constants (i.e. TAG_CONST, TAG_ARG)
- strategy used: vanilla, indy_constats, indy_plain
- codegen context: assignop vs. binary
- target option (-target 8 vs -target 9)

There are several combinatorial tests that generate a source and check 
it on the fly - one example (close to what's needed here) is:

http://hg.openjdk.java.net/jdk9/dev/langtools/file/176472b94f2e/test/tools/javac/lambda/bytecode/TestLambdaBytecode.java


Maurizio

On 19/11/15 20:58, Aleksey Shipilev 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/
>
> 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/
>
> 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
>