A small patch to use the StringConcatFactory instead of String.format() to implement toString() of a data class

[forax at univ-mlv.fr]

@Brian,
i've proposed the patch mainly because you write a comment in the code saying that it should be a good idea to use StringConcatFactory instead of String.valueOf,
given i had already written the same kind of code, it doesn't take me a long time.
For the testing part, it not very good at this game.

@Vincente,
i believe calling the StringConcatFactory directly is a mistake, mostly because the having our own BSM allow us to specify a better meta protocol i.e. the boostrap arguments sent to the different invokedynamic.

Speaking of the meta protocol, i wonder if at some point, it's not better to use a condy to encapsulate the data class, the field names and the getters. So the runtime representation that split the names and create the unmodifiable lists can be shared between the 3 invokedynamic calls. 

Also for makeEquals, i think it's worth to first check the fields that are primitive types before checking the fields that are references.  

cheers,
Rémi

----- Mail original -----
> De: "Brian Goetz" <brian.goetz at oracle.com>
> À: "Vicente Romero" <vicente.romero at oracle.com>
> Cc: "Remi Forax" <forax at univ-mlv.fr>, "amber-dev" <amber-dev at openjdk.java.net>
> Envoyé: Lundi 13 Novembre 2017 21:38:25
> Objet: Re: A small patch to use the StringConcatFactory instead of String.format() to implement toString() of a data
> class

