From: "Anil Saldhana" <Anil.Saldhana(a)redhat.com&gt; To: "Stuart Douglas" <sdouglas(a)redhat.com&gt; Cc: security-dev(a)lists.jboss.org, "Andrig Miller" <anmiller(a)redhat.com&gt; 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 >

From: "Stefan Guilhen" <sguilhen(a)redhat.com&gt; To: "Stuart Douglas" <sdouglas(a)redhat.com&gt; Cc: "Anil Saldhana" <Anil.Saldhana(a)redhat.com&gt;, security-dev(a)lists.jboss.org, "Andrig Miller" <anmiller(a)redhat.com&gt; 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(a)redhat.com&gt; >> To: "Stuart Douglas" <sdouglas(a)redhat.com&gt; >> Cc: security-dev(a)lists.jboss.org, "Andrig Miller" <anmiller(a)redhat.com&gt; >> 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(a)lists.jboss.org >> https://lists.jboss.org/mailman/listinfo/security-dev

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(a)redhat.com&gt; >> To: "Stuart Douglas" <sdouglas(a)redhat.com&gt; >> Cc: "Anil Saldhana" <Anil.Saldhana(a)redhat.com&gt;, security-dev(a)lists.jboss.org, "Andrig Miller" <anmiller(a)redhat.com&gt; >> 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(a)redhat.com&gt; >>>> To: "Stuart Douglas" <sdouglas(a)redhat.com&gt; >>>> Cc: security-dev(a)lists.jboss.org, "Andrig Miller" <anmiller(a)redhat.com&gt; >>>> 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(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/security-dev

Does this include my performance fixes? Stuart ----- Original Message ----- > From: "Peter Skopek" <pskopek(a)redhat.com&gt; > To: security-dev(a)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(a)redhat.com&gt; >>>> To: "Stuart Douglas" <sdouglas(a)redhat.com&gt; >>>> Cc: "Anil Saldhana" <Anil.Saldhana(a)redhat.com&gt;, >>>> security-dev(a)lists.jboss.org, "Andrig Miller" <anmiller(a)redhat.com&gt; >>>> 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(a)redhat.com&gt; >>>>>> To: "Stuart Douglas" <sdouglas(a)redhat.com&gt; >>>>>> Cc: security-dev(a)lists.jboss.org, "Andrig Miller" <anmiller(a)redhat.com&gt; >>>>>> 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(a)lists.jboss.org >> https://lists.jboss.org/mailman/listinfo/security-dev > _______________________________________________ > security-dev mailing list > security-dev(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/security-dev _______________________________________________ security-dev mailing list security-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/security-dev