A small patch to use the StringConcatFactory instead of String.format() to implement toString() of a data class
forax at univ-mlv.fr
forax at univ-mlv.fr
Tue Nov 14 09:40:52 UTC 2017
@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(";"))));
>>> }
>>> }
More information about the amber-dev
mailing list