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

Claes Redestad Tue, 24 Nov 2015 16:09:07 -0800

Hi Aleksey,

inhttp://cr.openjdk.java.net/~shade/8085796/webrev.jdk.01/src/java.base/share/classes/java/lang/invoke/StringConcatFactory.java.htmlthere are a number of internal strategy classes that will set up anumber 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 mannerssimilar to how MethodHandleImpl and BoundMethodHandle were recentlyupdated, 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, othersmight 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.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

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

Reply via email to