RFR: 8242039: Improve jlink VersionPropsPlugin

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

RFR: 8242039: Improve jlink VersionPropsPlugin

Langer, Christoph
Hi,

please review a small improvement for the jlink VersionPropsPlugin. The Plugin modifies the bytecode of java/lang/VersionProps.class to replace the initializion of certain vendor specific system properties with custom values. This modification currently adds two bytecodes per constant, a pop and another ldc. I overhauled it to simply replace the original ldc of the old value with another ldc, loading the custom value.

I was playing a bit with the plugin and tried to familiarize with the code – so that’s the outcome 😉

I also added an @SuppressWarnings("unused") in VersionProps.java.template to quiesce an IDE warning.

Bug: https://bugs.openjdk.java.net/browse/JDK-8242039
Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8242039.0/

Thanks
Christoph

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8242039: Improve jlink VersionPropsPlugin

Claes Redestad
Hi Christoph,

fun little micro-optimization. :-)

My only nit is that I'd have preferred if you kept the indentation
style, since the patch now shifts code around a bit (which makes it
harder to review and see the history)

/Claes


On 2020-04-02 17:01, Langer, Christoph wrote:

> Hi,
>
> please review a small improvement for the jlink VersionPropsPlugin. The Plugin modifies the bytecode of java/lang/VersionProps.class to replace the initializion of certain vendor specific system properties with custom values. This modification currently adds two bytecodes per constant, a pop and another ldc. I overhauled it to simply replace the original ldc of the old value with another ldc, loading the custom value.
>
> I was playing a bit with the plugin and tried to familiarize with the code – so that’s the outcome 😉
>
> I also added an @SuppressWarnings("unused") in VersionProps.java.template to quiesce an IDE warning.
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8242039
> Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8242039.0/
>
> Thanks
> Christoph
>
Reply | Threaded
Open this post in threaded view
|

RE: RFR: 8242039: Improve jlink VersionPropsPlugin

Langer, Christoph
Hi Claes,

> fun little micro-optimization. :-)

Yep, I got a bit obsessed here. 😉 But thanks for the review.

> My only nit is that I'd have preferred if you kept the indentation
> style, since the patch now shifts code around a bit (which makes it
> harder to review and see the history)

OK, I created an edition which doesn't change the formatting so that the added code becomes more obvious:
http://cr.openjdk.java.net/~clanger/webrevs/8242039.1/

Does it look better?

Cheers
Christoph

>
> /Claes
>
>
> On 2020-04-02 17:01, Langer, Christoph wrote:
> > Hi,
> >
> > please review a small improvement for the jlink VersionPropsPlugin. The
> Plugin modifies the bytecode of java/lang/VersionProps.class to replace the
> initializion of certain vendor specific system properties with custom values.
> This modification currently adds two bytecodes per constant, a pop and
> another ldc. I overhauled it to simply replace the original ldc of the old value
> with another ldc, loading the custom value.
> >
> > I was playing a bit with the plugin and tried to familiarize with the code – so
> that’s the outcome 😉
> >
> > I also added an @SuppressWarnings("unused") in
> VersionProps.java.template to quiesce an IDE warning.
> >
> > Bug: https://bugs.openjdk.java.net/browse/JDK-8242039
> > Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8242039.0/
> >
> > Thanks
> > Christoph
> >
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8242039: Improve jlink VersionPropsPlugin

Claes Redestad


On 2020-04-02 20:21, Langer, Christoph wrote:
> OK, I created an edition which doesn't change the formatting so that the added code becomes more obvious:
> http://cr.openjdk.java.net/~clanger/webrevs/8242039.1/
>
> Does it look better?

I think so, yes.

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

Re: RFR: 8242039: Improve jlink VersionPropsPlugin

mark.reinhold
In reply to this post by Langer, Christoph
2020/4/2 8:01:28 -0700, [hidden email]:
> please review a small improvement for the jlink
> VersionPropsPlugin. The Plugin modifies the bytecode of
> java/lang/VersionProps.class to replace the initializion of certain
> vendor specific system properties with custom values. This
> modification currently adds two bytecodes per constant, a pop and
> another ldc. I overhauled it to simply replace the original ldc of the
> old value with another ldc, loading the custom value.

I thought about doing this when I originally wrote that plugin, but it’s
so awkward to achieve with ASM -- as demonstrated by your patch -- that
I concluded it wasn’t worth it.  Who will notice an extra pop in a basic
block that’s only ever executed once?  Is the complexity of this new
code worth the benefit?

- Mark
Reply | Threaded
Open this post in threaded view
|

RE: RFR: 8242039: Improve jlink VersionPropsPlugin

Langer, Christoph
Hi Mark,

