Optimize RequestListener

[umpirsky]

Looks like RequestListener adds some latency, and not onCoreRequest as you'd expect, but just fetching the service from container can add ~25ms to each request.

I tried to track down the dependency graph and it goes like this: scheb_two_factor.security.request_listener -> scheb_two_factor.trusted_filter -> scheb_two_factor.provider_registry -> scheb_two_factor.security.google.provider -> scheb_two_factor.security.google.backup_code_validator -> scheb_two_factor.backup_code_validator -> scheb_two_factor.persister.doctrine.

If using lazy services and check FlagManager.isNotAuthenticated() earlier this can be optimized.

I know FlagManager does not know about providers, but maybe it can be tweaked to pull a minimal set of dependencies if FlagManager.isNotAuthenticated() is false.

[scheb]

The 25ms seem a little bit too high for me for just fetching a service. DIC in prod should be able to do that way faster. Did you measure those values in a "dev" or a "prod" environment? Do you have some op-code cache enabled, either APC or native PHP7 op-code cache?

Anyways, there IS room for improvment.

I think a major improvment would be if scheb_two_factor.provider_registry would lazy-load the authentication provider services, because it only needs the service instance, when two-factor authentication has to be executed. But I wouldn't like to require the ocramius/proxy-manager on the bundle-level. Using that is more of a application-level decision, that the user has to do, and I don't want people force using it. So a simple "AuthenticationProviderFactory" that fetches the service from DIC on-demand would be the best solution im my opinion.

[umpirsky]

@scheb Yes, it is too high, that's why I'm posting. Other services are fetched way faster. It's from Blackfire profile done on prouduction with op-code cache enabled. Thanks.

[zerkms]

So, what exactly consumes those 25ms?

[umpirsky]

@zerkms Fetching service from container:

[zerkms]

Right, but what exactly takes so long? 26ms is A LOT. Nothing but few objects are instantiated there.

On my symfony projects some of entire requests (from the first request byte to the last response byte) take less than that.

I would still suggest to find the real problem before trying to optimise it, since it looks suspicious.

[umpirsky]

@zerkms Yes, I agree, still investigating it. Only services from this bundle are loading this slow.

[umpirsky]

Here are the latest findings. Making scheb_two_factor.provider_registry service lazy lowers down latency by double. Making scheb_two_factor.trusted_filter makes latency go away, no services from this bundle are shown in Blackfire profile, which means bottleneck is gone.

@scheb I think requiring ocramius/proxy-manager is totally worth it.

[scheb]

e9f3d5e

Release will be published as soon as Travis build greenlights the change.

[umpirsky]

@scheb Awesome, thanks! 👍

[zerkms]

Well, I checked code and I'm not convinced that it should have been implemented like that.

The current solution mitigates the consequences but does not solve the root of the problems (since the roots haven't been detected).

Adding lazy to something that looks like slow but is not proven to be the problem - is crazy.

Especially that - when exclude_pattern is used it is not to be instantiated at all, and when it's not - the service must be instantiated anyway. So overall - the proposed solution made the performance worse (since the proxy is not free).

My best guess would be that it's other dependencies that scheb_two_factor.provider_registry depends on are slow, and making it lazy just postpones their initialisation. So - you just made it looking like it became faster, while the load was just spread over other services.

UPD: and after some debugging I'm even more confident that what I said above is true and that "the solution" solves nothing just hides the real problem somewhere else. Some candidates that do some heavy lifting are templating and perhaps the doctrine.

[umpirsky]

@zerkms I agree except that it does not solves nothing. It solves slowness on all pages that are not authentication. This is a huge win for me. I agree that it should be investigated further and optimize auth pages.

[zerkms]

@umpirsky well, lazy does not prevent the service from loading, it just postpones it. So it is still instantiated, just slightly later. My point is that the problem was shifted to other place.

Did you get instant improvement of performance of 26ms everywhere? So every page now loads 26ms faster? You should not have.

It solves slowness on all pages that are not authentication.

It's not possible - it is still instantiated.

[umpirsky]

@zerkms No, because it's not used during request that does not trigger 2FA. So it's not instantiated.

[zerkms]

@umpirsky have you tried to confirm that with a debugger?

https://github.com/scheb/two-factor-bundle/blob/master/Security/TwoFactor/EventListener/RequestListener.php#L84 this line is executed for every request that is not ignored using excludePattern (and that should be every url that is protected by symfony firewalls).

Otherwise the

that does not trigger 2FA

needs some clarification.

[umpirsky]

@zerkms Yes, that line is executed, but not the scheb_two_factor.trusted_filter.

[zerkms]

@umpirsky

$this->authHandler on that line is defined as <argument type="service" id="scheb_two_factor.trusted_filter" /> (at https://github.com/scheb/two-factor-bundle/blob/master/Resources/config/listeners.xml#L18)

Have you tried to run a debugger and confirm what you say?

I'm not in the office now so I cannot take a screenshot of a stack trace, not sure why you don't simply try it and see :-( Seriously, we are wasting time: performance optimisation is about precise stuff: everything must be proven, there must be no place for suggestions and guesses (and jeez - especially random changes, which that change surely is). I have made a statement that I have checked and confirmed with a debugger. It's not constructive if I spend dozens of comments more begging to revert a useless change (that not only makes nothing faster but slower for most of use cases this bundle was designed for).

[umpirsky]

@zerkms You are right, I will check again when I find some time and post results here. Now, I only remember that moving this to lazy fixed the issue for me.

[zerkms]

Now, I only remember that moving this to lazy fixed the issue for me.

It could not have - lazy does not reduce the amount of job to be done (it actually increases both cpu and memory footprint in case if the service is used, and it is used in almost every request).

So I believe it is either placebo or some issues with capturing the performance metrics.

[umpirsky]

Issue is still here, working on a fix...

[umpirsky]

@zerkms #66