[security-dev] Picketbox performance improvements

[Anil Saldhana]

Stefan applied your fixes. It is there.

On Sep 11, 2013, at 5:35 AM, Stuart Douglas <sdouglas at redhat.com> wrote:

> Does this include my performance fixes?
> 
> Stuart
> 
> ----- Original Message -----
>> From: "Peter Skopek" <pskopek at redhat.com>
>> To: security-dev at lists.jboss.org
>> Sent: Wednesday, 11 September, 2013 12:15:34 PM
>> Subject: Re: [security-dev] Picketbox performance improvements
>> 
>> Hi Stuart,
>> 
>> there is a PR https://github.com/wildfly/wildfly/pull/5029
>> to upgrade to new PicketBox which contains some fixes from Stefan in this
>> matter.
>> 
>> Peter
>> 
>> On 09/05/2013 05:03 PM, Anil Saldhana wrote:
>>> Hi Stuart,
>>>   Stefan and I started discussing the module organization for PicketBox
>>> few days back. I think it is time for a separate email thread on that,
>>> in this list. I think we are going to try to stick to "one jar per JBoss
>>> module" mantra in WF8.
>>> 
>>> Regards,
>>> Anil
>>> 
>>> On 09/05/2013 09:35 AM, Stuart Douglas wrote:
>>>> I thought of that, however the default security context classes are not
>>>> available from the maven module that houses the factory. Given that they
>>>> all just get assembled into 1 jar anyway I wonder if they should just be
>>>> merged into a single module?
>>>> 
>>>> Stuart
>>>> 
>>>> ----- Original Message -----
>>>>> From: "Stefan Guilhen" <sguilhen at redhat.com>
>>>>> To: "Stuart Douglas" <sdouglas at redhat.com>
>>>>> Cc: "Anil Saldhana" <Anil.Saldhana at redhat.com>,
>>>>> security-dev at lists.jboss.org, "Andrig Miller" <anmiller at redhat.com>
>>>>> Sent: Thursday, 5 September, 2013 3:47:27 PM
>>>>> Subject: Re: [security-dev] Picketbox performance improvements
>>>>> 
>>>>> Hi Stuart,
>>>>> 
>>>>> I've applied your changes and they will be available once we upgrade
>>>>> PicketBox, which should happen soon. I do believe that this heavy usage
>>>>> of reflection is overkill since most of the time the default
>>>>> SecurityContext implementation is used. I've never heard of a case where
>>>>> this implemenation has been changed. So I'm wondering if we couldn't
>>>>> just make sure that whenever the default context is being used we
>>>>> instantiate it by calling new JBossSecurityContext(securityDomain) and
>>>>> use the reflection stuff only if the caller really supplies a different
>>>>> implementation class.
>>>>> 
>>>>> Stefan
>>>>> 
>>>>> On 09/05/2013 06:41 AM, Stuart Douglas wrote:
>>>>>> Oops, just realised I missed that the same thing was happening with the
>>>>>> SecurityContextUtil class.
>>>>>> 
>>>>>> With this patch, and my recently merged SecurityContextAssociationValve
>>>>>> patch, I have seen a >20% increase in performance of an empty web
>>>>>> application (28k req/sec vs 23k req/sec).
>>>>>> 
>>>>>> Stuart
>>>>>> 
>>>>>> 
>>>>>> ----- Original Message -----
>>>>>>> From: "Anil Saldhana" <Anil.Saldhana at redhat.com>
>>>>>>> To: "Stuart Douglas" <sdouglas at redhat.com>
>>>>>>> Cc: security-dev at lists.jboss.org, "Andrig Miller" <anmiller at redhat.com>
>>>>>>> Sent: Wednesday, 4 September, 2013 3:47:47 PM
>>>>>>> Subject: Re: [security-dev] Picketbox performance improvements
>>>>>>> 
>>>>>>> Hi Stuart,
>>>>>>>     it will be a couple of days to upgrade. There are other fixes
>>>>>>>     going
>>>>>>> into the upgrade.
>>>>>>> 
>>>>>>> Regards,
>>>>>>> Anil
>>>>>>> 
>>>>>>> On 09/04/2013 04:31 AM, Stuart Douglas wrote:
>>>>>>>> Hi,
>>>>>>>> 
>>>>>>>> I have been benchmarking Wildfly upstream, and with the exception of
>>>>>>>> the
>>>>>>>> Weld listener (that I am about to try and fix), I am seeing the
>>>>>>>> creation
>>>>>>>> of the security context as by far the most expensive part of the web
>>>>>>>> request chain.
>>>>>>>> 
>>>>>>>> Firstly a bit about the tests, basically it is possible to run
>>>>>>>> Undertow
>>>>>>>> in
>>>>>>>> pipelining mode, where if you send it pipelined HTTP requests it will
>>>>>>>> buffer the responses and batch them. This is not super useful in
>>>>>>>> practice,
>>>>>>>> but what it does allow me to do is basically take most of the IO
>>>>>>>> component
>>>>>>>> out of a web request, and just look at which bits of the web request
>>>>>>>> chain
>>>>>>>> are consuming the most CPU.
>>>>>>>> 
>>>>>>>> The issues I am seeing are:
>>>>>>>> 
>>>>>>>> - Some unnecessary AccessController.doPrivilidged calls
>>>>>>>> - A reflection call on every request to lookup the constructor of the
>>>>>>>> security context class
>>>>>>>> 
>>>>>>>> I have provided a patch to address both of these, by adding a guard
>>>>>>>> around
>>>>>>>> the AccessController calls, and by caching the constructor used for
>>>>>>>> the
>>>>>>>> default security context class. I think it may even be worthwhile
>>>>>>>> taking
>>>>>>>> this one step further, and using generated bytecode to create the
>>>>>>>> class.
>>>>>>>> Normally I would consider this overkill but security context creation
>>>>>>>> happens on every request, so if we are serious about performance we
>>>>>>>> should
>>>>>>>> be trying to make it as cheap as possible.
>>>>>>>> 
>>>>>>>> To give you an idea of how much this affects the total cost of the
>>>>>>>> Servlet
>>>>>>>> pipeline:
>>>>>>>> 
>>>>>>>> Current WF upstream (without Weld): 134k req/sec
>>>>>>>> After removing the AccessController calls: 158k req/sec
>>>>>>>> Adding constructor caching: 171k req/sec
>>>>>>>> 
>>>>>>>> Note that the speedup will not be as big for more realistic workloads,
>>>>>>>> however it will still be significant.
>>>>>>>> 
>>>>>>>> Stuart
>>> _______________________________________________
>>> security-dev mailing list
>>> security-dev at lists.jboss.org
>>> https://lists.jboss.org/mailman/listinfo/security-dev
>> _______________________________________________
>> security-dev mailing list
>> security-dev at lists.jboss.org
>> https://lists.jboss.org/mailman/listinfo/security-dev
> _______________________________________________
> security-dev mailing list
> security-dev at lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/security-dev