[PATCH] Unnecessary Amount Of Internal Class Conversion

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

[PATCH] Unnecessary Amount Of Internal Class Conversion

Ben Walsh
Per Alan's request here (
http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-October/049532.html
), I am redirecting my initial email to this mailing list ...

I have observed a problem where an unnecessary amount of internal class
conversion is occurring.

I have a patch which I would like to contribute which represents a
performance optimisation for this problem in the java.instrument module
implementation.

It avoids having to convert all classes from a JVM's internal class format
to the .class file format, in order to call the ClassFileLoadHook, when no
java transformer installed.

I would like to pair with a sponsor who could host and review this patch,
so I can get it contributed.

Regards,

Ben Walsh


Unless stated otherwise above:
IBM United Kingdom Limited - Registered in England and Wales with number
741598.
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Unnecessary Amount Of Internal Class Conversion

Ben Walsh
Alan has hosted the patch for me : 
http://cr.openjdk.java.net/~alanb/8189731/webrev/

Regards,

Ben Walsh



-----Ben Walsh/UK/IBM wrote: -----
To: [hidden email]
From: Ben Walsh/UK/IBM
Date: 19/10/2017 11:07
Subject: [PATCH] Unnecessary Amount Of Internal Class Conversion

Per Alan's request here (http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-October/049532.html), I am redirecting my initial email to this mailing list ...

I have observed a problem where an unnecessary amount of internal class conversion is occurring.

I have a patch which I would like to contribute which represents a performance optimisation for this problem in the java.instrument module implementation.

It avoids having to convert all classes from a JVM's internal class format to the .class file format, in order to call the ClassFileLoadHook, when no java transformer installed.

I would like to pair with a sponsor who could host and review this patch, so I can get it contributed.

Regards,


Ben Walsh


Unless stated otherwise above:
IBM United Kingdom Limited - Registered in England and Wales with number 741598.
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU
Unless stated otherwise above:
IBM United Kingdom Limited - Registered in England and Wales with number 741598.
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Unnecessary Amount Of Internal Class Conversion

Ben Walsh
In reply to this post by Ben Walsh
Alan has hosted the patch for me :
http://cr.openjdk.java.net/~alanb/8189731/webrev/

Regards,

Ben Walsh


-----Ben Walsh/UK/IBM wrote: -----
To: [hidden email]
From: Ben Walsh/UK/IBM
Date: 19/10/2017 11:07
Subject: [PATCH] Unnecessary Amount Of Internal Class Conversion

Per Alan's request here (http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-October/049532.html), I am redirecting my initial email to this mailing list ...

I have observed a problem where an unnecessary amount of internal class conversion is occurring.

I have a patch which I would like to contribute which represents a performance optimisation for this problem in the java.instrument module implementation.

It avoids having to convert all classes from a JVM's internal class format to the .class file format, in order to call the ClassFileLoadHook, when no java transformer installed.

I would like to pair with a sponsor who could host and review this patch, so I can get it contributed.

Regards,

Ben Walsh


Unless stated otherwise above:
IBM United Kingdom Limited - Registered in England and Wales with number 741598.
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU
Unless stated otherwise above:
IBM United Kingdom Limited - Registered in England and Wales with number 741598.
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Unnecessary Amount Of Internal Class Conversion

serguei.spitsyn@oracle.com
Hi Ben,

The fix looks good, I see no problems.
Thank you for taking care about this!

I'll sponsor it.
A couple of copyright comments need an update but I can fix it myself.

Do I understand it right that you have no OpenJDK status yet?
At least one more review is required for push.

Thanks,
Serguei


On 10/20/17 05:57, Ben Walsh wrote:

> Alan has hosted the patch for me :
> http://cr.openjdk.java.net/~alanb/8189731/webrev/
>
> Regards,
>
> Ben Walsh
>
>
> -----Ben Walsh/UK/IBM wrote: -----
> To: [hidden email]
> From: Ben Walsh/UK/IBM
> Date: 19/10/2017 11:07
> Subject: [PATCH] Unnecessary Amount Of Internal Class Conversion
>
> Per Alan's request here (http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-October/049532.html), I am redirecting my initial email to this mailing list ...
>
> I have observed a problem where an unnecessary amount of internal class conversion is occurring.
>
> I have a patch which I would like to contribute which represents a performance optimisation for this problem in the java.instrument module implementation.
>
> It avoids having to convert all classes from a JVM's internal class format to the .class file format, in order to call the ClassFileLoadHook, when no java transformer installed.
>
> I would like to pair with a sponsor who could host and review this patch, so I can get it contributed.
>
> Regards,
>
> Ben Walsh
>
>
> Unless stated otherwise above:
> IBM United Kingdom Limited - Registered in England and Wales with number 741598.
> Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU
> Unless stated otherwise above:
> IBM United Kingdom Limited - Registered in England and Wales with number 741598.
> Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU
>

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Unnecessary Amount Of Internal Class Conversion

