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

[Aleksey Shipilev]

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.shipilev at 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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <http://mail.openjdk.java.net/pipermail/compiler-dev/attachments/20151125/0cae1a61/signature.asc>