Hi Paul, Thanks for the review!
Updated webrevs: http://cr.openjdk.java.net/~shade/8085796/webrev.langtools.01/ http://cr.openjdk.java.net/~shade/8085796/webrev.jdk.01/ More reviews, please :) Comments below: On 11/24/2015 05:16 PM, Paul Sandoz wrote: >> On 19 Nov 2015, at 21:58, Aleksey Shipilev <aleksey.shipi...@oracle.com> >> wrote: >> Webrev: >> http://cr.openjdk.java.net/~shade/8085796/webrev.langtools.00/ > — > > 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. Yes, I was trying not to use (ahem) "new" language features in a language compiler. But it seems javac uses String switches everywhere. Fixed. > I have a marginal preference for an enum StringConcatStrategy with StringBuilder, Indy, IndyWithConstants values. But i will defer to other javac reviewers. I think string values are fine, also because -XD options seem to be Stringly-typed anyhow. >> Webrev: >> http://cr.openjdk.java.net/~shade/8085796/webrev.jdk.00/ >> > My inclination would be to consolidate all the sys props under one doPriv > block. Yes, done. > 200 private static final Map<Key, MethodHandle> CACHE; > > ConcurrentMap? To make it clearer that this is operated on concurrently. Yes. Fixed. > 287 elements = new ArrayList<>(el); > 288 Collections.reverse(el); > 289 elementsRev = new ArrayList<>(el); > > elementsRev = e1? Yes. Fixed. > 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! :-) Yeah, this code is exempted from Indified String Concat to avoid circularity ;) > > 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. Yes, should have used Objects.requireNonNull. The NPE exceptions would not ever fire if you invoke BSM through invokedynamic anyhow. Fixed. > 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. In theory, it can, but I would like to avoid that. The debugging bytecode is not really complicated to warrant moving to a separate Java source file (which should probably required to be public to begin with) and blow up the implementation complexity. MethodHandle-based strategies already do something similar to what you are proposing, but they can use look up private methods. > 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. See! You can experiment with wild ideas like these after the infrastructure lands in JDK. > 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. My rationale for keeping all strategies untouched is based on the premise that most improvements in strategies would probably be some form of existing strategies, and having all strategies handy and tested simplifies improvements. Also, we may have to do a real-life experiments for choosing a better default strategy. > Do you anticipate it might be possible to reduce the scope of the > existing JIT optimisations? Yes, we don't need to extend OptimizeStringConcat heavily anymore, and once all the StringBuilder::append-shaped bytecode phases out of existence, we can drop the compiler optimization on the floor. Thanks, -Aleksey
