Code (Pre-)Review for JEP 280: Indify String Concat
Ivan Gerasimov
ivan.gerasimov at oracle.com
Fri Nov 27 11:33:53 UTC 2015
Hi Alexey!
Not a thorough review, but a few of minor comments:
1) As you touch Long.java anyway, could you change
508 while (i <= Integer.MIN_VALUE) {
to
508 while (i < Integer.MIN_VALUE) {
, which may save us a half of nanosecond on some inputs?
And the same on the line# 563.
2) Integer.java and Long.java
In the javadoc, just for a sake of accuracy:
550 * @return index of the most significant digit *or minus sign,
if present*
3) StringConcatFactory.java
255 acc = new StringBuilder();
wouldn't it be a bit more efficient to do `acc.setLength(0)`?
4) StringConcatFactory.java
586 try {
587 mh = generate(caller, mt, rec);
588 } catch (Throwable t) {
589 if (t instanceof StringConcatException) {
590 throw (StringConcatException)t;
591 } else {
592 throw new StringConcatException("Generator
failed", t);
593 }
594 }
it seems to be a bit more straight-forward:
try {
mh = generate(caller, mt, rec);
} catch (StringConcatException sce) {
throw sce;
} catch (Throwable t) {
throw new StringConcatException("Generator failed", t);
}
Sincerely yours,
Ivan
On 19.11.2015 23:58, Aleksey Shipilev wrote:
> Hi,
>
> I would like to have a code pre-review for Indify String Concat changes.
> For the reference, the rationale, design constraints, etc. are outlined
> in the JEP itself:
> https://bugs.openjdk.java.net/browse/JDK-8085796
>
> The experimental notes, including the bytecode shapes, benchmark data,
> and benchmark analysis, interaction with Compact Strings are here:
> http://cr.openjdk.java.net/~shade/8085796/notes.txt
>
> Javadoc (CCC is in progress):
>
> http://cr.openjdk.java.net/~shade/8085796/javadocs/StringConcatFactory.html
>
>
> === javac change:
>
> Webrev:
> http://cr.openjdk.java.net/~shade/8085796/webrev.langtools.00/
>
> A little guide:
>
> * We cut in at the same two points current javac does the String
> concat. But instead of emitting the StringBuilder append chains, we
> collect all arguments and hand them over to JDK's
> java.lang.invoke.StringConcatFactory.
>
> * The bytecode flavor is guarded by hidden -XDstringConcat option, with
> three possible values: inline, indy, indyWithConstants.
> "indyWithConstants" is selected as the default bytecode flavor.
>
> * Since a String concat expression may contain more operands than any
> Java call can manage, we sometimes have to peel the concat in several
> back-to-back calls, and concat-ting the results. That peeling is
> one-level, and can accommodate 200*200 = 40000 arguments, well beyond
> what can be achieved for a singular String concat expression (limited
> either by constant pool size, or by method code size).
>
>
> === JDK change:
>
> Webrev:
> http://cr.openjdk.java.net/~shade/8085796/webrev.jdk.00/
>
> A little guide:
>
> * The entry point is java.lang.invoke.StringConcatFactory. Its methods
> would be used as invokedynamic bootstrap methods, at least in javac.
>
> * There are multiple concatenation strategies supported by SCF. We
> explored the performance characteristics of all factories, chosen one
> performing good throughput- and startup-wise. All strategies are fully
> functional, covered by tests, and kept as the base for the future work
> and fine tuning.
>
> * Most strategies delegate to public StringBuilder API, which makes
> them quite simple, since they are not forced to deal with actual storage
> (this got complicated with Compact Strings).
>
> * The only strategy that builds the String directly is
> MH_INLINE_SIZED_EXACT strategy, and thus it is the most complicated of
> all. It uses private java.lang.StringConcatHelper class to get access to
> package-private (Integer|Long).(stringSize|getChar*) methods; the access
> to j.l.SCH is granted by a private lookup.
>
> * Some tests assume the particular code shape and/or invokedynamic
> counts, needed to fix those as well.
>
> === Build change
>
> (I don't copy build-dev@ to avoid spamming that list with langtools/jdk
> reviews; this section is FYI)
>
> Webrev:
> http://cr.openjdk.java.net/~shade/8085796/webrev.root.00/
>
> * This one is simple: we exempt java.base and interim classes from Indy
> String Concat to avoid bootstrapping issues.
>
>
> Thanks,
> -Aleksey
>
>
More information about the core-libs-dev
mailing list