-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
All,
On 1/25/16 5:23 AM, Mark Thomas wrote:
> 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).
If there were a chain of filters with a common interface, the "easy"
ones (e.g. className.startsWith("org.apache")) ones could be written
with a very fast matching algorithm and the complicated ones could be
written using e.g. regular expressions.
There's more infrastructure to build, but better performance, plus we
don't have to hand-write our own hybrid matching algorithms.
This suggestion has been unpopular in the past. I still think it's a
good idea.
- -chris
-----BEGIN PGP SIGNATURE-----
Comment: GPGTools - http://gpgtools.org
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
iEYEARECAAYFAlan454ACgkQ9CaO5/Lv0PAYIwCfRWBGbIMM6FKePcdIpZmtReIE
vYkAnAi7UcdxNzbVqkGOFhz2VfQwQ6Al
=P1AM
-----END PGP SIGNATURE-----
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
