Re: porting netty-tcnative to tomcat-native
Rémy, On 6/15/15 5:10 AM, Rémy Maucherat wrote: 2015-06-15 10:42 GMT+02:00 Mark Thomas ma...@apache.org: It seems they are used in etPeerCertChain() and getCiphers() only, if someone uses client certificates than might make sense. Those will get called more than once so no objection to caching those classes here. No prooblem with adding that cahching but I don't think it will make a real performance difference. Every little bit helps :) But if it makes the code hard to read, awkward, or otherwise more difficult to maintain, we should get rid of it. Right now, it doesn't look too bad. -chris signature.asc Description: OpenPGP digital signature
Re: porting netty-tcnative to tomcat-native
On 06/15/2015 08:25 AM, Mark Thomas wrote: On 15/06/2015 06:55, jean-frederic clere wrote: Hi, The netty-tcnative is based on the 1.1.x so the porting taking more than excepted. Are you going to be able to commit this in multiple commits or is it going to be one big commit? A big commit it probably more easy, I have a huge diff that I am reviewing, but I will try to cut in piece like ssl.c (and depencencies), sslcontext.c etc (if possible). Picking the netty-tcnative commits one by one would take a lot more time. (And https://github.com/netty/netty-tcnative/commit/c788f64138946f15b8fb8f53d067ccf0fb9699af is a big commit). I would like some comments on the class caching for performances like in ssl.c: +++ TCN_FREE_CSTRING(engine); + +/* Cache the byte[].class for performance reasons */ +clazz = (*e)-FindClass(e, [B); +byteArrayClass = (jclass) (*e)-NewGlobalRef(e, clazz); + +/* Cache the String.class for performance reasons */ +sClazz = (*e)-FindClass(e, java/lang/String); +stringClass = (jclass) (*e)-NewGlobalRef(e, sClazz); + return (jint)APR_SUCCESS; } +++ Should I just put or have a flag in configure for it? I can't see a reason this would need to be optional. Either it helps performance (in which case it should be in) or it is an unnecessary optimisation (in which case it shouldn't be in at all). I took a quick look at ssl.c and I didn't see a lot of lookups for those classes? Does the netty code add some or am I missing something? It seems they are used in etPeerCertChain() and getCiphers() only, if someone uses client certificates than might make sense. Cheers Jean-Frederic - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: porting netty-tcnative to tomcat-native
On 15/06/2015 09:35, jean-frederic clere wrote: On 06/15/2015 08:25 AM, Mark Thomas wrote: On 15/06/2015 06:55, jean-frederic clere wrote: Hi, The netty-tcnative is based on the 1.1.x so the porting taking more than excepted. Are you going to be able to commit this in multiple commits or is it going to be one big commit? A big commit it probably more easy, I have a huge diff that I am reviewing, but I will try to cut in piece like ssl.c (and depencencies), sslcontext.c etc (if possible). Picking the netty-tcnative commits one by one would take a lot more time. (And https://github.com/netty/netty-tcnative/commit/c788f64138946f15b8fb8f53d067ccf0fb9699af is a big commit). I would like some comments on the class caching for performances like in ssl.c: +++ TCN_FREE_CSTRING(engine); + +/* Cache the byte[].class for performance reasons */ +clazz = (*e)-FindClass(e, [B); +byteArrayClass = (jclass) (*e)-NewGlobalRef(e, clazz); + +/* Cache the String.class for performance reasons */ +sClazz = (*e)-FindClass(e, java/lang/String); +stringClass = (jclass) (*e)-NewGlobalRef(e, sClazz); + return (jint)APR_SUCCESS; } +++ Should I just put or have a flag in configure for it? I can't see a reason this would need to be optional. Either it helps performance (in which case it should be in) or it is an unnecessary optimisation (in which case it shouldn't be in at all). I took a quick look at ssl.c and I didn't see a lot of lookups for those classes? Does the netty code add some or am I missing something? It seems they are used in etPeerCertChain() and getCiphers() only, if someone uses client certificates than might make sense. Those will get called more than once so no objection to caching those classes here. Mark - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: porting netty-tcnative to tomcat-native
On 15/06/2015 06:55, jean-frederic clere wrote: Hi, The netty-tcnative is based on the 1.1.x so the porting taking more than excepted. Are you going to be able to commit this in multiple commits or is it going to be one big commit? I would like some comments on the class caching for performances like in ssl.c: +++ TCN_FREE_CSTRING(engine); + +/* Cache the byte[].class for performance reasons */ +clazz = (*e)-FindClass(e, [B); +byteArrayClass = (jclass) (*e)-NewGlobalRef(e, clazz); + +/* Cache the String.class for performance reasons */ +sClazz = (*e)-FindClass(e, java/lang/String); +stringClass = (jclass) (*e)-NewGlobalRef(e, sClazz); + return (jint)APR_SUCCESS; } +++ Should I just put or have a flag in configure for it? I can't see a reason this would need to be optional. Either it helps performance (in which case it should be in) or it is an unnecessary optimisation (in which case it shouldn't be in at all). I took a quick look at ssl.c and I didn't see a lot of lookups for those classes? Does the netty code add some or am I missing something? Cheers, Mark - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
