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

[Claes Redestad]

On 2016-04-01 16:57, Mandy Chung wrote:

On Apr 1, 2016, at 5:32 AM, Claes Redestad  wrote:

http://cr.openjdk.java.net/~redestad/8152641/webrev.06/


Looks good except the new test with 2015 copyright start year.


Fixed. Thanks Mandy!

/Claes

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

[Mandy Chung]

> On Apr 1, 2016, at 5:32 AM, Claes Redestad  wrote:
> 
> http://cr.openjdk.java.net/~redestad/8152641/webrev.06/
> 

Looks good except the new test with 2015 copyright start year.

> 
> [1] Mandy filed https://bugs.openjdk.java.net/browse/JDK-8153238 to improve 
> this test.

Yes the test should be revised to validate a well-known list of plugins rather 
than hardcoding the number of plugins.

Mandy

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

[Claes Redestad]

Hi Mandy et al,

On 2016-04-01 01:10, Mandy Chung wrote:

GenerateBMHClassesPlugin::configure
The plugin should validate of the input argument and throw an exception if 
it’s invalid.  The plugin API is still being revised and JDK-8152800 is related 
to the exception case.  The existing plugins throw PluginException.  
GenerateBMHClassesPlugin can do the same.

Perhaps the expanded signatures should be stored after validation.

GenerateBMHClassesPlugin::generateConcreteClass
It issues a warning if any exception thrown.  If the user specifies the 
types in the command line, they should specify the valid values (I would 
expect).  I prefer it to be an error and throw PluginException.

It may be time to rename this plugin to —-generate-jli-classes plugin, as John 
suggests.  A jlink plugin can define its sub options e.g. 
—-generate-jli-classes=bmh[:species=LL,LLL].

Can you add a test to sanity test if the classes are generated (both the 
default species types as well as specified in the input argument)?

Mandy


http://cr.openjdk.java.net/~redestad/8152641/webrev.06/

- Renamed GenerateBMHClassesPlugin to GenerateJLIClassesPlugin, 
--generate-bmh -> --generate-jli-classes
- Implement control of BMH species with sub-parameters 
--generate-jli-classes=bmh:bmh-species=LL,L3,L5 etc. This will be easy 
to extend into something like 
--generate-jli-classes=all:bmh-species=LL,L3,...:invokers=??,...:lambda-forms=??,... 
as we add other things to this plugin

- Expand and validate input in the plugin
- Add a test which tests default, explicit, invalid and disabled cases
- Add a defaultSpecies() method for testing convenience together with a 
comment alluding to how it was generated.

- Bumped the counter in JLinkTest...[1]

To try and answer John's other comments: Yes, I intend to file RFEs and 
work on adding support to generate other j.l.i classes in the near 
future as time allows, and also have some naïve hope that we can add 
support to generate the default input arguments at build-time.


Thanks for all the feedback and reviews!

/Claes

[1] Mandy filed https://bugs.openjdk.java.net/browse/JDK-8153238 to 
improve this test.

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

[Mandy Chung]

> On Mar 31, 2016, at 4:10 PM, Mandy Chung  wrote:
> 
> 
>> On Mar 30, 2016, at 9:17 AM, Claes Redestad  
>> wrote:
>> 
>> Hi Peter,
>> 
>> something like this, then:
>> 
>> http://cr.openjdk.java.net/~redestad/8152641/webrev.05/
>> 
> 
> BoundMethodHandle::generateConcreteBMHClassBytes
>   It only allows “LIJFD” characters.  But the default species types include 
> digit e.g. L3, L4, etc.  Do you see the warnings generated from 
> GenerateBMHClassesPlugin?

Ignore this comment - I meant to take it out but forgot.

Mandy

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

[Mandy Chung]

> On Mar 30, 2016, at 9:17 AM, Claes Redestad  wrote:
> 
> Hi Peter,
> 
> something like this, then:
> 
> http://cr.openjdk.java.net/~redestad/8152641/webrev.05/
> 

BoundMethodHandle::generateConcreteBMHClassBytes
   It only allows “LIJFD” characters.  But the default species types include 
digit e.g. L3, L4, etc.  Do you see the warnings generated from 
GenerateBMHClassesPlugin?

Nit: the method parameters are wrapped in the next line with 8-space 
indentation. It’d be better to follow the convention this source file uses.

GenerateBMHClassesPlugin::configure
   The plugin should validate of the input argument and throw an exception if 
it’s invalid.  The plugin API is still being revised and JDK-8152800 is related 
to the exception case.  The existing plugins throw PluginException.  
GenerateBMHClassesPlugin can do the same.

   Perhaps the expanded signatures should be stored after validation.

GenerateBMHClassesPlugin::generateConcreteClass
   It issues a warning if any exception thrown.  If the user specifies the 
types in the command line, they should specify the valid values (I would 
expect).  I prefer it to be an error and throw PluginException.

It may be time to rename this plugin to —-generate-jli-classes plugin, as John 
suggests.  A jlink plugin can define its sub options e.g. 
—-generate-jli-classes=bmh[:species=LL,LLL].

Can you add a test to sanity test if the classes are generated (both the 
default species types as well as specified in the input argument)?

Mandy

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

[Mandy Chung]

> On Mar 29, 2016, at 4:03 PM, Claes Redestad  wrote:
> 
> Mandy: I've not found any test (under jdk/test/tools/jlink or elsewhere) which
> has to be updated when adding a plugin like this.

jdk/test/tools/jlink/JLinkTest.java

This is the one I recalled.  This is in the core_tools or tier2 test group.  
The jlink tests are not run in the default test group.

Mandy

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

[John Rose]

I like it too.  Reviewed.

A couple of comments:

The default list of species in the plugin (List.of("LL", "L3", … "I", "LLILL")) 
should
be commented with information for maintainers to reproduce the procedure you
used to generate that list.  Otherwise the list is just magic, and 
unmaintainable.

I'm glad to see this pre-generation work happening.  Please consider (or file
a followup RFE) pre-generating other classes dynamically generated by jli.
The lambda form editor produces a large number of stereotyped classes.
In the common case where an anonymous class is generated just for one
static method, the linker plugin should generate a *single* named class
containing *all* of the static methods.

Eventually, I think, you will want a jlink option which will control the 
pre-generation
of all sorts of jli classes.  Perhaps it would be it would be something like
"--generate-jli-classes=", where the argument includes a way
to specify all the BMH species you are working with now, but would also
provide a way to ask for LFE transforms.  What you have now is fine as
far as it goes.

