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

[Ivan Gerasimov]

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
>
>