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

[Aleksey Shipilev]

Thanks again, I'll upload the webrevs after further langtools cleanup.


On 11/28/2015 12:05 PM, Andrej Golovnin wrote:
>>>   http://cr.openjdk.java.net/~shade/8085796/webrev.jdk.04/
> 
> src/java.base/share/classes/java/lang/invoke/StringConcatFactory.java
> 
> 356         ARG,
> 
> The comma is not needed.

Yes.


>  548             // Mock the recipe to reuse the concat generator code
>  549             StringBuilder sb = new StringBuilder();
>  550             for (int c = 0; c < concatType.parameterCount(); c++) {
>  551                 sb.append(TAG_ARG);
>  552             }
>  553             recipe = sb.toString();
> 
> This can be rewritten as:
> 
> char[] value = new char[concatType.parameterCount()];
> Arrays.fill(value, TAG_ARG);
> recipe = new String(value);

Yes.


> And when you add to the JavaLangAccess interface a new method
> newStringUnsafe(byte[] value, byte coder), then you can even use the
> package private constructor String(byte[], byte) to avoid copying of
> bytes. But I'm not sure if it is worthwhile.

Actually, java.lang.StringConcatHelper gains access to that constructor.
But this is important only hot path, which is linkage method is
hopefully not the part of.


>  558         int cCount = 0;
>  559         int oCount = 0;
>  560         for (int i = 0; i < recipe.length(); i++) {
>  561             char c = recipe.charAt(i);
>  562             if (c == TAG_CONST) cCount++;
>  563             if (c == TAG_ARG)   oCount++;
>  564         }
> 
> This loop is only needed, when the recipe is already known. When you
> move the lines 558 and 559 before the line
> 
>  547         if (generateRecipe) {
> 
> then you can initialize oCount directly after the line 547:
> 
> oCount = concatType.parameterCount();
> 
> and move the for-loop (lines 560-564) into the else clause of the "if
> (generateRecipe)" statement.

Yes, okay.


> Btw. I would rename the variables cCount and oCount to constCount and argCount.
> 
>  601         Key key = new Key(mt, rec);
>  602         MethodHandle mh = CACHE_ENABLE ? CACHE.get(key) : null;
> 
> If you rewrite this lines as:
> 
>  601         Key key = null;
>  602         MethodHandle mh = CACHE_ENABLE ? CACHE.get(key = new
> Key(mt, rec)) : null;
> 
> then you can avoid creation of Key objects when caching is disabled.

I rearranged the code there, so that Key is never needed when caching is
disabled.


>  669         SIZED_EXACT(true, true),
> 
> The comma is not needed.

Yes.


>  672         boolean sized, exact;
> 
> sized and exact should be final.

Yes.


> 1619         private static Class<?> STRING_HELPER;
> 1620
> 1621         static {
> 1622             try {
> 1623                 STRING_HELPER =
> Class.forName("java.lang.StringConcatHelper");
> 1624             } catch (ClassNotFoundException e) {
> 1625                 throw new AssertionError(e);
> 1626             }
> 
> Why STRING_HELPER is not final?

No reason, overlook. Fixed.


> I have a small question to the Recipe class. Is it really better to
> create elementsRev instead of using the old-school for-loop and
> List.get(int) to loop over the list of RecipeElements in the reverse
> order? If there is any reason, then could you please document it to
> avoid stupid questions in the future? :-)

I just think that a for-each loop over a collection is simpler there.


> src/java.base/share/classes/java/lang/invoke/StringConcatException.java
> 
>   31     private static final long serialVersionUID = 292L + 9L;
> 
> I see that the pattern for serialVersionUID like this is already used
> in four other classes (BootstrapMethodError, MethodType,
> WrongMethodTypeException, LambdaConversioinException). Does it have
> some special meaning? I'm just curious.

I think this means: JSR 292 (invokedynamic) exception, introduces in JDK
9. StringConcatException follows suit.

Thanks,
-Aleksey