RFR: 8175026: Capture build-time parameters to --generate-jli-classes

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR: 8175026: Capture build-time parameters to --generate-jli-classes

Claes Redestad
Hi,

currently the file we generate at build time as input to
--generate-jli-classes is lost when linking custom images, which means
user generate images may perform worse in certain ways, mostly
generating more classes during startup.

Additionally, there's a strong assumption in --generate-jli-classes that
the VM running jlink is going to generate compatible classes with the
image being linked, which we can only really guarantee if the java.base
in the linked image is of the same version as the java.base in the VM
running jlink.  This patch tightens these checks to ensure we have
freedom to evolve and re-evaluate the implementation in future
releases.

JDK: http://cr.openjdk.java.net/~redestad/8175026/jdk.01/
Top: http://cr.openjdk.java.net/~redestad/8175026/top.01/
Bug: https://bugs.openjdk.java.net/browse/JDK-8175026

Thanks!

/Claes
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8175026: Capture build-time parameters to --generate-jli-classes

Mandy Chung

> On Feb 15, 2017, at 9:12 AM, Claes Redestad <[hidden email]> wrote:
>
> Hi,
>
> currently the file we generate at build time as input to
> --generate-jli-classes is lost when linking custom images, which means
> user generate images may perform worse in certain ways, mostly
> generating more classes during startup.
>
> Additionally, there's a strong assumption in --generate-jli-classes that
> the VM running jlink is going to generate compatible classes with the
> image being linked, which we can only really guarantee if the java.base
> in the linked image is of the same version as the java.base in the VM
> running jlink.  This patch tightens these checks to ensure we have
> freedom to evolve and re-evaluate the implementation in future
> releases.
>
> JDK: http://cr.openjdk.java.net/~redestad/8175026/jdk.01/
> Top: http://cr.openjdk.java.net/~redestad/8175026/top.01/


This restriction sounds reasonable and we can enhance this in a future release. I think the plugin can record the configuration in its own format to be independent of the trace output format.

Instead of throwing ISA, you can have a test method to return a boolean to indicate if the default trace file should be read.

Instead of running java.base from the runtime, you can use Runtime.version() instead and I think comparing Version::major should be adequate.
This change disables this default setting entirely if the image being created is not the same version as this plugin (defaultSpecies and defaultInvokers, etc).  Is it intended?

In addition, if the main argument is specified but the version does not match, it will ignore the given argument.  Should it be an error instead?  We are the one who will generate a trace file and specify it in the jlink plugin option.  It’s okay to ignore the default trace output if no plugin option is specified and I think no warning should be printed in this case.  It’s just like this plugin is disabled.  You may want to add a suboption to turn on verbose that will trace what is generated and what is ignored.

Mandy



Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8175026: Capture build-time parameters to --generate-jli-classes

Magnus Ihse Bursie
In reply to this post by Claes Redestad
On 2017-02-15 18:12, Claes Redestad wrote:

> Hi,
>
> currently the file we generate at build time as input to
> --generate-jli-classes is lost when linking custom images, which means
> user generate images may perform worse in certain ways, mostly
> generating more classes during startup.
>
> Additionally, there's a strong assumption in --generate-jli-classes that
> the VM running jlink is going to generate compatible classes with the
> image being linked, which we can only really guarantee if the java.base
> in the linked image is of the same version as the java.base in the VM
> running jlink.  This patch tightens these checks to ensure we have
> freedom to evolve and re-evaluate the implementation in future
> releases.
>
> JDK: http://cr.openjdk.java.net/~redestad/8175026/jdk.01/
> Top: http://cr.openjdk.java.net/~redestad/8175026/top.01/
> Bug: https://bugs.openjdk.java.net/browse/JDK-8175026
>
> Thanks!

Build changes look good.

/Magnus
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8175026: Capture build-time parameters to --generate-jli-classes

Claes Redestad
In reply to this post by Mandy Chung
Hi Mandy,

On 02/15/2017 08:09 PM, Mandy Chung wrote:

