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
