All stack middlewares are constructed at the same time even for cached pages

I think it is worth a try to use:

lazy: true

for the low priority middleware's for now, the proxy should have only 2-3 function calls overhead per middleware, which should be acceptable.

Hrm, you said "1.5 ms for cached responses". Making responses served by the Page Cache 1.5 ms faster would be *huge*.

But when you talked to me in person just now, it seemed you said that it doesn't seem to be worth it. So now I'm confused :)

Raising to major if the 1.5 ms win is correct.

To clarify: saving 1.5 ms would be a 22% gain. If it's just 1 ms on my machine, then it'd be a 15% gain. Both would be great wins.

#4: It is especially HUGE if you use a non-DB backend (e.g. pure APC), because you then save the 1.6 ms of PDO construction, too it seems ...

Correct! That time would be saved even when only the page cache's cache back-end would not be in the DB.

for the low priority middleware's for now, the proxy should have only 2-3 function calls overhead per middleware, which should be acceptable.

Well, for debugability I'd propose to mark the one before page cache not as lazy, as otherwise you just have one more level in all of the call stacks.

#7: Good point!

For those not used to reading xhprof-kit reports. The SUM usually is a sum of 100 (so divide by 100) and znerol did put the patch first then the core, so we need to reverse the reading, so the above means we save:

10.9% function calls == 229 function calls
21.6% wt == 2 ms, on average

We only have 26 function calls more due to the proxying, which is 0.45 ms on average or 0.3% of 169 ms that a full bootstrap takes.

That both sounds acceptable.

CNW for moving the priorities.

I agree < 0 ==> lazy as guideline, >= 0 is not lazy.

One downside of making them lazy I guess is that it makes the stacktrace a bit deeper. I think it adds at least one or two nested methods per middleware, so that's a bit unfortunate but probably acceptable.

In my patch I declared as lazy only these 2 services which have chance to be used after page cache.

Result of profiling is attached as well, we're getting 8% in average in speed. According to xhprof we have reduced number of class instantiation via Composer\Autoload\ClassLoader::loadClass exactly by 2, from 59 to 57

@znerol: please assign the issue to yourself next time. @dpovshed was doing the same work as you! (Only necessary during sprints.)

I propose to move the page cache middleware to priority 0 and push low-priority ones to negative values. That way we'd have an easy indicator whether or not lazy should be present on the service definition.

+1

We only have 26 function calls more due to the proxying, which is 0.45 ms on average or 0.3% of 169 ms that a full bootstrap takes.

"Only 26" and "only 0.45 ms"? That seems a lot?

One downside of making them lazy I guess is that it makes the stacktrace a bit deeper. I think it adds at least one or two nested methods per middleware, so that's a bit unfortunate but probably acceptable.

That's why @dawehner requested to not make the ones before the page cache middleware lazy in #7.

We need tests for that as well.

+++ b/core/core.services.yml
@@ -567,6 +567,7 @@ services:
   http_kernel.basic:
     class: Symfony\Component\HttpKernel\HttpKernel
     arguments: ['@event_dispatcher', '@controller_resolver', '@request_stack']
+    lazy: true

Should we not use -50 for that one?

--

Yes, tests should check that any middleware marked with a priority of < 0 is lazy?

Tentatively raising this to Critical, because it meets the >1ms savings for page cache threshold in #1744302: [meta] Resolve known performance regressions in Drupal 8. I don't know that it meets the would need to be deferred to 9.x threshold: I don't think we should change service priorities in patch-level releases, but I don't think we've had a discussion yet on whether we can in minor releases.

We only have 26 function calls more due to the proxying, which is 0.45 ms on average or 0.3% of 169 ms that a full bootstrap takes.

It's a shame that making 3 services lazy adds that much overhead when internal page cache is not used. And if that's really what it is, it means that for sites using Varnish or other external proxies, overhead is added to cache misses with no compensating benefit on cache hits. And in http://wimleers.com/blog/drupal-8-page-caching-enabled-by-default, Wim mentions that even some cheap hosting plans offer Varnish, and that might increase during the lifetime of 8.x.

