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