— John

On Mar 30, 2016, at 10:11 AM, Vladimir Ivanov  
wrote:
> 
> Nice work, Claes!
> 
> j.l.i part looks fine.
> 
> Best regards,
> Vladimir Ivanov
> 
> On 3/30/16 7:17 PM, Claes Redestad wrote:
>> Hi Peter,
>> 
>> something like this, then:
>> 
>> http://cr.openjdk.java.net/~redestad/8152641/webrev.05/
>> 
>> - Added a method only used by the plugin which validates input; added a
>> comment about the dependency
>> - Invalid types are logged but otherwise ignored now (I bet someone
>> might suggest a better way to handle user errors)
>> - Some cleanup, introduced constant for class name prefix and removed
>> duplicate type string shortening etc
>> 
>> /Claes
>> 
>> On 2016-03-30 17:17, Peter Levart wrote:
>>> Hi Claes,
>>> 
>>> webrev.04 looks good now.
>>> 
>>> Maybe just one nit. For production quality, plugin input could be
>>> verified that after expansion it is composed of just the following
>>> characters: "LIJFD". Otherwise ClassWriter might generate an unusable
>>> class without complaining (for example if someone sneaks-in characters
>>> 'S' 'Z' 'B' or 'C')...
>>> 
>>> Or, better yet, create another method in BMH that will be the "public"
>>> API between the plugin and BMH which does the validation and calls
>>> generateConcreteBMHClassBytes(). Internally in BMH the validation is
>>> not necessary as the types strings are composed programmatically and
>>> are guaranteed to be correct.
>>> 
>>> Regards, Peter
>>> 
>>> On 03/30/2016 04:15 PM, Claes Redestad wrote:
 
 
 On 2016-03-30 14:21, Peter Levart wrote:
> Hi Claes,
> 
> On 03/30/2016 12:53 PM, Claes Redestad wrote:
>> Hi,
>> 
>> I think Class.forName(Module, String) seemed like a nice
>> efficiency/simplicity compromise, but there is some usage of
>> lambdas/method-refs in the Module lookup code, especially for
>> exploded modules (which get exercised during JDK build). Depending
>> on a lambda from code in java.lang.invoke means we fail to bootstrap.
>> 
>> But hey, we're living in an encapsulated world now, and this is in
>> java.base, so why not go directly to the BootLoader:
>> 
>> http://cr.openjdk.java.net/~redestad/8152641/webrev.03/
>> 
>> Since this is what's called from Class.forName(Module, String), the
>> overhead should be even smaller than in your classForNameInModule
>> test.
> 
> Good idea.
> 
>> 
>> If I call this final, will you say "Reviewed"? :-)
> 
> Sorry, I don't posses the powers. :-)
> 
> I could say "rEVIEWED", but...
> 
> In the plugin, the input is shortened speciesTypes strings. What
> guarantees that they really are normalized? For example, If one
> specifies "LLL" as input, it will get expanded into "LLL", the
> generated class will have "_L3" as a name suffix, but you will pack
> it in the image with "_LLL.class" filename suffix.
> 
> That's another reason why a method in BoundMethodHandle$Factory with
> the following signature:
> 
> Map.Entry generateConcreteBMHClassBytes(String types);
> 
> ...would be a good idea. It would return class bytes and the name of
> the class which you could use to derive the class filename without
> hardcoding the same logic in plugin and in BMH.
> 
> You just move the "LambdaForm.shortenSignature(types)" from
> getConcreteBMHClass and className/sourceFile calculation from
> generateConcreteBMHClass down to generateConcreteBMHClassBytes
> method and change the signatures...
 
 Yes, it makes sense to keep control over the class name inside the
 factory class, and this does allow specifying shortened or expanded
 forms (L3 vs LLL) interchangeably as input to the plugin, which
 reduces possibility for user errors.
 
 How about this: 

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

[Claes Redestad]

Hi Peter,

something like this, then:

http://cr.openjdk.java.net/~redestad/8152641/webrev.05/

- Added a method only used by the plugin which validates input; added a 
comment about the dependency
- Invalid types are logged but otherwise ignored now (I bet someone 
might suggest a better way to handle user errors)
- Some cleanup, introduced constant for class name prefix and removed 
duplicate type string shortening etc


/Claes

On 2016-03-30 17:17, Peter Levart wrote:

Hi Claes,

webrev.04 looks good now.

Maybe just one nit. For production quality, plugin input could be 
verified that after expansion it is composed of just the following 
characters: "LIJFD". Otherwise ClassWriter might generate an unusable 
class without complaining (for example if someone sneaks-in characters 
'S' 'Z' 'B' or 'C')...


Or, better yet, create another method in BMH that will be the "public" 
API between the plugin and BMH which does the validation and calls 
generateConcreteBMHClassBytes(). Internally in BMH the validation is 
not necessary as the types strings are composed programmatically and 
are guaranteed to be correct.


Regards, Peter

On 03/30/2016 04:15 PM, Claes Redestad wrote:



On 2016-03-30 14:21, Peter Levart wrote:

Hi Claes,

On 03/30/2016 12:53 PM, Claes Redestad wrote:

Hi,

I think Class.forName(Module, String) seemed like a nice 
efficiency/simplicity compromise, but there is some usage of 
lambdas/method-refs in the Module lookup code, especially for 
exploded modules (which get exercised during JDK build). Depending 
on a lambda from code in java.lang.invoke means we fail to bootstrap.


But hey, we're living in an encapsulated world now, and this is in 
java.base, so why not go directly to the BootLoader:


http://cr.openjdk.java.net/~redestad/8152641/webrev.03/

Since this is what's called from Class.forName(Module, String), the 
overhead should be even smaller than in your classForNameInModule test.


Good idea.



If I call this final, will you say "Reviewed"? :-)


Sorry, I don't posses the powers. :-)

I could say "rEVIEWED", but...

In the plugin, the input is shortened speciesTypes strings. What 
guarantees that they really are normalized? For example, If one 
specifies "LLL" as input, it will get expanded into "LLL", the 
generated class will have "_L3" as a name suffix, but you will pack 
it in the image with "_LLL.class" filename suffix.


That's another reason why a method in BoundMethodHandle$Factory with 
the following signature:


Map.Entry generateConcreteBMHClassBytes(String types);