So, what if instead of adding lazy: true in the YAML files, we make page_cache module implement a compiler pass to add the lazy flag to all <0 priority middlewares? In other words, only add the proxy overhead when the module that benefits from it is enabled.

So, what if instead of adding lazy: true in the YAML files, we make page_cache module implement a compiler pass to add the lazy flag to all <0 priority middlewares? In other words, only add the proxy overhead when the module that benefits from it is enabled.

I like that idea a lot. Also removes the burden of having to remember to set lazy: true in the first place. We should apply that strategy also for paramconverters etc.

So, what if instead of adding lazy: true in the YAML files, we make page_cache module implement a compiler pass to add the lazy flag to all <0 priority middlewares? In other words, only add the proxy overhead when the module that benefits from it is enabled.

I like that idea a lot. Also removes the burden of having to remember to set lazy: true in the first place. We should apply that strategy also for paramconverters etc.

Just brainstorming a name for the flag: commonly_skips_delegation? Reprioritizing back to Major if this approach can be successful without changing existing priorities, since that means this could be solved in a minor or patch release if necessary.

What about:

lazy_load_remaining: true

Or:

lazy_load_delegates: true

Or:

lazy_load_successors: true

Or:

make_delegates_lazy: true

etc.

+1 for lazy_load_delegates: true

I think a term in the line suggested by znerol will be more explicit because it implies a purpose, not just a technical behavior.

In this line of thinking, from a DX PoV, terms in the (early|first|quick)_responder vein seem more appropriate, because an active behavior (intercepting the request) is associated with a boolean TRUE value, whereas a term like forwarder associates the active behavior with a boolean FALSE, which is not consistent cognitively.

And to complete selection, first is an absolute, which may be wrong since several middlewares may have this behavior and only one of them can be first, while quick introduces an element of performance relative to the middleware itself, which is not necessarily true.

In that light, early_responder seems the best choice.

/me exits bikeshedding mode

I'm fine with responder, its at least clear what it does.
It would be though great in some docs to explain what a responder does different compared to other middlewares ... and doesn't that mean we should mark the main http kernel as responder?

Otherwise this looks pretty nice!

I'm fine with responder, its at least clear what it does.
It would be though great in some docs to explain what a responder does different compared to other middlewares ... and doesn't that mean we should mark the main http kernel as responder?

Otherwise this looks pretty nice!

Ups, you are right.

One please where we could place that documentation could be the top of \Drupal\Core\DependencyInjection\Compiler\StackedKernelPass
but #2264047: [meta] Document service definition tags seems to suggest maybe a different form.

@znerol
I guess we don't need tests anymore, right?

RTBC! Docs looks great!

The variable is called $first_responder , even though the tag is just responder. Would be great if we can quick-fix that, thanks.

Yes, $first_responder is right as we explicitly search for the first responder in the chain.

This looks awesome!

According to my profiling we could potentially could shave off 1.5 ms for cached responses if we do not construct low priority middlewares.

Is this number still accurate? If nobody else does, I'll profile it.

Benchmarked, 1000 requests ($ ab -n 1000 -c 1 -g ab.tsv http://tres/).

Before

Requests per second:    146.47 [#/sec] (mean)
Time per request:       6.827 [ms] (mean)
Time per request:       6.827 [ms] (mean, across all concurrent requests)
Transfer rate:          1731.07 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:     5    7   0.4      7      10
Waiting:        5    6   0.4      6       9
Total:          5    7   0.4      7      10

After

Requests per second:    157.93 [#/sec] (mean)
Time per request:       6.332 [ms] (mean)
Time per request:       6.332 [ms] (mean, across all concurrent requests)
Transfer rate:          1866.52 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:     5    6   0.4      6       9
Waiting:        5    6   0.4      6       8
Total:          5    6   0.4      6       9

Conclusion

Median decreased by 1 ms

0.5 ms/request faster on average

Max is 1 ms lower

Committed 9166525 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

Ahh yes, thanks for the clarification!