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).
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