>> On Feb 15, 2017, at 9:12 AM, Claes Redestad <[hidden email]> wrote:
>>
>> Hi,
>>
>> currently the file we generate at build time as input to
>> --generate-jli-classes is lost when linking custom images, which means
>> user generate images may perform worse in certain ways, mostly
>> generating more classes during startup.
>>
>> Additionally, there's a strong assumption in --generate-jli-classes that
>> the VM running jlink is going to generate compatible classes with the
>> image being linked, which we can only really guarantee if the java.base
>> in the linked image is of the same version as the java.base in the VM
>> running jlink.  This patch tightens these checks to ensure we have
>> freedom to evolve and re-evaluate the implementation in future
>> releases.
>>
>> JDK: http://cr.openjdk.java.net/~redestad/8175026/jdk.01/
>> Top: http://cr.openjdk.java.net/~redestad/8175026/top.01/
>
> This restriction sounds reasonable and we can enhance this in a future release. I think the plugin can record the configuration in its own format to be independent of the trace output format.
>
> Instead of throwing ISA, you can have a test method to return a boolean to indicate if the default trace file should be read.
>
> Instead of running java.base from the runtime, you can use Runtime.version() instead and I think comparing Version::major should be adequate.

Sure, I've changed to compare Runtime.Version.major and minor. While
minor differences should be fine, the plugin depends on internals that
are subject to change in minor
releases. To support experimentation and testing I've added a
ignore-version suboption to override this.

> This change disables this default setting entirely if the image being created is not the same version as this plugin (defaultSpecies and defaultInvokers, etc).  Is it intended?

Yes, this is intended, as we can't guarantee the code generated using
one version of jlink will be compatible with the next; the format of the
trace is also subject to change.

>
> In addition, if the main argument is specified but the version does not match, it will ignore the given argument.  Should it be an error instead?  We are the one who will generate a trace file and specify it in the jlink plugin option.  It’s okay to ignore the default trace output if no plugin option is specified and I think no warning should be printed in this case.  It’s just like this plugin is disabled.  You may want to add a suboption to turn on verbose that will trace what is generated and what is ignored.

I think a warning is reasonable in all cases: Using a different version
of jlink than the java.base you're linking will lose some optimizations
and the user would be none the wiser as to why, verbosity helps avoid
surprises.

http://cr.openjdk.java.net/~redestad/8175026/jdk.02/

More verbosity control would be great, but that seems like a good fit
for a JDK 10 RFE.

Thanks!

/Claes

>
> Mandy
>
>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8175026: Capture build-time parameters to --generate-jli-classes

Mandy Chung

> On Feb 16, 2017, at 5:20 AM, Claes Redestad <[hidden email]> wrote:
>
>>
>> In addition, if the main argument is specified but the version does not match, it will ignore the given argument.  Should it be an error instead?  We are the one who will generate a trace file and specify it in the jlink plugin option.  It’s okay to ignore the default trace output if no plugin option is specified and I think no warning should be printed in this case.  It’s just like this plugin is disabled.  You may want to add a suboption to turn on verbose that will trace what is generated and what is ignored.
>
> I think a warning is reasonable in all cases: Using a different version of jlink than the java.base you're linking will lose some optimizations and the user would be none the wiser as to why, verbosity helps avoid surprises.

The plugin is enabled by default.  With this change, I consider
this plugin is "auto-enabled" when it’s creating the image of
the version that this plugin supports (i.e. matching major.minor
version).

So if the —generate-jli-classes option is not specified, it might
be confusing when I get this warning.  I would prefer in this case
no warning should be emitted and the plugin is not enabled.

If the option is specified on the command-line, it should emit
the warning.

>
> http://cr.openjdk.java.net/~redestad/8175026/jdk.02/

 322         if (!initialize(in)) {

Maybe refactor line 175-190 in a new method and something like this:
   if (!checkVersion(getLinkedVersion(in)))
       :
   }
   
Then follow with initialize(in) here.  That’d make it explicit.
One thing to handle is when exception is thrown when reading
the trace file (default or mainArgument). Maybe that part can
be done early in configure method and store the lines for later
consumption.

line 235-238: you may use orThrow in this case.

Mandy
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8175026: Capture build-time parameters to --generate-jli-classes

Mandy Chung
One more thing: the warning message should be in plugin.properties
to be localized.

Mandy