> -----Original Message-----
> From: [hidden email] <[hidden email]>
> Sent: Donnerstag, 2. April 2020 23:13
> To: Langer, Christoph <[hidden email]>
> Cc: [hidden email]; [hidden email]
> Subject: Re: RFR: 8242039: Improve jlink VersionPropsPlugin
>
> 2020/4/2 8:01:28 -0700, [hidden email]:
> > please review a small improvement for the jlink
> > VersionPropsPlugin. The Plugin modifies the bytecode of
> > java/lang/VersionProps.class to replace the initializion of certain
> > vendor specific system properties with custom values. This
> > modification currently adds two bytecodes per constant, a pop and
> > another ldc. I overhauled it to simply replace the original ldc of the
> > old value with another ldc, loading the custom value.
>
> I thought about doing this when I originally wrote that plugin, but it’s
> so awkward to achieve with ASM -- as demonstrated by your patch -- that
> I concluded it wasn’t worth it.  Who will notice an extra pop in a basic
> block that’s only ever executed once?  Is the complexity of this new
> code worth the benefit?

Well, first I started playing with this and got a bit obsessed to find optimizations in that area. (I learned quite a bit about java asm.)
It would be of higher (micro-)benefit for common VM startup if the fields to be modified could be final but that's even more awkward to do and requires intricate knowledge and assumptions about how VersionProps.java is structured. So I decided against messing with that.

Eventually I came up with this result and then I also asked myself the question whether the new complexity was worth the benefit. I answered myself with a yes (though definitely not a clear one 😉), and that's why I proposed the change. After all, the new complexity isn't huge...

So, would that be your terminal veto or could you imagine accepting the change?

Best regards
Christoph

Reply | Threaded
Open this post in threaded view
|

RE: RFR: 8242039: Improve jlink VersionPropsPlugin

Langer, Christoph
In reply to this post by Claes Redestad
Thanks, Claes.

> -----Original Message-----
> From: Claes Redestad <[hidden email]>
> Sent: Donnerstag, 2. April 2020 23:05
> To: Langer, Christoph <[hidden email]>; jigsaw-
> [hidden email]; [hidden email]
> Subject: Re: RFR: 8242039: Improve jlink VersionPropsPlugin
>
>
>
> On 2020-04-02 20:21, Langer, Christoph wrote:
> > OK, I created an edition which doesn't change the formatting so that the
> added code becomes more obvious:
> > http://cr.openjdk.java.net/~clanger/webrevs/8242039.1/
> >
> > Does it look better?
>
> I think so, yes.
>
> /Claes
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8242039: Improve jlink VersionPropsPlugin

mark.reinhold
In reply to this post by Langer, Christoph
2020/4/3 6:36:53 -0700, [hidden email]:

> 2020/4/2 14:12:54 -0700, [hidden email]:
>> I thought about doing this when I originally wrote that plugin, but it’s
>> so awkward to achieve with ASM -- as demonstrated by your patch -- that
>> I concluded it wasn’t worth it.  Who will notice an extra pop in a basic
>> block that’s only ever executed once?  Is the complexity of this new
>> code worth the benefit?
>
> Well, first I started playing with this and got a bit obsessed to find
> optimizations in that area. (I learned quite a bit about java asm.)
> It would be of higher (micro-)benefit for common VM startup if the
> fields to be modified could be final but that's even more awkward to
> do and requires intricate knowledge and assumptions about how
> VersionProps.java is structured. So I decided against messing with
> that.
>
> Eventually I came up with this result and then I also asked myself the
> question whether the new complexity was worth the benefit. I answered
> myself with a yes (though definitely not a clear one 😉), and that's
> why I proposed the change. After all, the new complexity isn't huge...
>
> So, would that be your terminal veto or could you imagine accepting
> the change?

I won’t veto it.

- Mark
Reply | Threaded
Open this post in threaded view
|

RE: RFR: 8242039: Improve jlink VersionPropsPlugin

Langer, Christoph
In reply to this post by Langer, Christoph
Hi Claes and Mark,

thanks again for your inputs.

I'll push this with Claes' review then.

Best regards
Christoph

> -----Original Message-----
> From: core-libs-dev <[hidden email]> On Behalf
> Of Claes Redestad
> Sent: Mittwoch, 8. April 2020 00:12
> To: [hidden email]
> Subject: Re: RFR: 8242039: Improve jlink VersionPropsPlugin
>
>
>
> On 2020-04-03 15:36, Langer, Christoph wrote:
> > Eventually I came up with this result and then I also asked myself the
> question whether the new complexity was worth the benefit. I answered
> myself with a yes (though definitely not a clear one 😉), and that's why I
> proposed the change. After all, the new complexity isn't huge...
>
> I don't mind the cleaned up patch[1].
>
> It also gets rid of the constants being replaced, which I assume will
> otherwise be loaded and kept on the heap and in the string table
> forever. While unlikely to cause confusion, I'd argue that not finding
> the value replaced in heap dumps might be of some value.
>
> /Claes
>
> [1] http://cr.openjdk.java.net/~clanger/webrevs/8242039.1/