...would be a good idea. It would return class bytes and the name of 
the class which you could use to derive the class filename without 
hardcoding the same logic in plugin and in BMH.


You just move the "LambdaForm.shortenSignature(types)" from 
getConcreteBMHClass and className/sourceFile calculation from 
generateConcreteBMHClass down to generateConcreteBMHClassBytes 
method and change the signatures...


Yes, it makes sense to keep control over the class name inside the 
factory class, and this does allow specifying shortened or expanded 
forms (L3 vs LLL) interchangeably as input to the plugin, which 
reduces possibility for user errors.


How about this: http://cr.openjdk.java.net/~redestad/8152641/webrev.04/

/Claes



Regards, Peter



/Claes

PS: The default list of types is generated in a few adhoc tests not 
part of this patch. I'm considering proposing add support for 
generating this list at build time. Maybe a JEP called "Build 
system support for profile-guided optimizations", which could also 
handle https://bugs.openjdk.java.net/browse/JDK-8150044


On 2016-03-30 09:53, Peter Levart wrote:

Hi Claes,

Sorry, I found a flaw in the benchmark (the regex pattern to split 
comma-separated string into substrings was wrong). What the 
benchmark did was compare the overheads of a single lookup of a 
longer class name containing commas. Here's the corrected result 
of overheads of 5 unsuccessful lookups:


Benchmark (generate)   (lookup)  Mode  Cnt 
Score Error  Units
ClassForNameBench.classForName LL,LLL,,L,LL  
LLI,LLLI,I,LI,LLI avgt   10  29627.141 ± 567.427  ns/op
ClassForNameBench.classForNameInModule LL,LLL,,L,LL  
LLI,LLLI,I,LI,LLI avgt   10   1073.256 ±  23.794  ns/op
ClassForNameBench.hashSetContains LL,LLL,,L,LL  
LLI,LLLI,I,LI,LLI avgt   10 33.022 ±   0.066  ns/op
ClassForNameBench.switchStatement LL,LLL,,L,LL  
LLI,LLLI,I,LI,LLI avgt   10 38.498 ±   5.842  ns/op


...overheads are a little bigger (x5 approx.).


Here's the corrected benchmark:


package jdk.test;

import org.openjdk.jmh.annotations.*;
import org.openjdk.jmh.infra.Blackhole;

import java.io.Serializable;
import java.lang.invoke.MethodHandle;
import java.util.Iterator;
import java.util.Set;
import java.util.concurrent.TimeUnit;
import java.util.stream.Collectors;
import 

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

[Peter Levart]

Hi Claes,

webrev.04 looks good now.

Maybe just one nit. For production quality, plugin input could be 
verified that after expansion it is composed of just the following 
characters: "LIJFD". Otherwise ClassWriter might generate an unusable 
class without complaining (for example if someone sneaks-in characters 
'S' 'Z' 'B' or 'C')...


Or, better yet, create another method in BMH that will be the "public" 
API between the plugin and BMH which does the validation and calls 
generateConcreteBMHClassBytes(). Internally in BMH the validation is not 
necessary as the types strings are composed programmatically and are 
guaranteed to be correct.


Regards, Peter

On 03/30/2016 04:15 PM, Claes Redestad wrote:



On 2016-03-30 14:21, Peter Levart wrote:

Hi Claes,

On 03/30/2016 12:53 PM, Claes Redestad wrote:

Hi,

I think Class.forName(Module, String) seemed like a nice 
efficiency/simplicity compromise, but there is some usage of 
lambdas/method-refs in the Module lookup code, especially for 
exploded modules (which get exercised during JDK build). Depending 
on a lambda from code in java.lang.invoke means we fail to bootstrap.


But hey, we're living in an encapsulated world now, and this is in 
java.base, so why not go directly to the BootLoader:


http://cr.openjdk.java.net/~redestad/8152641/webrev.03/

Since this is what's called from Class.forName(Module, String), the 
overhead should be even smaller than in your classForNameInModule test.


Good idea.



If I call this final, will you say "Reviewed"? :-)


Sorry, I don't posses the powers. :-)

I could say "rEVIEWED", but...

In the plugin, the input is shortened speciesTypes strings. What 
guarantees that they really are normalized? For example, If one 
specifies "LLL" as input, it will get expanded into "LLL", the 
generated class will have "_L3" as a name suffix, but you will pack 
it in the image with "_LLL.class" filename suffix.


That's another reason why a method in BoundMethodHandle$Factory with 
the following signature:


Map.Entry generateConcreteBMHClassBytes(String types);

...would be a good idea. It would return class bytes and the name of 
the class which you could use to derive the class filename without 
hardcoding the same logic in plugin and in BMH.


You just move the "LambdaForm.shortenSignature(types)" from 
getConcreteBMHClass and className/sourceFile calculation from 
generateConcreteBMHClass down to generateConcreteBMHClassBytes method 
and change the signatures...


Yes, it makes sense to keep control over the class name inside the 
factory class, and this does allow specifying shortened or expanded 
forms (L3 vs LLL) interchangeably as input to the plugin, which 
reduces possibility for user errors.


How about this: http://cr.openjdk.java.net/~redestad/8152641/webrev.04/

/Claes



Regards, Peter



/Claes

PS: The default list of types is generated in a few adhoc tests not 
part of this patch. I'm considering proposing add support for 
generating this list at build time. Maybe a JEP called "Build system 
support for profile-guided optimizations", which could also handle 
https://bugs.openjdk.java.net/browse/JDK-8150044


On 2016-03-30 09:53, Peter Levart wrote:

Hi Claes,

Sorry, I found a flaw in the benchmark (the regex pattern to split 
comma-separated string into substrings was wrong). What the 
benchmark did was compare the overheads of a single lookup of a 
longer class name containing commas. Here's the corrected result of 
overheads of 5 unsuccessful lookups:


Benchmark (generate)   (lookup)  Mode  Cnt 
Score Error  Units
ClassForNameBench.classForName LL,LLL,,L,LL  
LLI,LLLI,I,LI,LLI avgt   10  29627.141 ± 567.427  ns/op
ClassForNameBench.classForNameInModule LL,LLL,,L,LL  
LLI,LLLI,I,LI,LLI avgt   10   1073.256 ±  23.794  ns/op
ClassForNameBench.hashSetContains LL,LLL,,L,LL  
LLI,LLLI,I,LI,LLI avgt   10 33.022 ±   0.066  ns/op
ClassForNameBench.switchStatement LL,LLL,,L,LL  
LLI,LLLI,I,LI,LLI avgt   10 38.498 ±   5.842  ns/op


