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

[Brian Goetz]

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(";"))));
>>      }
>>  }
>