> On Feb 16, 2017, at 12:04 PM, Mandy Chung <[hidden email]> wrote:
>
>
>> On Feb 16, 2017, at 5:20 AM, Claes Redestad <[hidden email]> wrote:
>>
>>>
>>> In addition, if the main argument is specified but the version does not match, it will ignore the given argument.  Should it be an error instead?  We are the one who will generate a trace file and specify it in the jlink plugin option.  It’s okay to ignore the default trace output if no plugin option is specified and I think no warning should be printed in this case.  It’s just like this plugin is disabled.  You may want to add a suboption to turn on verbose that will trace what is generated and what is ignored.
>>
>> I think a warning is reasonable in all cases: Using a different version of jlink than the java.base you're linking will lose some optimizations and the user would be none the wiser as to why, verbosity helps avoid surprises.
>
> The plugin is enabled by default.  With this change, I consider
> this plugin is "auto-enabled" when it’s creating the image of
> the version that this plugin supports (i.e. matching major.minor
> version).
>
> So if the —generate-jli-classes option is not specified, it might
> be confusing when I get this warning.  I would prefer in this case
> no warning should be emitted and the plugin is not enabled.
>
> If the option is specified on the command-line, it should emit
> the warning.
>
>>
>> http://cr.openjdk.java.net/~redestad/8175026/jdk.02/
>
> 322         if (!initialize(in)) {
>
> Maybe refactor line 175-190 in a new method and something like this:
>   if (!checkVersion(getLinkedVersion(in)))
>       :
>   }
>
> Then follow with initialize(in) here.  That’d make it explicit.
> One thing to handle is when exception is thrown when reading
> the trace file (default or mainArgument). Maybe that part can
> be done early in configure method and store the lines for later
> consumption.
>
> line 235-238: you may use orThrow in this case.
>
> Mandy

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8175026: Capture build-time parameters to --generate-jli-classes

Claes Redestad

On 02/16/2017 10:24 PM, Mandy Chung wrote:
> One more thing: the warning message should be in plugin.properties
> to be localized.

Done:

http://cr.openjdk.java.net/~redestad/8175026/jdk.03/

/Claes

>
> Mandy
>
>> On Feb 16, 2017, at 12:04 PM, Mandy Chung <[hidden email]> wrote:
>>
>>
>>> On Feb 16, 2017, at 5:20 AM, Claes Redestad <[hidden email]> wrote:
>>>
>>>> In addition, if the main argument is specified but the version does not match, it will ignore the given argument.  Should it be an error instead?  We are the one who will generate a trace file and specify it in the jlink plugin option.  It’s okay to ignore the default trace output if no plugin option is specified and I think no warning should be printed in this case.  It’s just like this plugin is disabled.  You may want to add a suboption to turn on verbose that will trace what is generated and what is ignored.
>>> I think a warning is reasonable in all cases: Using a different version of jlink than the java.base you're linking will lose some optimizations and the user would be none the wiser as to why, verbosity helps avoid surprises.
>> The plugin is enabled by default.  With this change, I consider
>> this plugin is "auto-enabled" when it’s creating the image of
>> the version that this plugin supports (i.e. matching major.minor
>> version).
>>
>> So if the —generate-jli-classes option is not specified, it might
>> be confusing when I get this warning.  I would prefer in this case
>> no warning should be emitted and the plugin is not enabled.
>>
>> If the option is specified on the command-line, it should emit
>> the warning.
>>
>>> http://cr.openjdk.java.net/~redestad/8175026/jdk.02/
>> 322         if (!initialize(in)) {
>>
>> Maybe refactor line 175-190 in a new method and something like this:
>>    if (!checkVersion(getLinkedVersion(in)))
>>        :
>>    }
>>
>> Then follow with initialize(in) here.  That’d make it explicit.
>> One thing to handle is when exception is thrown when reading
>> the trace file (default or mainArgument). Maybe that part can
>> be done early in configure method and store the lines for later
>> consumption.
>>
>> line 235-238: you may use orThrow in this case.
>>
>> Mandy

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8175026: Capture build-time parameters to --generate-jli-classes

Mandy Chung

> On Feb 17, 2017, at 6:08 AM, Claes Redestad <[hidden email]> wrote:
>
>
> http://cr.openjdk.java.net/~redestad/8175026/jdk.03/
>

Looks good.  

 82 pre-generate, otherwise the list used to build the host JDK image wil\n\

typo: s/wil/will

Minor suggestion to the wording to "generate-jli-classes.description” message:

Specify a file listing the java.lang.invoke classes to pre-generate
as a hint to this plugin.  By default the plugin will use a builtin
list of pre-generated classes if the image being created matches
the runtime version of this plugin; otherwise, this plugin will be

No need to generate a webrev.

thanks
Mandy