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

[Claes Redestad]

Hi Aleksey,

in 
http://cr.openjdk.java.net/~shade/8085796/webrev.jdk.01/src/java.base/share/classes/java/lang/invoke/StringConcatFactory.java.html 
there are a number of internal strategy classes that will set up a 
number of MethodHandles in bulk once initialized:

1561         private static final MethodHandle NEW_STRING, 
NEW_STRING_CHECKED;
1562         private static final MethodHandle NEW_ARRAY;
1563         private static final Map<Class<?>, MethodHandle> 
PREPENDERS   = new HashMap<>();
1564         private static final Map<Class<?>, MethodHandle> 
LENGTH_MIXERS = new HashMap<>();
1565         private static final Map<Class<?>, MethodHandle> 
CODER_MIXERS = new HashMap<>();

1580             for (Class<?> ptype : new Class<?>[]{boolean.class, 
byte.class, char.class, short.class, int.class, long.class, 
String.class}) {
1581                 PREPENDERS.put(ptype, 
lookupStatic(Lookup.IMPL_LOOKUP, stringHelper, "prepend", int.class,  
int.class, byte[].class, byte.class, ptype));
1582                 LENGTH_MIXERS.put(ptype, 
lookupStatic(Lookup.IMPL_LOOKUP, stringHelper, "mixLen", int.class,  
int.class, ptype));
1583                 CODER_MIXERS.put(ptype, 
lookupStatic(Lookup.IMPL_LOOKUP, stringHelper, "mixCoder", byte.class, 
byte.class, ptype));
1584             }
1585
1586             NEW_STRING         = lookupStatic(Lookup.IMPL_LOOKUP, 
stringHelper, "newString", String.class, byte[].class, byte.class);
1587             NEW_STRING_CHECKED = lookupStatic(Lookup.IMPL_LOOKUP, 
stringHelper, "newStringChecked", String.class, int.class, byte[].class, 
byte.class);
1588             NEW_ARRAY          = lookupStatic(Lookup.IMPL_LOOKUP, 
MethodHandleInlineCopyStrategy.class, "newArray", byte[].class, 
int.class, byte.class);

I'm wondering if these could be made more lazily initialized in manners 
similar to how MethodHandleImpl and BoundMethodHandle were recently 
updated, e.g.:

private static final ConcurrentMap<Class<?>, MethodHandle> PREPENDERS = 
new ConcurrentHashMap<>();

private static MethodHandle getPrepender(Class<?> ptype) {
    return PREPENDERS.computeIfAbsent(ptype, new Function<Class<?>, 
MethodHandle>() {
        @Override
        public MethodHandle apply(Class<?> ptype) {
            // will throw AssertionError if ptype not in {boolean.class, 
byte.class, char.class, short.class, int.class, long.class, String.class}
            lookupStatic(Lookup.IMPL_LOOKUP, stringHelper, "prepend",  
int.class,  int.class, byte[].class, byte.class, ptype);
        }
    });
}

Some variants might be non-existent in a majority of programs, others 
might not be needed early on, which could help reduce the footprint a bit.

Thanks!

/Claes

On 2015-11-24 23:30, Aleksey Shipilev wrote:
> 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
>