Ben Walsh
In reply to this post by Ben Walsh
Excellent. Thanks Serguei.

That's correct. It's my first contribution.

Could you recommend someone for the second required review ?


Thanks,
Ben Walsh

-----"[hidden email]" <[hidden email]> wrote: -----
To: Ben Walsh <[hidden email]>, [hidden email]
From: "[hidden email]" <[hidden email]>
Date: 27/10/2017 6:42
Cc: alan Bateman <[hidden email]>
Subject: Re: [PATCH] Unnecessary Amount Of Internal Class Conversion

Hi Ben,

The fix looks good, I see no problems.
Thank you for taking care about this!

I'll sponsor it.
A couple of copyright comments need an update but I can fix it myself.

Do I understand it right that you have no OpenJDK status yet?
At least one more review is required for push.

Thanks,
Serguei


On 10/20/17 05:57, Ben Walsh wrote:

> Alan has hosted the patch for me :
> https://urldefense.proofpoint.com/v2/url?u=http-3A__cr.openjdk.java.net_-7Ealanb_8189731_webrev_&d=DwICaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=0rTKw9slljsdE7-sx--lzTNyOod7e8UQH1LYkfqUvuI&m=g2uzGnK1r9fSRVv1_xM4GD7tmiC3zrARxelV9Iw5KRc&s=w53BxK8jD973k3MASlvUlnnCp1mDJR9R9jSOiDRoYGg&e=
>
> Regards,
>
> Ben Walsh
>
>
> -----Ben Walsh/UK/IBM wrote: -----
> To: [hidden email]
> From: Ben Walsh/UK/IBM
> Date: 19/10/2017 11:07
> Subject: [PATCH] Unnecessary Amount Of Internal Class Conversion
>
> Per Alan's request here (https://urldefense.proofpoint.com/v2/url?u=http-3A__mail.openjdk.java.net_pipermail_core-2Dlibs-2Ddev_2017-2DOctober_049532.html&d=DwICaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=0rTKw9slljsdE7-sx--lzTNyOod7e8UQH1LYkfqUvuI&m=g2uzGnK1r9fSRVv1_xM4GD7tmiC3zrARxelV9Iw5KRc&s=-4eQ0EEyG3wyhV69AvRRNWG5ZreRsVtNHMjTDTYvtso&e=), I am redirecting my initial email to this mailing list ...
>
> I have observed a problem where an unnecessary amount of internal class conversion is occurring.
>
> I have a patch which I would like to contribute which represents a performance optimisation for this problem in the java.instrument module implementation.
>
> It avoids having to convert all classes from a JVM's internal class format to the .class file format, in order to call the ClassFileLoadHook, when no java transformer installed.
>
> I would like to pair with a sponsor who could host and review this patch, so I can get it contributed.
>
> Regards,
>
> Ben Walsh
>
>
> Unless stated otherwise above:
> IBM United Kingdom Limited - Registered in England and Wales with number 741598.
> Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU
> Unless stated otherwise above:
> IBM United Kingdom Limited - Registered in England and Wales with number 741598.
> Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU
>

Unless stated otherwise above:
IBM United Kingdom Limited - Registered in England and Wales with number 741598.
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Unnecessary Amount Of Internal Class Conversion

serguei.spitsyn@oracle.com
Let's check if Alan has a time to look at it.
The patch is not that big.

Thanks,
Serguei


On 10/27/17 03:35, Ben Walsh wrote:

> Excellent. Thanks Serguei.
>
> That's correct. It's my first contribution.
>
> Could you recommend someone for the second required review ?
>
>
> Thanks,
> Ben Walsh
>
> -----"[hidden email]" <[hidden email]> wrote: -----
> To: Ben Walsh <[hidden email]>, [hidden email]
> From: "[hidden email]" <[hidden email]>
> Date: 27/10/2017 6:42
> Cc: alan Bateman <[hidden email]>
> Subject: Re: [PATCH] Unnecessary Amount Of Internal Class Conversion
>
> Hi Ben,
>
> The fix looks good, I see no problems.
> Thank you for taking care about this!
>
> I'll sponsor it.
> A couple of copyright comments need an update but I can fix it myself.
>
> Do I understand it right that you have no OpenJDK status yet?
> At least one more review is required for push.
>
> Thanks,
> Serguei
>
>
> On 10/20/17 05:57, Ben Walsh wrote:
>> Alan has hosted the patch for me :
>> https://urldefense.proofpoint.com/v2/url?u=http-3A__cr.openjdk.java.net_-7Ealanb_8189731_webrev_&d=DwICaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=0rTKw9slljsdE7-sx--lzTNyOod7e8UQH1LYkfqUvuI&m=g2uzGnK1r9fSRVv1_xM4GD7tmiC3zrARxelV9Iw5KRc&s=w53BxK8jD973k3MASlvUlnnCp1mDJR9R9jSOiDRoYGg&e=
>>
>> Regards,
>>
>> Ben Walsh
>>
>>
>> -----Ben Walsh/UK/IBM wrote: -----
>> To: [hidden email]
>> From: Ben Walsh/UK/IBM
>> Date: 19/10/2017 11:07
>> Subject: [PATCH] Unnecessary Amount Of Internal Class Conversion
>>
>> Per Alan's request here (https://urldefense.proofpoint.com/v2/url?u=http-3A__mail.openjdk.java.net_pipermail_core-2Dlibs-2Ddev_2017-2DOctober_049532.html&d=DwICaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=0rTKw9slljsdE7-sx--lzTNyOod7e8UQH1LYkfqUvuI&m=g2uzGnK1r9fSRVv1_xM4GD7tmiC3zrARxelV9Iw5KRc&s=-4eQ0EEyG3wyhV69AvRRNWG5ZreRsVtNHMjTDTYvtso&e=), I am redirecting my initial email to this mailing list ...
>>
>> I have observed a problem where an unnecessary amount of internal class conversion is occurring.
>>
>> I have a patch which I would like to contribute which represents a performance optimisation for this problem in the java.instrument module implementation.
>>
>> It avoids having to convert all classes from a JVM's internal class format to the .class file format, in order to call the ClassFileLoadHook, when no java transformer installed.
>>
>> I would like to pair with a sponsor who could host and review this patch, so I can get it contributed.
>>
>> Regards,
>>
>> Ben Walsh
>>
>>
>> Unless stated otherwise above:
>> IBM United Kingdom Limited - Registered in England and Wales with number 741598.
>> Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU
>> Unless stated otherwise above:
>> IBM United Kingdom Limited - Registered in England and Wales with number 741598.
>> Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU
>>
> Unless stated otherwise above:
> IBM United Kingdom Limited - Registered in England and Wales with number 741598.
> Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU
>

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Unnecessary Amount Of Internal Class Conversion

Alan Bateman


On 27/10/2017 12:08, [hidden email] wrote:
> Let's check if Alan has a time to look at it.
> The patch is not that big. Hi Ben,
There isn't any changes to the hotspot code so one reviewer should be fine.

In any case, I looked through it and it seems okay. My only comment is
that setHasTransformers would be clearer if it were named something like
enableClassFileLoadHook as it just enables or disables the CFLH. I
realize this would mean find a better name for
setHasRetransformableTransformers too.

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

Re: [PATCH] Unnecessary Amount Of Internal Class Conversion

serguei.spitsyn@oracle.com
Hi Alan,

Sorry for the delay (forgot to push the send button).


On 10/30/17 00:52, Alan Bateman wrote:
>
>
> On 27/10/2017 12:08, [hidden email] wrote:
>> Let's check if Alan has a time to look at it.
>> The patch is not that big. Hi Ben,
> There isn't any changes to the hotspot code so one reviewer should be
> fine.

Ok.
I normally push to the jdk10/hs, so that was thinking it is better to
have two reviews.


> In any case, I looked through it and it seems okay.

Ok, I'll add you as a reviewer.


> My only comment is that setHasTransformers would be clearer if it were
> named something like enableClassFileLoadHook as it just enables or
> disables the CFLH.

Just enableClassFileLoadHook is not good enough as we enable CFLH events
separately for retransformable and non-retransformable environments.
The name setHasTransformers looks Ok to me as it signals about the
status change that causes enabling the CFLH events.
I'd suggest to keep it as it is for now.
Please, let me know if it is Ok with you.

> I realize this would mean find a better name for
> setHasRetransformableTransformers too.

Right.


Thanks,
Serguei

>
> -Alan