Problem/Motivation

As research in #2407177: Profile to determine which services should be lazy has shown, we initialize a couple of services, without actually using it.

Proposed resolution

Let's mark them lazy.

Doing so yields a nice performance win — quoting @dawehner in #2407177-24: Profile to determine which services should be lazy:

Did some quick profiling for the following usecases:

I'm have profiled the entire request, with opcache as well as the apcu based classloader enabled.

Absolute numbers

Scenario HEAD PATCH
Default frontpage, no content, anonymous user 254ms 236ms
admin/report/status/php, anonymous (bootstrap + phpinfo()) 31ms 28.3ms
REST request: /node/1 with hal_json 68.4ms 65.7ms

Remaining tasks

None.

User interface changes

None.

API changes

None.

CommentFileSizeAuthor
#4 interdiff.txt775 bytesdawehner
#4 2454287-4.patch5.15 KBdawehner
#1 2454287-1.patch5.91 KBdawehner
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Active » Needs review
Parent issue: » #2407177: Profile to determine which services should be lazy
FileSize
2454287-1.patch5.91 KB

Here is the last patch from the other issue.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented 
+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -389,9 +388,6 @@ public function boot() {
-    // Start a page timer:
-    Timer::start('page');

We never called Timer::stop(), so this is probably okay. OTOH it makes it more difficult for e.g. webprofiler.module to do its work?

I suppose this is to avoid loading this class. That makes sense. But it seems out of scope WRT the issue title?

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Issue summary: View changes
Issue tags: +Performance

Updated IS to include profiling data.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
FileSize
2454287-4.patch5.15 KB
interdiff.txt775 bytes

We never called Timer::stop(), so this is probably okay. OTOH it makes it more difficult for e.g. webprofiler.module to do its work?

Devel used to use it, BUT, they can also use one of the really early events being fired (which is what webprofiler is afaik doing). The usecase of devel is not to profile the bootstrap but rather to give a time for the full page request, so the time difference from super early bootstrap to first even fired, is not a lot.

I suppose this is to avoid loading this class. That makes sense. But it seems out of scope WRT the issue title?

Alright, reverted that bit, but yeah I think its simply not something you really need.

Fabianx’s picture

Fabianx CreditAttribution: Fabianx for Drupal Association commented
Status: Needs review » Reviewed & tested by the community

RTBC, lets add some lazyness!

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

RTBC +1

#4: thanks for that explanation. If you prefer, I'm fine with doing that hunk in this issue, since devel was the only user, and it now has an alternative.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

#4: thanks for that explanation. If you prefer, I'm fine with doing that hunk in this issue, since devel was the only user, and it now has an alternative.

As fabian said yesterday, the problem is actually not the classloading (if you have opcache enabled) but the construction of the objects itself,
so I think keeping it in here is totally fine, but in general I'd also like to remove it in order to clean up DrupalKernel a bit.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

Sounds good :)

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Fixed

Zero disruption and good for performance - what is not to like!. Committed 23ed78e and pushed to 8.0.x. Thanks!

  • alexpott committed 23ed78e on 8.0.x 
    Issue #2454287 by dawehner: Make a couple of services lazy

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.