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

Aleksey Shipilev aleksey.shipilev at
Fri Nov 27 00:20:13 UTC 2015

Hi Maurizio,

Thanks for the reviews!

Updated webrevs:

On 11/26/2015 02:40 PM, Maurizio Cimadamore wrote:
> 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.

Excellent, thanks! Didn't know that nuisance with j.l.Void availability.
Removed in favor of the suggested pattern.

> * 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,
> }
> 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.

Yes, indeed, done.

> * 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.

Yes, I stepped further and created a separate helper class that does all
the String concat. Moved all the logic, including the current
StringBuilder-based one there, this lets to un-clutter Gen itself.

Please see the updated webrevs. I would need to polish langtools changes
a bit more, but tell me if the general direction is what you like.

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

Yes, that is a correct understanding. The credit for the idea actually
goes to John Rose. This scheme has a nice implication that *most*
recipes would have only the recipe string + dynamic arguments, and no
other static arguments. Which saves space in constant pool. Also, some
compilers may find it easier to avoid building a complicated recipe, and
rather pass the String constants untouched.

There is a guidance in StringConcatFactory for that:

 @apiNote Code generators have three distinct ways to process a constant
 string operand S in a string concatenation expression.  First, S can be
 materialized as a reference (using ldc) and passed as an ordinary
 argument (recipe '\1'). Or, S can be stored in the constant pool and
 passed as a constant (recipe '\2') . Finally, if S contains neither of
 the recipe tag characters ('\1', '\2') then S can be interpolated into
 the recipe itself, causing its characters to be inserted into the

> * 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)

In fact, we have these tests in jdk/test/java/lang/String concat, see
the jdk webrev. The rationale for keeping these tests in jdk: you would
want to test the JDK concat code along with those combinatorial tests,
and verify the *actual* concatenation result is sane. Simply generating
the bytecode and trying to verify it seems to be a half-way approach to
the problem (also amenable to bugs in your own bytecode verifier!)


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <>

More information about the compiler-dev mailing list