You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by Romain Manni-Bucau <rm...@gmail.com> on 2022/08/22 09:20:14 UTC
jakarta.el, useless getResource?
Hi all,
I just spotted that in jakarta.el.ImportHandler#findClass there is a
cl.getResource(path)
== null (similar code exists when there is a security manager).
One line later there is a clazz = cl.loadClass(name);.
I assume the intent is to check the class is physically accessible but I
have a few questions about that:
1. It seems it does not respect the classloader API since this one enables
by contract to define a class (loadclass) without having a resource - it is
often used by proxies for ex,
2. getResource is insanely slow (dropping it by using a subclass which
overrides ImportHandler by reflection I got a x10 ops/s on a 4 lines
template with one import)
3. Why would getResource fail but loadClass succeed - I ignore any
configuration error which is "fixable"?
From the comment on top of the getResource check it seems it is for
performances (getresource being cheaper than loadClass) but I wonder if it
is still accurate (from my window it depends on the classpath size plus
loadClass will mainly go through getResource).
So overall I wonder if this check can be dropped now we have concurrent
classloaders and cache almost everywhere. If not, should the missed items
be cached in some (webapp) classloader to help to exit faster?
Romain Manni-Bucau
@rmannibucau <https://twitter.com/rmannibucau> | Blog
<https://rmannibucau.metawerx.net/> | Old Blog
<http://rmannibucau.wordpress.com> | Github <https://github.com/rmannibucau> |
LinkedIn <https://www.linkedin.com/in/rmannibucau> | Book
<https://www.packtpub.com/application-development/java-ee-8-high-performance>
Re: jakarta.el, useless getResource?
Posted by Romain Manni-Bucau <rm...@gmail.com>.
Hi Chris,
My x10 dropping it was on java 17.
Romain
Le ven. 26 août 2022 à 02:39, Christopher Schultz <
chris@christopherschultz.net> a écrit :
> Mark,
>
> On 8/22/22 07:54, Mark Thomas wrote:
> > On 22/08/2022 11:48, Mark Thomas wrote:
> >> On 22/08/2022 10:20, Romain Manni-Bucau wrote:
> >
> > <snip/>
> >
> >>> So overall I wonder if this check can be dropped now we have concurrent
> >>> classloaders and cache almost everywhere. If not, should the missed
> >>> items
> >>> be cached in some (webapp) classloader to help to exit faster?
> >>
> >> We need to test with various JDKs but if the results are comparable to
> >> those for Java 11, I'd have no objection to simplifying the code.
> >
> > I've just run the performance test with Java 7, Java 8 and Java 11 with
> > 8.5.x and in all three cases, the average time to run the test was less
> > without the performance fix than with it.
> >
> > Given these, results, I think we remove this performance hack for all
> > current versions.
> >
> > Objections?
>
> I would run under Java 17 just to make sure that whatever optimizations
> were done in Java 11 are still there. I suspect there will be no change
> in behavior, but as we have seen over the last 20 years, sometimes
> things do change wrt performance.
>
> -chris
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
>
Re: jakarta.el, useless getResource?
Posted by Christopher Schultz <ch...@christopherschultz.net>.
Mark,
On 8/22/22 07:54, Mark Thomas wrote:
> On 22/08/2022 11:48, Mark Thomas wrote:
>> On 22/08/2022 10:20, Romain Manni-Bucau wrote:
>
> <snip/>
>
>>> So overall I wonder if this check can be dropped now we have concurrent
>>> classloaders and cache almost everywhere. If not, should the missed
>>> items
>>> be cached in some (webapp) classloader to help to exit faster?
>>
>> We need to test with various JDKs but if the results are comparable to
>> those for Java 11, I'd have no objection to simplifying the code.
>
> I've just run the performance test with Java 7, Java 8 and Java 11 with
> 8.5.x and in all three cases, the average time to run the test was less
> without the performance fix than with it.
>
> Given these, results, I think we remove this performance hack for all
> current versions.
>
> Objections?
I would run under Java 17 just to make sure that whatever optimizations
were done in Java 11 are still there. I suspect there will be no change
in behavior, but as we have seen over the last 20 years, sometimes
things do change wrt performance.
-chris
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: jakarta.el, useless getResource?
Posted by Mark Thomas <ma...@apache.org>.
On 24/08/2022 14:40, Romain Manni-Bucau wrote:
> Hi
>
> Went ahead and created https://github.com/apache/tomcat/pull/547 (if it
> helps)
Thanks. There weren't any objections so I'll merge that PR shortly.
Mark
>
> Romain Manni-Bucau
> @rmannibucau <https://twitter.com/rmannibucau> | Blog
> <https://rmannibucau.metawerx.net/> | Old Blog
> <http://rmannibucau.wordpress.com> | Github <https://github.com/rmannibucau> |
> LinkedIn <https://www.linkedin.com/in/rmannibucau> | Book
> <https://www.packtpub.com/application-development/java-ee-8-high-performance>
>
>
> Le lun. 22 août 2022 à 14:25, Romain Manni-Bucau <rm...@gmail.com> a
> écrit :
>
>> +1
>>
>> To answer the proxy reference: it affects other cases - loading classes
>> from a "database", proxies is just a well known case I used to illustrate
>> my point. By contract a classloader is not always an URLClassLoader which
>> is the assumption of the impl right now. Also CDS changes the perf there
>> too - a lot when enabled.
>>
>> Side note: graalvm integration is way easier without that check ;).
>>
>> Romain Manni-Bucau
>> @rmannibucau <https://twitter.com/rmannibucau> | Blog
>> <https://rmannibucau.metawerx.net/> | Old Blog
>> <http://rmannibucau.wordpress.com> | Github
>> <https://github.com/rmannibucau> | LinkedIn
>> <https://www.linkedin.com/in/rmannibucau> | Book
>> <https://www.packtpub.com/application-development/java-ee-8-high-performance>
>>
>>
>> Le lun. 22 août 2022 à 13:54, Mark Thomas <ma...@apache.org> a écrit :
>>
>>> On 22/08/2022 11:48, Mark Thomas wrote:
>>>> On 22/08/2022 10:20, Romain Manni-Bucau wrote:
>>>
>>> <snip/>
>>>
>>>>> So overall I wonder if this check can be dropped now we have concurrent
>>>>> classloaders and cache almost everywhere. If not, should the missed
>>> items
>>>>> be cached in some (webapp) classloader to help to exit faster?
>>>>
>>>> We need to test with various JDKs but if the results are comparable to
>>>> those for Java 11, I'd have no objection to simplifying the code.
>>>
>>> I've just run the performance test with Java 7, Java 8 and Java 11 with
>>> 8.5.x and in all three cases, the average time to run the test was less
>>> without the performance fix than with it.
>>>
>>> Given these, results, I think we remove this performance hack for all
>>> current versions.
>>>
>>> Objections?
>>>
>>> Mark
>>>
>>> ---------------------------------------------------------------------
>>> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
>>> For additional commands, e-mail: dev-help@tomcat.apache.org
>>>
>>>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: jakarta.el, useless getResource?
Posted by Romain Manni-Bucau <rm...@gmail.com>.
Hi
Went ahead and created https://github.com/apache/tomcat/pull/547 (if it
helps)
Romain Manni-Bucau
@rmannibucau <https://twitter.com/rmannibucau> | Blog
<https://rmannibucau.metawerx.net/> | Old Blog
<http://rmannibucau.wordpress.com> | Github <https://github.com/rmannibucau> |
LinkedIn <https://www.linkedin.com/in/rmannibucau> | Book
<https://www.packtpub.com/application-development/java-ee-8-high-performance>
Le lun. 22 août 2022 à 14:25, Romain Manni-Bucau <rm...@gmail.com> a
écrit :
> +1
>
> To answer the proxy reference: it affects other cases - loading classes
> from a "database", proxies is just a well known case I used to illustrate
> my point. By contract a classloader is not always an URLClassLoader which
> is the assumption of the impl right now. Also CDS changes the perf there
> too - a lot when enabled.
>
> Side note: graalvm integration is way easier without that check ;).
>
> Romain Manni-Bucau
> @rmannibucau <https://twitter.com/rmannibucau> | Blog
> <https://rmannibucau.metawerx.net/> | Old Blog
> <http://rmannibucau.wordpress.com> | Github
> <https://github.com/rmannibucau> | LinkedIn
> <https://www.linkedin.com/in/rmannibucau> | Book
> <https://www.packtpub.com/application-development/java-ee-8-high-performance>
>
>
> Le lun. 22 août 2022 à 13:54, Mark Thomas <ma...@apache.org> a écrit :
>
>> On 22/08/2022 11:48, Mark Thomas wrote:
>> > On 22/08/2022 10:20, Romain Manni-Bucau wrote:
>>
>> <snip/>
>>
>> >> So overall I wonder if this check can be dropped now we have concurrent
>> >> classloaders and cache almost everywhere. If not, should the missed
>> items
>> >> be cached in some (webapp) classloader to help to exit faster?
>> >
>> > We need to test with various JDKs but if the results are comparable to
>> > those for Java 11, I'd have no objection to simplifying the code.
>>
>> I've just run the performance test with Java 7, Java 8 and Java 11 with
>> 8.5.x and in all three cases, the average time to run the test was less
>> without the performance fix than with it.
>>
>> Given these, results, I think we remove this performance hack for all
>> current versions.
>>
>> Objections?
>>
>> Mark
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
>> For additional commands, e-mail: dev-help@tomcat.apache.org
>>
>>
Re: jakarta.el, useless getResource?
Posted by Romain Manni-Bucau <rm...@gmail.com>.
+1
To answer the proxy reference: it affects other cases - loading classes
from a "database", proxies is just a well known case I used to illustrate
my point. By contract a classloader is not always an URLClassLoader which
is the assumption of the impl right now. Also CDS changes the perf there
too - a lot when enabled.
Side note: graalvm integration is way easier without that check ;).
Romain Manni-Bucau
@rmannibucau <https://twitter.com/rmannibucau> | Blog
<https://rmannibucau.metawerx.net/> | Old Blog
<http://rmannibucau.wordpress.com> | Github <https://github.com/rmannibucau> |
LinkedIn <https://www.linkedin.com/in/rmannibucau> | Book
<https://www.packtpub.com/application-development/java-ee-8-high-performance>
Le lun. 22 août 2022 à 13:54, Mark Thomas <ma...@apache.org> a écrit :
> On 22/08/2022 11:48, Mark Thomas wrote:
> > On 22/08/2022 10:20, Romain Manni-Bucau wrote:
>
> <snip/>
>
> >> So overall I wonder if this check can be dropped now we have concurrent
> >> classloaders and cache almost everywhere. If not, should the missed
> items
> >> be cached in some (webapp) classloader to help to exit faster?
> >
> > We need to test with various JDKs but if the results are comparable to
> > those for Java 11, I'd have no objection to simplifying the code.
>
> I've just run the performance test with Java 7, Java 8 and Java 11 with
> 8.5.x and in all three cases, the average time to run the test was less
> without the performance fix than with it.
>
> Given these, results, I think we remove this performance hack for all
> current versions.
>
> Objections?
>
> Mark
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
>
Re: jakarta.el, useless getResource?
Posted by Mark Thomas <ma...@apache.org>.
On 22/08/2022 11:48, Mark Thomas wrote:
> On 22/08/2022 10:20, Romain Manni-Bucau wrote:
<snip/>
>> So overall I wonder if this check can be dropped now we have concurrent
>> classloaders and cache almost everywhere. If not, should the missed items
>> be cached in some (webapp) classloader to help to exit faster?
>
> We need to test with various JDKs but if the results are comparable to
> those for Java 11, I'd have no objection to simplifying the code.
I've just run the performance test with Java 7, Java 8 and Java 11 with
8.5.x and in all three cases, the average time to run the test was less
without the performance fix than with it.
Given these, results, I think we remove this performance hack for all
current versions.
Objections?
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: jakarta.el, useless getResource?
Posted by Mark Thomas <ma...@apache.org>.
On 22/08/2022 10:20, Romain Manni-Bucau wrote:
> Hi all,
>
> I just spotted that in jakarta.el.ImportHandler#findClass there is a
> cl.getResource(path)
> == null (similar code exists when there is a security manager).
> One line later there is a clazz = cl.loadClass(name);.
>
> I assume the intent is to check the class is physically accessible but I
> have a few questions about that:
>
> 1. It seems it does not respect the classloader API since this one enables
> by contract to define a class (loadclass) without having a resource - it is
> often used by proxies for ex,
Do we need to worry about proxies for this use case?
> 2. getResource is insanely slow (dropping it by using a subclass which
> overrides ImportHandler by reflection I got a x10 ops/s on a 4 lines
> template with one import)
See TesterImportHandlerPerformance. Previous results were 2 orders of
magnitude the other way.
> 3. Why would getResource fail but loadClass succeed - I ignore any
> configuration error which is "fixable"?
From memory this trick failed when used under a SecurityManager with
classes that had circular dependencies. I might not have the details
quite right but there wasn't anything wrong with the classes or the
application.
> From the comment on top of the getResource check it seems it is for
> performances (getresource being cheaper than loadClass) but I wonder if it
> is still accurate (from my window it depends on the classpath size plus
> loadClass will mainly go through getResource).
A quick test with TesterImportHandlerPerformance and Java 11 suggests
there performance benefits are a lot smaller.
> So overall I wonder if this check can be dropped now we have concurrent
> classloaders and cache almost everywhere. If not, should the missed items
> be cached in some (webapp) classloader to help to exit faster?
We need to test with various JDKs but if the results are comparable to
those for Java 11, I'd have no objection to simplifying the code.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org