...overheads are a little bigger (x5 approx.).


Here's the corrected benchmark:


package jdk.test;

import org.openjdk.jmh.annotations.*;
import org.openjdk.jmh.infra.Blackhole;

import java.io.Serializable;
import java.lang.invoke.MethodHandle;
import java.util.Iterator;
import java.util.Set;
import java.util.concurrent.TimeUnit;
import java.util.stream.Collectors;
import java.util.stream.Stream;

@BenchmarkMode(Mode.AverageTime)
@Fork(value = 1, warmups = 0)
@Warmup(iterations = 10)
@Measurement(iterations = 10)
@OutputTimeUnit(TimeUnit.NANOSECONDS)
@State(Scope.Thread)
public class ClassForNameBench {

static final String BMH = "java/lang/invoke/BoundMethodHandle";
static final String SPECIES_PREFIX_NAME = "Species_";
static final String SPECIES_PREFIX_PATH = BMH + "$" + 
SPECIES_PREFIX_NAME;

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

[Claes Redestad]

On 2016-03-30 14:21, Peter Levart wrote:

Hi Claes,

On 03/30/2016 12:53 PM, Claes Redestad wrote:

Hi,

I think Class.forName(Module, String) seemed like a nice 
efficiency/simplicity compromise, but there is some usage of 
lambdas/method-refs in the Module lookup code, especially for 
exploded modules (which get exercised during JDK build). Depending on 
a lambda from code in java.lang.invoke means we fail to bootstrap.


But hey, we're living in an encapsulated world now, and this is in 
java.base, so why not go directly to the BootLoader:


http://cr.openjdk.java.net/~redestad/8152641/webrev.03/

Since this is what's called from Class.forName(Module, String), the 
overhead should be even smaller than in your classForNameInModule test.


Good idea.



If I call this final, will you say "Reviewed"? :-)


Sorry, I don't posses the powers. :-)

I could say "rEVIEWED", but...

In the plugin, the input is shortened speciesTypes strings. What 
guarantees that they really are normalized? For example, If one 
specifies "LLL" as input, it will get expanded into "LLL", the 
generated class will have "_L3" as a name suffix, but you will pack it 
in the image with "_LLL.class" filename suffix.


That's another reason why a method in BoundMethodHandle$Factory with 
the following signature:


Map.Entry generateConcreteBMHClassBytes(String types);

...would be a good idea. It would return class bytes and the name of 
the class which you could use to derive the class filename without 
hardcoding the same logic in plugin and in BMH.


You just move the "LambdaForm.shortenSignature(types)" from 
getConcreteBMHClass and className/sourceFile calculation from 
generateConcreteBMHClass down to generateConcreteBMHClassBytes method 
and change the signatures...


Yes, it makes sense to keep control over the class name inside the 
factory class, and this does allow specifying shortened or expanded 
forms (L3 vs LLL) interchangeably as input to the plugin, which reduces 
possibility for user errors.


How about this: http://cr.openjdk.java.net/~redestad/8152641/webrev.04/

/Claes



Regards, Peter



/Claes

PS: The default list of types is generated in a few adhoc tests not 
part of this patch. I'm considering proposing add support for 
generating this list at build time. Maybe a JEP called "Build system 
support for profile-guided optimizations", which could also handle 
https://bugs.openjdk.java.net/browse/JDK-8150044


On 2016-03-30 09:53, Peter Levart wrote:

Hi Claes,

Sorry, I found a flaw in the benchmark (the regex pattern to split 
comma-separated string into substrings was wrong). What the 
benchmark did was compare the overheads of a single lookup of a 
longer class name containing commas. Here's the corrected result of 
overheads of 5 unsuccessful lookups:


Benchmark (generate)   (lookup)  Mode  Cnt 
Score Error  Units
ClassForNameBench.classForName LL,LLL,,L,LL  
LLI,LLLI,I,LI,LLI avgt   10  29627.141 ± 567.427  ns/op
ClassForNameBench.classForNameInModule LL,LLL,,L,LL  
LLI,LLLI,I,LI,LLI avgt   10   1073.256 ±  23.794  ns/op
ClassForNameBench.hashSetContains LL,LLL,,L,LL  
LLI,LLLI,I,LI,LLI avgt   10 33.022 ±   0.066  ns/op
ClassForNameBench.switchStatement LL,LLL,,L,LL  
LLI,LLLI,I,LI,LLI avgt   10 38.498 ±   5.842  ns/op


...overheads are a little bigger (x5 approx.).


Here's the corrected benchmark:


package jdk.test;

import org.openjdk.jmh.annotations.*;
import org.openjdk.jmh.infra.Blackhole;

import java.io.Serializable;
import java.lang.invoke.MethodHandle;
import java.util.Iterator;
import java.util.Set;
import java.util.concurrent.TimeUnit;
import java.util.stream.Collectors;
import java.util.stream.Stream;

