Code (Pre-)Review for JEP 280: Indify String Concat
Aleksey Shipilev
aleksey.shipilev at oracle.com
Sun Nov 29 15:45:25 UTC 2015
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
More information about the core-libs-dev
mailing list