Code (Pre-)Review for JEP 280: Indify String Concat
Paul Sandoz
paul.sandoz at oracle.com
Tue Nov 24 14:16:57 UTC 2015
> 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
>
More information about the core-libs-dev
mailing list