@BenchmarkMode(Mode.AverageTime)
@Fork(value = 1, warmups = 0)
@Warmup(iterations = 10)
@Measurement(iterations = 10)
@OutputTimeUnit(TimeUnit.NANOSECONDS)
@State(Scope.Thread)
public class ClassForNameBench {

static final String BMH = "java/lang/invoke/BoundMethodHandle";
static final String SPECIES_PREFIX_NAME = "Species_";
static final String SPECIES_PREFIX_PATH = BMH + "$" + 
SPECIES_PREFIX_NAME;


@Param({"LL,LLL,,L,LL"})
public String generate;

@Param({"LLI,LLLI,I,LI,LLI"})
public String lookup;

private String[] generatedTypes;
private String[] lookedUpTypes;
private Set generatedNames;
private String[] lookedUpNames;

@Setup(Level.Trial)
public void setup() {
generatedTypes = generate.trim().split("\\s*,\\s*");
lookedUpTypes = lookup.trim().split("\\s*,\\s*");
generatedNames = Stream.of(generatedTypes)
   .map(types -> SPECIES_PREFIX_PATH + 
shortenSignature(types))

   .collect(Collectors.toSet());
lookedUpNames = Stream.of(lookedUpTypes)
  .map(types 

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

[Peter Levart]

Hi Claes,

On 03/30/2016 12:53 PM, Claes Redestad wrote:

Hi,

I think Class.forName(Module, String) seemed like a nice 
efficiency/simplicity compromise, but there is some usage of 
lambdas/method-refs in the Module lookup code, especially for exploded 
modules (which get exercised during JDK build). Depending on a lambda 
from code in java.lang.invoke means we fail to bootstrap.


But hey, we're living in an encapsulated world now, and this is in 
java.base, so why not go directly to the BootLoader:


http://cr.openjdk.java.net/~redestad/8152641/webrev.03/

Since this is what's called from Class.forName(Module, String), the 
overhead should be even smaller than in your classForNameInModule test.


Good idea.



If I call this final, will you say "Reviewed"? :-)


Sorry, I don't posses the powers. :-)

I could say "rEVIEWED", but...

In the plugin, the input is shortened speciesTypes strings. What 
guarantees that they really are normalized? For example, If one 
specifies "LLL" as input, it will get expanded into "LLL", the generated 
class will have "_L3" as a name suffix, but you will pack it in the 
image with "_LLL.class" filename suffix.


That's another reason why a method in BoundMethodHandle$Factory with the 
following signature:


Map.Entry generateConcreteBMHClassBytes(String types);

...would be a good idea. It would return class bytes and the name of the 
class which you could use to derive the class filename without 
hardcoding the same logic in plugin and in BMH.


You just move the "LambdaForm.shortenSignature(types)" from 
getConcreteBMHClass and className/sourceFile calculation from 
generateConcreteBMHClass down to generateConcreteBMHClassBytes method 
and change the signatures...


Regards, Peter



/Claes

PS: The default list of types is generated in a few adhoc tests not 
part of this patch. I'm considering proposing add support for 
generating this list at build time. Maybe a JEP called "Build system 
support for profile-guided optimizations", which could also handle 
https://bugs.openjdk.java.net/browse/JDK-8150044


On 2016-03-30 09:53, Peter Levart wrote:

Hi Claes,

Sorry, I found a flaw in the benchmark (the regex pattern to split 
comma-separated string into substrings was wrong). What the benchmark 
did was compare the overheads of a single lookup of a longer class 
name containing commas. Here's the corrected result of overheads of 5 
unsuccessful lookups:


Benchmark (generate)   (lookup)  Mode  Cnt 
Score Error  Units
ClassForNameBench.classForName LL,LLL,,L,LL  
LLI,LLLI,I,LI,LLI  avgt 10  29627.141 ± 567.427  ns/op
ClassForNameBench.classForNameInModule LL,LLL,,L,LL  
LLI,LLLI,I,LI,LLI  avgt 10   1073.256 ±  23.794  ns/op
ClassForNameBench.hashSetContains LL,LLL,,L,LL  
LLI,LLLI,I,LI,LLI  avgt 10 33.022 ±   0.066  ns/op
ClassForNameBench.switchStatement LL,LLL,,L,LL  
LLI,LLLI,I,LI,LLI  avgt 10 38.498 ±   5.842  ns/op


...overheads are a little bigger (x5 approx.).


Here's the corrected benchmark:


package jdk.test;

import org.openjdk.jmh.annotations.*;
import org.openjdk.jmh.infra.Blackhole;

import java.io.Serializable;
import java.lang.invoke.MethodHandle;
import java.util.Iterator;
import java.util.Set;
import java.util.concurrent.TimeUnit;
import java.util.stream.Collectors;
import java.util.stream.Stream;

@BenchmarkMode(Mode.AverageTime)
@Fork(value = 1, warmups = 0)
@Warmup(iterations = 10)
@Measurement(iterations = 10)
@OutputTimeUnit(TimeUnit.NANOSECONDS)
@State(Scope.Thread)
public class ClassForNameBench {

static final String BMH = "java/lang/invoke/BoundMethodHandle";
static final String SPECIES_PREFIX_NAME = "Species_";
static final String SPECIES_PREFIX_PATH = BMH + "$" + 
SPECIES_PREFIX_NAME;


@Param({"LL,LLL,,L,LL"})
public String generate;

@Param({"LLI,LLLI,I,LI,LLI"})
public String lookup;

private String[] generatedTypes;
private String[] lookedUpTypes;
private Set generatedNames;
private String[] lookedUpNames;

@Setup(Level.Trial)
public void setup() {
generatedTypes = generate.trim().split("\\s*,\\s*");
lookedUpTypes = lookup.trim().split("\\s*,\\s*");
generatedNames = Stream.of(generatedTypes)
   .map(types -> SPECIES_PREFIX_PATH + 
shortenSignature(types))

   .collect(Collectors.toSet());
lookedUpNames = Stream.of(lookedUpTypes)
  .map(types -> SPECIES_PREFIX_PATH + 
shortenSignature(types))

  .toArray(String[]::new);
}

@Benchmark
public void classForName(Blackhole bh) {
for (String name : lookedUpNames) {
try {
bh.consume(Class.forName(name, false, 
MethodHandle.class.getClassLoader()));

} catch 

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

[Claes Redestad]

Hi,

On 2016-03-30 13:11, Remi Forax wrote:

Hi Claes,
Did you considere to use
   return c.asSubclass(BoundMethodHandle.class);
instead of
   return (Class)c;
to avoid the @SupressWarnings, like in generateConcreteBMHClass ?


no, but it's a good cleanup. :-)

Updated in-place: http://cr.openjdk.java.net/~redestad/8152641/webrev.03/

/Claes

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

[Remi Forax]

Hi Claes,
Did you considere to use
  return c.asSubclass(BoundMethodHandle.class);
instead of
  return (Class)c;
to avoid the @SupressWarnings, like in generateConcreteBMHClass ?

cheers,
Rémi

- Mail original -
> De: "Claes Redestad" <claes.redes...@oracle.com>
> À: "Peter Levart" <peter.lev...@gmail.com>, "Mandy Chung" 
> <mandy.ch...@oracle.com>
> Cc: "jigsaw-dev" <jigsaw-dev@openjdk.java.net>, "core-libs-dev" 
> <core-libs-...@openjdk.java.net>
> Envoyé: Mercredi 30 Mars 2016 12:53:08
> Objet: Re: RFR: 8152641: Plugin to generate BMH$Species classes ahead-of-time
> 
> Hi,
> 
> I think Class.forName(Module, String) seemed like a nice
> efficiency/simplicity compromise, but there is some usage of
> lambdas/method-refs in the Module lookup code, especially for exploded
> modules (which get exercised during JDK build). Depending on a lambda
> from code in java.lang.invoke means we fail to bootstrap.
> 
> But hey, we're living in an encapsulated world now, and this is in
> java.base, so why not go directly to the BootLoader:
> 
> http://cr.openjdk.java.net/~redestad/8152641/webrev.03/
> 
> Since this is what's called from Class.forName(Module, String), the
> overhead should be even smaller than in your classForNameInModule test.
> 
> If I call this final, will you say "Reviewed"? :-)
> 
> /Claes
> 
> PS: The default list of types is generated in a few adhoc tests not part
> of this patch. I'm considering proposing add support for generating this
> list at build time. Maybe a JEP called "Build system support for
> profile-guided optimizations", which could also handle
> https://bugs.openjdk.java.net/browse/JDK-8150044
> 
> On 2016-03-30 09:53, Peter Levart wrote:
> > Hi Claes,
> >
> > Sorry, I found a flaw in the benchmark (the regex pattern to split
> > comma-separated string into substrings was wrong). What the benchmark
> > did was compare the overheads of a single lookup of a longer class
> > name containing commas. Here's the corrected result of overheads of 5
> > unsuccessful lookups:
> >
> > Benchmark (generate)   (lookup)  Mode  Cnt
> > Score Error  Units
> > ClassForNameBench.classForName  LL,LLL,,L,LL
> > LLI,LLLI,I,LI,LLI  avgt   10  29627.141 ± 567.427 ns/op
> > ClassForNameBench.classForNameInModule  LL,LLL,,L,LL
> > LLI,LLLI,I,LI,LLI  avgt   10   1073.256 ±  23.794 ns/op
> > ClassForNameBench.hashSetContains   LL,LLL,,L,LL
> > LLI,LLLI,I,LI,LLI  avgt   10 33.022 ±   0.066 ns/op
> > ClassForNameBench.switchStatement   LL,LLL,,L,LL
> > LLI,LLLI,I,LI,LLI  avgt   10 38.498 ±   5.842 ns/op
> >
> > ...overheads are a little bigger (x5 approx.).
> >
> >
> > Here's the corrected benchmark:
> >
> >
> > package jdk.test;
> >
> > import org.openjdk.jmh.annotations.*;
> > import org.openjdk.jmh.infra.Blackhole;
> >
> > import java.io.Serializable;
> > import java.lang.invoke.MethodHandle;
> > import java.util.Iterator;
> > import java.util.Set;
> > import java.util.concurrent.TimeUnit;
> > import java.util.stream.Collectors;
> > import java.util.stream.Stream;
> >
> > @BenchmarkMode(Mode.AverageTime)
> > @Fork(value = 1, warmups = 0)
> > @Warmup(iterations = 10)
> > @Measurement(iterations = 10)
> > @OutputTimeUnit(TimeUnit.NANOSECONDS)
> > @State(Scope.Thread)
> > public class ClassForNameBench {
> >
> > static final String BMH = "java/lang/invoke/BoundMethodHandle";
> > static final String SPECIES_PREFIX_NAME = "Species_";
> > static final String SPECIES_PREFIX_PATH = BMH + "$" +
> > SPECIES_PREFIX_NAME;
> >
> > @Param({"LL,LLL,,L,LL"})
> > public String generate;
> >
> > @Param({"LLI,LLLI,I,LI,LLI"})
> > public String lookup;
> >
> > private String[] generatedTypes;
> > private String[] lookedUpTypes;
> > private Set generatedNames;
> > private String[] lookedUpNames;
> >
> > @Setup(Level.Trial)
> > public void setup() {
> > generatedTypes = generate.trim().split("\\s*,\\s*");
> > lookedUpTypes = lookup.trim().split("\\s*,\\s*");
> > generatedNames = Stream.of(generatedTypes)
> >.map(types -> SPECIES_PREFIX_PATH +
> > shortenSignatu

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

[Claes Redestad]

Hi,

I think Class.forName(Module, String) seemed like a nice 
efficiency/simplicity compromise, but there is some usage of 
lambdas/method-refs in the Module lookup code, especially for exploded 
modules (which get exercised during JDK build). Depending on a lambda 
from code in java.lang.invoke means we fail to bootstrap.


But hey, we're living in an encapsulated world now, and this is in 
java.base, so why not go directly to the BootLoader:


http://cr.openjdk.java.net/~redestad/8152641/webrev.03/

Since this is what's called from Class.forName(Module, String), the 
overhead should be even smaller than in your classForNameInModule test.


If I call this final, will you say "Reviewed"? :-)

/Claes

PS: The default list of types is generated in a few adhoc tests not part 
of this patch. I'm considering proposing add support for generating this 
list at build time. Maybe a JEP called "Build system support for 
profile-guided optimizations", which could also handle 
https://bugs.openjdk.java.net/browse/JDK-8150044


On 2016-03-30 09:53, Peter Levart wrote:

Hi Claes,

Sorry, I found a flaw in the benchmark (the regex pattern to split 
comma-separated string into substrings was wrong). What the benchmark 
did was compare the overheads of a single lookup of a longer class 
name containing commas. Here's the corrected result of overheads of 5 
unsuccessful lookups:


Benchmark (generate)   (lookup)  Mode  Cnt 
Score Error  Units
ClassForNameBench.classForName  LL,LLL,,L,LL 
LLI,LLLI,I,LI,LLI  avgt   10  29627.141 ± 567.427 ns/op
ClassForNameBench.classForNameInModule  LL,LLL,,L,LL 
LLI,LLLI,I,LI,LLI  avgt   10   1073.256 ±  23.794 ns/op
ClassForNameBench.hashSetContains   LL,LLL,,L,LL 
LLI,LLLI,I,LI,LLI  avgt   10 33.022 ±   0.066 ns/op
ClassForNameBench.switchStatement   LL,LLL,,L,LL 
LLI,LLLI,I,LI,LLI  avgt   10 38.498 ±   5.842 ns/op


...overheads are a little bigger (x5 approx.).


Here's the corrected benchmark:


package jdk.test;

import org.openjdk.jmh.annotations.*;
import org.openjdk.jmh.infra.Blackhole;

import java.io.Serializable;
import java.lang.invoke.MethodHandle;
import java.util.Iterator;
import java.util.Set;
import java.util.concurrent.TimeUnit;
import java.util.stream.Collectors;
import java.util.stream.Stream;

@BenchmarkMode(Mode.AverageTime)
@Fork(value = 1, warmups = 0)
@Warmup(iterations = 10)
@Measurement(iterations = 10)
@OutputTimeUnit(TimeUnit.NANOSECONDS)
@State(Scope.Thread)
public class ClassForNameBench {

static final String BMH = "java/lang/invoke/BoundMethodHandle";
static final String SPECIES_PREFIX_NAME = "Species_";
static final String SPECIES_PREFIX_PATH = BMH + "$" + 
SPECIES_PREFIX_NAME;


@Param({"LL,LLL,,L,LL"})
public String generate;

@Param({"LLI,LLLI,I,LI,LLI"})
public String lookup;

private String[] generatedTypes;
private String[] lookedUpTypes;
private Set generatedNames;
private String[] lookedUpNames;

@Setup(Level.Trial)
public void setup() {
generatedTypes = generate.trim().split("\\s*,\\s*");
lookedUpTypes = lookup.trim().split("\\s*,\\s*");
generatedNames = Stream.of(generatedTypes)
   .map(types -> SPECIES_PREFIX_PATH + 
shortenSignature(types))

   .collect(Collectors.toSet());
lookedUpNames = Stream.of(lookedUpTypes)
  .map(types -> SPECIES_PREFIX_PATH + 
shortenSignature(types))

  .toArray(String[]::new);
}

@Benchmark
public void classForName(Blackhole bh) {
for (String name : lookedUpNames) {
try {
bh.consume(Class.forName(name, false, 
MethodHandle.class.getClassLoader()));

} catch (ClassNotFoundException e) {
bh.consume(e);
}
}
}

@Benchmark
public void classForNameInModule(Blackhole bh) {
for (String name : lookedUpNames) {
bh.consume(Class.forName(MethodHandle.class.getModule(), name));
}
}

@Benchmark
public void hashSetContains(Blackhole bh) {
for (String name : lookedUpNames) {
bh.consume(generatedNames.contains(name));
}
}

@Benchmark
public void switchStatement(Blackhole bh) {
for (String types : lookedUpTypes) {
bh.consume(getBMHSwitch(types));
}
}

static Class getBMHSwitch(String types) {
// should be in sync with @Param generate above...
switch (types) {
case "LL": return Object.class;
case "LLL": return Serializable.class;
case "": return Iterator.class;
case "L": return Throwable.class;
case "LL": return String.class;
default: return null;
}
}

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

[Peter Levart]

Hi Claes,

Sorry, I found a flaw in the benchmark (the regex pattern to split 
comma-separated string into substrings was wrong). What the benchmark 
did was compare the overheads of a single lookup of a longer class name 
containing commas. Here's the corrected result of overheads of 5 
unsuccessful lookups:


Benchmark (generate)   (lookup)  Mode  Cnt  
Score Error  Units
ClassForNameBench.classForName  LL,LLL,,L,LL 
LLI,LLLI,I,LI,LLI  avgt   10  29627.141 ± 567.427  ns/op
ClassForNameBench.classForNameInModule  LL,LLL,,L,LL 
LLI,LLLI,I,LI,LLI  avgt   10   1073.256 ±  23.794  ns/op
ClassForNameBench.hashSetContains   LL,LLL,,L,LL 
LLI,LLLI,I,LI,LLI  avgt   10 33.022 ±   0.066  ns/op
ClassForNameBench.switchStatement   LL,LLL,,L,LL 
LLI,LLLI,I,LI,LLI  avgt   10 38.498 ±   5.842  ns/op


...overheads are a little bigger (x5 approx.).


Here's the corrected benchmark:


package jdk.test;

import org.openjdk.jmh.annotations.*;
import org.openjdk.jmh.infra.Blackhole;

import java.io.Serializable;
import java.lang.invoke.MethodHandle;
import java.util.Iterator;
import java.util.Set;
import java.util.concurrent.TimeUnit;
import java.util.stream.Collectors;
import java.util.stream.Stream;

@BenchmarkMode(Mode.AverageTime)
@Fork(value = 1, warmups = 0)
@Warmup(iterations = 10)
@Measurement(iterations = 10)
@OutputTimeUnit(TimeUnit.NANOSECONDS)
@State(Scope.Thread)
public class ClassForNameBench {

static final String BMH = "java/lang/invoke/BoundMethodHandle";
static final String SPECIES_PREFIX_NAME = "Species_";
static final String SPECIES_PREFIX_PATH = BMH + "$" + 
SPECIES_PREFIX_NAME;


@Param({"LL,LLL,,L,LL"})
public String generate;

@Param({"LLI,LLLI,I,LI,LLI"})
public String lookup;

private String[] generatedTypes;
private String[] lookedUpTypes;
private Set generatedNames;
private String[] lookedUpNames;

@Setup(Level.Trial)
public void setup() {
generatedTypes = generate.trim().split("\\s*,\\s*");
lookedUpTypes = lookup.trim().split("\\s*,\\s*");
generatedNames = Stream.of(generatedTypes)
   .map(types -> SPECIES_PREFIX_PATH + 
shortenSignature(types))

   .collect(Collectors.toSet());
lookedUpNames = Stream.of(lookedUpTypes)
  .map(types -> SPECIES_PREFIX_PATH + 
shortenSignature(types))

  .toArray(String[]::new);
}

@Benchmark
public void classForName(Blackhole bh) {
for (String name : lookedUpNames) {
try {
bh.consume(Class.forName(name, false, 
MethodHandle.class.getClassLoader()));

} catch (ClassNotFoundException e) {
bh.consume(e);
}
}
}

@Benchmark
public void classForNameInModule(Blackhole bh) {
for (String name : lookedUpNames) {
bh.consume(Class.forName(MethodHandle.class.getModule(), 
name));

}
}

@Benchmark
public void hashSetContains(Blackhole bh) {
for (String name : lookedUpNames) {
bh.consume(generatedNames.contains(name));
}
}

@Benchmark
public void switchStatement(Blackhole bh) {
for (String types : lookedUpTypes) {
bh.consume(getBMHSwitch(types));
}
}

static Class getBMHSwitch(String types) {
// should be in sync with @Param generate above...
switch (types) {
case "LL": return Object.class;
case "LLL": return Serializable.class;
case "": return Iterator.class;
case "L": return Throwable.class;
case "LL": return String.class;
default: return null;
}
}

// copied from non-public LambdaForm
static String shortenSignature(String signature) {
// Hack to make signatures more readable when they show up in 
method names.

final int NO_CHAR = -1, MIN_RUN = 3;
int c0, c1 = NO_CHAR, c1reps = 0;
StringBuilder buf = null;
int len = signature.length();
if (len < MIN_RUN) return signature;
for (int i = 0; i <= len; i++) {
// shift in the next char:
c0 = c1; c1 = (i == len ? NO_CHAR : signature.charAt(i));
if (c1 == c0) { ++c1reps; continue; }
// shift in the next count:
int c0reps = c1reps; c1reps = 1;
// end of a  character run
if (c0reps < MIN_RUN) {
if (buf != null) {
while (--c0reps >= 0)
buf.append((char) c0);
}
continue;
}
// found three or more in a row
if (buf == null)
buf = new StringBuilder().append(signature, 

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

[Peter Levart]

Hi Claes,

On 03/30/2016 01:03 AM, Claes Redestad wrote:

Hi Peter, Mandy,

On 2016-03-26 12:47, Peter Levart wrote:


Comparing this with proposed code from webrev:

 493 try {
 494 return (ClassBoundMethodHandle>)
 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


to have something to compare with I built a version which, like you 
suggest,

generates a HashSet with the set of generated classes here:

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

This adds a fair bit of complexity to the plugin and requires we add a 
nested

class in BoundMethodHandle that we can replace. Using the anonymous
compute Function for this seems like the best choice for this.


...what I had in mind as alternative to a pregenerated class with a 
switch was a simple textual resource file, generated by plugin, read-in 
by BMH into a HashSet. No special-purpose class generation and much less 
complexity for the plugin.




I've not been able to measure any statistical difference in real 
startup terms,

though, and not figured out a smart way to benchmark the overhead of the
CNFE in relation to the class generation (my guess it adds a fraction 
to the
total cost) and since this adds ever so little footprint and an extra 
lookup to the

fast path it would seem that this is the wrong trade-off to do here.


Yes, perhaps it would be best to just use Class.forName(module, 
className) instead. I have created a little benchmark to compare 
overheads (just overheads) of unsuccessful lookups for pregenerated 
classes (a situation where a BMH class is requested that has not been 
pregenerated) and here's the result for overhead of 5 unsuccessfull lookups:


Benchmark (generate)   (lookup)  Mode  Cnt Score 
Error  Units
ClassForNameBench.classForName  LL,LLL,,L,LL 
LLI,LLLI,I,LI,LLI  avgt   10  6800.878 ± 421.424  ns/op
ClassForNameBench.classForNameInModule  LL,LLL,,L,LL 
LLI,LLLI,I,LI,LLI  avgt   10   209.574 ±   2.114  ns/op
ClassForNameBench.hashSetContains   LL,LLL,,L,LL 
LLI,LLLI,I,LI,LLI  avgt   10 6.813 ±   0.317  ns/op
ClassForNameBench.switchStatement   LL,LLL,,L,LL 
LLI,LLLI,I,LI,LLI  avgt   10 6.601 ±   0.061  ns/op


...compared to runtime BMH class generation and loading this is really a 
very minor overhead. I would just use Class.forName(module, className) 
and reduce the complexity of the plugin.


What do you think?


Here's the benchmark:

package jdk.test;

import org.openjdk.jmh.annotations.*;
import org.openjdk.jmh.infra.Blackhole;

import java.io.Serializable;
import java.lang.invoke.MethodHandle;
import java.util.Iterator;
import java.util.Set;
import java.util.concurrent.TimeUnit;
import java.util.stream.Collectors;
import java.util.stream.Stream;

@BenchmarkMode(Mode.AverageTime)
@Fork(value = 1, warmups = 0)
@Warmup(iterations = 10)
@Measurement(iterations = 10)
@OutputTimeUnit(TimeUnit.NANOSECONDS)
@State(Scope.Thread)
public class ClassForNameBench {

static final String BMH = "java/lang/invoke/BoundMethodHandle";
static final String SPECIES_PREFIX_NAME = "Species_";
static final String SPECIES_PREFIX_PATH = BMH + "$" + 
SPECIES_PREFIX_NAME;


@Param({"LL,LLL,,L,LL"})
public String generate;

@Param({"LLI,LLLI,I,LI,LLI"})
public String lookup;

private String[] generatedTypes;
private String[] lookedUpTypes;
private Set generatedNames;
private String[] lookedUpNames;

@Setup(Level.Trial)
public void setup() {
generatedTypes = generate.trim().split("\\s+,\\s+");
lookedUpTypes = lookup.trim().split("\\s+,\\s+");
generatedNames = Stream.of(generatedTypes)

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

[Claes Redestad]

Hi Peter, Mandy,

On 2016-03-26 12:47, Peter Levart wrote:


Comparing this with proposed code from webrev:

 493 try {
 494 return (ClassBoundMethodHandle>)
 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


to have something to compare with I built a version which, like you 
suggest,

generates a HashSet with the set of generated classes here:

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

This adds a fair bit of complexity to the plugin and requires we add a 
nested

class in BoundMethodHandle that we can replace. Using the anonymous
compute Function for this seems like the best choice for this.

I've not been able to measure any statistical difference in real startup 
terms,

though, and not figured out a smart way to benchmark the overhead of the
CNFE in relation to the class generation (my guess it adds a fraction to 
the
total cost) and since this adds ever so little footprint and an extra 
lookup to the

fast path it would seem that this is the wrong trade-off to do here.

All-in-all I lean towards moving forward with the first, simpler 
revision of this

patch[1] and then evaluate if webrev.02 or a String-switch+static resolve
as a follow-up.

A compromise would be to keep the SpeciesLookup class introduced here
to allow removing all overhead in case the plugin is disabled.

Mandy: I've not found any test (under jdk/test/tools/jlink or elsewhere) 
which

has to be updated when adding a plugin like this.

Thanks!

/Claes

[1] http://cr.openjdk.java.net/~redestad/8152641/webrev.01/

Re: 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 (ClassBoundMethodHandle>)
 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 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 
 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 

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

[Mandy Chung]

> On Mar 25, 2016, at 7:58 AM, 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);
> }
> 

This is exactly what I was thinking.

Mandy

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

[Claes Redestad]

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





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 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 
 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

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

[Peter Levart]

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.




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.

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 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?

Regards, Peter




On Mar 24, 2016, at 6:42 AM, Claes Redestad  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

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

[Claes Redestad]

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