Am 25.01.2016 um 11:23 schrieb Mark Thomas:
On 25/01/2016 09:32, Rainer Jung wrote:
Hi Konstantin,
thanks for the feedback. More inline.
Am 25.01.2016 um 03:51 schrieb Konstantin Kolinko:
2016-01-23 15:17 GMT+03:00 Rainer Jung <rainer.j...@kippdata.de>:
I observed a bottleneck in WebappClassLoaderBase.filter() during a
stress
test. The reason is, that the method is synchronized. It looks to me,
that
the reason for the synchronization is only the access to the
non-thread safe
Matchers packageTriggersPermit and packageTriggersDeny. Since they
are not
used anywhere else, IMHO one can get away with using a separate Lock
object
for access to the two. E.g.
1. Instead of a lock it is possible to create a new Matcher object on
each invocation.
For performance reasons - this is a hot code path - I would actually
prefer the variant I sent later. The hand written code that doesn't use
regexp (in a variant that uses enums instead of the "*_or_*" boolean
variables. Probably also adjusted due to your remark about the use of
"." and "/".
Given the performance improvements, that works for me.
2. There is no need for those Matcher / new Lock object to be
"protected". An interested party can overwrite the whole filter()
method.
Right. With the hand written code they would be completely gone.
3. Filtering resources and classes with the same regexp looks wrong.
It is safe to use that regexp on class names (those do not have '/'
character in them), but it is wrong to use it on resources ('.' is a
valid character in a file name, not a package separator).
The hand written code no longer supports filtering names with mixed "."
and "/". I guess those were never intended to get filtered.
If we want to make it even more correct, namely using "." when filtering
class names and "/" when filtering resources (I hope that would be
correct), the filter() method would need a flag to indicate which case
we are in. That would mean changing the signature of a protected method.
I guess we need to keep the old signature additionally for compatibility.
4. Generally (if I remember correctly) this filter
a) saves people that do a stupid thing of trying to bundle API classes
or Tomcat classes in their war (e.g. due to misconfiguration of Maven
dependencies)
b) allows a performance improvement, as the search for those classes
in web application classloader can be skipped
It doesn't skip, but it delegates to the parent first for the filtered
names, where it most likely will find the searched items.
I forgot whether there were also spec reasons. You indicate, that the
reasons are not spec compliance but performance.
The specification requirements are in 10.7.2. The filter method is
implementing the part that states:
<quote>
The container should not allow applications to override or access the
container’s implementation classes.
</quote>
The JavaSE parts of that section are implemented elsewhere (the svn
history for WebappClassLoaderBase has the details).
I committed first some regexp minor enhancements, then the improved
locking and finally the hand tuned code that doesn't use any regexp or
locking. The intermediate commits are just in case if we want to go back
to the regexp based solution. I plan to backport all changes unless I
hear objections.
Regards,
Rainer
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
