RFR: 8163369: Enable generating DMH classes at link time

[Claes Redestad]

On 2016-08-08 14:28, Aleksey Shipilev wrote:
> On 08/08/2016 02:46 PM, Claes Redestad wrote:
>> Hi,
>>
>> please review this change to add the ability to generate
>> DirectMethodHandles to the --generate-jli-classes jlink plugin.
>>
>> The implementation generates all the specified DMHs as methods into a
>> single class, java.lang.invoke.DirectMethodHandle$DMH. At runtime when a
>> DMH's LF is set up, we speculatively resolve the member from this class
>> instead of generating and loading the bytecode as a distinct anonymous
>> class. This avoids loading a potentially large number of anonymous
>> classes at runtime, and also enables other startup optimizations such as
>> allowing CDS to see and dump this class to the shared archive.
>>
>> webrev: http://cr.openjdk.java.net/~redestad/8163369/webrev.01/
> DirectMethodHandle:
>
>   * DMH placeholder class might better be called
> DirectMethodHandle.Holder, or something else that clearly spells out the
> intent?

Sure.

>
> InvokerBytecodeGenerator:
>
>   * Both generateNamedFunctionInvokerImpl() and
> generateLambdaFormInterpreterEntryPointBytes() have methodEpilog() call,
> but no methodPrologue()? classFilePrologue() used to do the prologue.

Nice catch, there was even 2 failed tests I had missed due to this.

>
> GenerateJLIClassesPlugin:
>
>   * Do we actually need this block in this form?
>
>         // DirectMethodHandles
>         // Enable by default
>         boolean dmhEnabled = true;
>         if (mainArgument != null) {
>             Set<String> args = Arrays.stream(mainArgument.split(","))
>                     .collect(Collectors.toSet());
>             if (!args.contains(DMH_PARAM)) {
>                 dmhEnabled = false;
>             }
>         }
>
>   seems equivalent to a simpler:
>
>         boolean dmhEnabled = true;
>         if (mainArgument != null) {
>             List<String> args = Arrays.asList(mainArgument.split(","));
>             if (!args.contains(DMH_PARAM)) {
>                 dmhEnabled = false;
>             }
>          }
>
>   You can even split the mainArgument once and reuse throughout configure()?

Fixed.

>
>   * This block does not check the first argument as the comment suggests,
> only checks the second group:
>
>      String[] typeParts = type.split("_");
>      if (typeParts.length != 2 || typeParts[1].length() != 1
>          || "LJIFDV".indexOf(typeParts[1].charAt(0)) == -1) {
>         throw new PluginException(
>               "Method type signature must be of form [LJIFD]*_[LJIFDV]");
>      }

I'm checking the return type here (the char after _ in "LL_L") and
then checking the first part inside expandSignature. Clarified the
intent with comments.

>
>   * primitiveType('Z') would throw a misleading "Not a primitive: Z"

Yes, added special casing for sub-int primitives (B, S, Z, C)
to indicate that 'I' should be used instead.

>
>   * This can iterate over values only:
>
>     for (Map.Entry<String, List<String>> entry : dmhMethods.entrySet()) {
>        count += entry.getValue().size();
>     }

Sure.

>
>   * I don't know the module visibility rules between jlink and java.base,
> but can we reference DirectMethodHandle.DMH class as literal from the
> plugin?

No, while java.lang.invoke is exported, DirectMethodHandle is 
package-private
which prohibits static visibility from the plugin.

>
>   * requireBasicType(last) is loop invariant here:
>
>      for (int j = 1; j < count; j++) {
>          requireBasicType(last);
>          sb.append(last);
>      }

Sure.

http://cr.openjdk.java.net/~redestad/8163369/webrev.02/

/Claes

>
> Thanks,
> -Aleksey
>