RFR: 8152641: Plugin to generate BMH$Species classes ahead-of-time

[Peter Levart]

Hi Claes, Mandy,

On 03/25/2016 03:58 PM, Claes Redestad wrote:
> Hi,
>
> On 2016-03-25 11:11, Peter Levart wrote:
>> Hi Claes, Mandy,
>>
>> On 03/25/2016 02:51 AM, Mandy Chung wrote:
>>> Hi Claes,
>>>
>>> This is a good interesting work to generate 
>>> BoundMethodHandle$Species_* classes at link time.  This will save 
>>> the class generation time.  Instead of Class.forName, you may want 
>>> to have a class (similar to SystemModules [1]) that will be updated 
>>> at link time so that BoundMethodHandle can reference it statically 
>>> to determine what species are generated at link time and even return 
>>> statically Class<?> of the generated class.
>>
>> You may still want to load this classes lazily when needed. Otherwise 
>> the BMH.CLASS_CACHE could simply be pre-populated with name -> 
>> Class<?> entries at appropriate early time.
>
> I think laziness is a good property here, since it won't penalize 
> modular applications that choose to instruct jlink to pregenerate a 
> lot of BMH classes.
>
> Perhaps we can allow for static resolve + lazy classload by emitting a 
> switch equivalent to:
>
> switch (types) {
>   case "LL":
>     return Species_LL.class;
>   case "L3":
>     return Species_L3.class;
>   ..
>   default:
>     return generateBMHSpeciesClass(types);
> }

Comparing this with proposed code from webrev:

  493                         try {
  494                             return (Class<? extends 
BoundMethodHandle>)
  495 Class.forName("java.lang.invoke.BoundMethodHandle$Species_" + 
LambdaForm.shortenSignature(types));
  496                         } catch (ClassNotFoundException cnf) {
  497                             // Not pregenerated, generate the class
  498                             return generateConcreteBMHClass(types);
  499                         }


...note that the function passed to CLASS_CACHE.computeIfAbsent is 
executed just once per distinct 'types' argument. Even if you put the 
generated switch between a call to getConcreteBMHClass and 
CLASS_CACHE.computeIfAbsent, getConcreteBMHClass(String types) is 
executed just once per distinct 'types' argument (except in rare 
occasions when VM can not initialize the loaded class).

In this respect a successful Class.forName() is not any worse than 
static resolving. It's just that unsuccessful Class.forName() represents 
some overhead for classes that are not pre-generated. So an alternative 
way to get rid of that overhead is to have a HashSet of 'types' strings 
for pre-generated classes at hand in order to decide whether to call 
Class.forName or generateConcreteBMHClass.

What's easier to support is another question.

Regards, Peter

>
>>
>>>
>>> About CNFE, the new Class.forName(Module, String) method does not 
>>> throw CNFE that you could use instead (if not static reference as 
>>> suggested above).  If not found, it will return null.
>
> Thanks, this one had passed under my radar. :-)
>
>>>
>>> This plugin accesses the non-public methods in BoundMethodHandle  
>>> and has to invoke it via reflection.  With module boundary enforced 
>>> now, maybe we could consider moving to some internal package to 
>>> share with this jlink plugin.
>>>
>>> I think there are tests needed to be updated when a new plugin is 
>>> added.
>>>
>>> Mandy
>>> [1] 
>>> http://hg.openjdk.java.net/jdk9/dev/jdk/file/3abd25870915/src/java.base/share/classes/jdk/internal/module/SystemModules.java
>>
>> One thing to consider is the following: the names of the classes obey 
>> a particular convention which is now hardcoded in two places: the 
>> BoundMethodHandle and the GenerateBMHClassesPlugin. You could have a 
>> method with the following signature in BoundMethodHandle$Factory:
>>
>> Map.Entry<String, byte[]> generateConcreteBMHClassBytes(String types)
>>
>> ...which would also return the name of the class derived from the 
>> types which you would then use to derive the path of the .class file 
>> in the module of the image. Also, this method - although 
>> package-private becomes part of the API and that should be written in 
>> a comment.
>>
>> What do you think?
>
> Definitely should add a comment about generateConcreteBMHClassBytes 
> being an internal API used by the plugin.
>
> I'm open to the idea of moving static utility code to some internal 
> package, but want to hear what maintainers of java.lang.invoke think 
> about factoring out utility code like this.
>
> Thanks!
>
> /Claes
>
>>
>> Regards, Peter
>>
>>>
>>>> On Mar 24, 2016, at 6:42 AM, Claes Redestad 
>>>> <claes.redestad at oracle.com> wrote:
>>>>
>>>> Hi,
>>>>
>>>> please review this patch which add an enabled-by-default plugin to 
>>>> generate a
>>>> configurable list of BoundMethodHandle$Species_*-classes using jlink.
>>>>
>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8152641
>>>> Webrev: http://cr.openjdk.java.net/~redestad/8152641/webrev.01/
>>>>
>>>> This plugin adds the --generate-bmh flag to jlink, which takes a
>>>> comma-separated list of shortened species type strings.
>>>>
>>>> As an example: --generate-bmh LL,L3,L4 will generate
>>>> BoundMethodHandle$Species_LL, ...$Species_L3 and ...$Species_L4 and 
>>>> add
>>>> them to the java.base module.
>>>>
>>>> The Species class lookup in BoundMethodHandle will first check if 
>>>> there is a
>>>> generated class, otherwise generate it as previously. Adding an 
>>>> exceptional
>>>> path might seem problematic, but as the code generation is 
>>>> magnitudes more
>>>> expensive than the exception it doesn't seem fruitful to go to 
>>>> lengths to avoid
>>>> the CNFE.
>>>>
>>>> More notes along with some results can be found here:
>>>>
>>>> http://cr.openjdk.java.net/~redestad/scratch/bmh_species_gen.txt
>>>>
>>>> Thanks!
>>>>
>>>> /Claes
>>
>