[External] : Re: String concatenation order of operations

[Liam Miller-Cushon]

On Wed, Sep 8, 2021 at 3:01 PM Brian Goetz <brian.goetz at oracle.com> wrote:

> The natural way to do this is to have the various arguments passed as real
> stack arguments, causing them to all be evaluated before being pushed
> through the MH nest
>

I'm not sure I understand this part. Aren't non-constant arguments already
being evaluated and passed as stack arguments to the invokedynamic?

On Thu, Sep 16, 2021 at 11:51 PM John Rose <john.r.rose at oracle.com> wrote:

> Wouldn’t be enough for javac to run toString() on arguments that need it,
> and pass those (to the *old* indy mechanism) as strings?
>
> If javac can determine that toString() can be safely delayed (for String
> and primitive boxes, for example) then it doesn’t need to call toString()
> eagerly.
>

That sounds like it'd work to me.

On Fri, Sep 17, 2021 at 6:25 AM Remi Forax <forax at univ-mlv.fr> wrote:

> It will be less efficient because the StringConcatFactory for a primitive
> type does not create the intermediary String objects.
>

Remi isn't that addressed by John's suggestion to skip eagerly calling
toString() for types where it's safe to do so (including primitives and
their boxes, and Strings)?

The main performance disadvantage might be the additional bytecode from the
extra calls to toString (or valueOf, for null-safety), but I'm not sure how
to avoid that and also have the evaluation order required by the spec.

The following patch adds the extra valueOf calls to the indyWithConstants
strategy, and passes tier1 langtools tests (*). If there's interest in
pursuing this approach I can write a test and open a PR.

(*) except for test/langtools/tools/javac/StringConcat/access/Test.java,
which is testing the types of the arguments in the invokedynamic call and
needs updating

diff --git
a/src/jdk.compiler/share/classes/com/sun/tools/javac/jvm/StringConcat.java
b/src/jdk.compiler/share/classes/com/sun/tools/javac/jvm/StringConcat.java
index f11054d7302..5a46fd8a468 100644
---
a/src/jdk.compiler/share/classes/com/sun/tools/javac/jvm/StringConcat.java
+++
b/src/jdk.compiler/share/classes/com/sun/tools/javac/jvm/StringConcat.java
@@ -439,10 +439,16 @@ public abstract class StringConcat {
                     } else {
                         // Ordinary arguments come through the dynamic
arguments.
                         recipe.append(TAG_ARG);
-                        dynamicArgs.add(sharpestAccessible(arg.type));
+                        Type argType = sharpestAccessible(arg.type);
                         if (!first || generateFirstArg) {
                             gen.genExpr(arg, arg.type).load();
+                            if
(!types.unboxedTypeOrType(argType).isPrimitive() && argType.tsym !=
syms.stringType.tsym) {
+                                gen.callMethod(pos, syms.stringType,
names.valueOf,
+                                        List.of(syms.objectType), true);
+                                argType = syms.stringType;
+                            }
                         }
+                        dynamicArgs.add(argType);
                         first = false;
                     }
                 }
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/compiler-dev/attachments/20210918/66c3cd87/attachment.htm>