> Thanks!  We are refining a combo test to validate these sorts of changes.
> 
> Performance tweaks are low priority for now, and we’ll need to chose based on
> realistic Performance measurements. I am sure the version I wrote is much
> slower than optimal so I doubt well be keeping that one. But if anybody wants
> to work on a performance testing harness for code gen options here that would
> be great!
> 
> Sent from my iPad
> 
>> On Nov 13, 2017, at 5:58 PM, Vicente Romero <vicente.romero at oracle.com> wrote:
>> 
>> Hi Remi,
>> 
>> Thanks for the patch, seems useful. We could add it as an optional
>> implementation and make a comparison between the possible implementations.
>> Another possible implementation, again in the case of the toString, is to
>> generate an indy call directly from the data class to a StringConcatFactory
>> method,
>> 
>> Thanks,
>> Vicente
>> 
>>> On 11/12/2017 04:58 PM, Remi Forax wrote:
>>> Hi all,
>>> A small patch to use the StringConcatFactory to implement the toString of a data
>>> class.
>>> 
>>> The patch manage the case where there is no getter like the current code, even
>>> if you can not create a data class with no field using javac.
>>> 
>>> cheers,
>>> Rémi
>>> 
>>> diff -r a6a70e4541b3
>>> src/java.base/share/classes/java/lang/invoke/ObjectMethodBuilders.java
>>> --- a/src/java.base/share/classes/java/lang/invoke/ObjectMethodBuilders.java
>>> Fri Nov 10 18:51:06 2017 +0100
>>> +++ b/src/java.base/share/classes/java/lang/invoke/ObjectMethodBuilders.java
>>> Sun Nov 12 22:54:42 2017 +0100
>>> @@ -189,49 +189,50 @@
>>>        /**
>>>       * Generates a method handle for the {@code toString} method for a given data
>>>       class
>>> +     * @param lookup          the lookup object
>>>       * @param receiverClass   the data class
>>>       * @param getters         the list of getters
>>>       * @param names           the names
>>>       * @return the method handle
>>>       */
>>> -    private static MethodHandle makeToString(Class<?> receiverClass,
>>> +    private static MethodHandle makeToString(MethodHandles.Lookup lookup,
>>> +                                            Class<?> receiverClass,
>>>                                              List<MethodHandle> getters,
>>>                                              List<String> names) {
>>> -        // This is a pretty lousy algorithm; we spread the receiver over N
>>> places,
>>> -        // apply the N getters, apply N toString operations, and concat the
>>> result with String.format
>>> -        // Better to use String.format directly, or delegate to
>>> StringConcatFactory
>>> -        // Also probably want some quoting around String components
>>> +        // This is a simple algorithm; we spread the receiver over N places,
>>> +        // apply the N getters, then delegate to StringConcatFactory
>>> +        // We may want some quoting around String components ?
>>>            assert getters.size() == names.size();
>>>  -        int[] invArgs = new int[getters.size()];
>>> -        Arrays.fill(invArgs, 0);
>>> -        MethodHandle[] filters = new MethodHandle[getters.size()];
>>> -        StringBuilder sb = new StringBuilder();
>>> -        sb.append(receiverClass.getSimpleName()).append("[");
>>> -        for (int i=0; i<getters.size(); i++) {
>>> -            MethodHandle getter = getters.get(i); // (R)T
>>> -            MethodHandle stringify = stringifier(getter.type().returnType());
>>> // (T)String
>>> -            MethodHandle stringifyThisField =
>>> MethodHandles.filterArguments(stringify, 0, getter);    // (R)String
>>> -            filters[i] = stringifyThisField;
>>> -            sb.append(names.get(i)).append("=%s");
>>> -            if (i != getters.size() - 1)
>>> -                sb.append(", ");
>>> -        }
>>> -        sb.append(']');
>>> -        String formatString = sb.toString();
>>> -        MethodHandle formatter = MethodHandles.insertArguments(STRING_FORMAT,
>>> 0, formatString)
>>> -                                              .asCollector(String[].class,
>>> getters.size()); // (R*)String
>>> -        if (getters.size() == 0) {
>>> -            // Add back extra R
>>> -            formatter = MethodHandles.dropArguments(formatter, 0,
>>> receiverClass);
>>> -        }
>>> -        else {
>>> -            MethodHandle filtered = MethodHandles.filterArguments(formatter, 0,
>>> filters);
>>> -            formatter = MethodHandles.permuteArguments(filtered,
>>> MethodType.methodType(String.class, receiverClass), invArgs);
>>> +        if (getters.isEmpty()) {
>>> +            return MethodHandles.dropArguments(
>>> +                MethodHandles.constant(String.class, "[]"),
>>> +                0, receiverClass);
>>>          }
>>>  -        return formatter;
>>> +        Class<?>[] getterTypes = new Class<?>[getters.size()];
>>> +        StringBuilder recipeBuilder = new StringBuilder();
>>> +        recipeBuilder.append(receiverClass.getSimpleName()).append('[');
>>> +        String separator = "";
>>> +        for (int i = 0; i < getters.size(); i++) {
>>> +            recipeBuilder.append(separator).append(names.get(i)).append("=\1");
>>> +            getterTypes[i] = getters.get(i).type().returnType();
>>> +            separator = ", ";
>>> +        }
>>> +        String recipe = recipeBuilder.append(']').toString();
>>> +        MethodType concatType = MethodType.methodType(String.class,
>>> getterTypes);
>>> +
>>> +        CallSite cs;
>>> +        try {
>>> +            cs = StringConcatFactory.makeConcatWithConstants(lookup,
>>> "toString", concatType, recipe);
>>> +        } catch (StringConcatException e) {
>>> +            throw (LinkageError)new LinkageError().initCause(e);
>>> +        }
>>> +        MethodHandle target = cs.dynamicInvoker();
>>> +        target = MethodHandles.filterArguments(target, 0, getters.toArray(new
>>> MethodHandle[0]));
>>> +        target = MethodHandles.permuteArguments(target,
>>> MethodType.methodType(String.class, receiverClass), new int[getters.size()]);
>>> +        return target;
>>>      }
>>>        /**
>>> @@ -277,6 +278,6 @@
>>>       */
>>>      public static CallSite makeToString(MethodHandles.Lookup lookup, String invName,
>>>      MethodType invType,
>>>                                          Class<?> dataClass, String names, MethodHandle... getters) throws Throwable {
>>> -        return new ConstantCallSite(makeToString(dataClass, List.of(getters),
>>> List.of(names.split(";"))));
>>> +        return new ConstantCallSite(makeToString(lookup, dataClass,
>>> List.of(getters), List.of(names.split(";"))));
>>>      }
>>>  }