Spin-off from #1715940: Remove DIC compilation until it can be dumped to disk. Symfony's kernel calls registerBundles() every request, not only when rebuilding the container. That's because Symfony's BundleInterface supports a boot() method. If DrupalKernel's registerBundles() implementation invokes the autoloader for a bundle class in every enabled module on every request, that adds a substantial amount of time (>5%) to every request.

Some options:
- Skip the autoloader, but that requires the .module file to explicitly load its bundle class.
- We decide to not support a boot() method in Drupal, in which case, we can limit loading bundle classes to only during container rebuilding.
- We implement autoloader caching (for non-APC as well as APC).
- We implement our own custom registerBundles() caching.

This patch does the first as an interim step until we implement one of the other 3 options.

Files: 

Comments

Crell’s picture

Status:Needs review» Needs work

I am very -1 on the first option, as it bypasses the whole point of PSR-0 (easy autoloading) and is a DX issue. That's also a premature optimization until we know how common it will be for modules to have bundle classes in practice. We don't know that yet.

If every/most modules have a bundle class, then we're adding one additional autoload hit per module. That's chicken feed. Views (whether it gets into core or stays in contrib) has hundreds of classes. (Tim Plunkett just counted for me and it stands at 325 right now.) Special casing this one specific class of, er, classes is dumb.

We don't need a special case optimization for bundle classes. We need a general solution to improving autoloading speed, which would improve all cases, not just this one. There are other discussions already happening in other issues about that.

This is premature optimization, plain and simple.

effulgentsia’s picture

Title:Don't autoload bundle classes until that's optimized» Optimize DrupalKernel::registerBundles()
Project:WSCCI» Drupal core
Version:» 8.x-dev
Component:Code» base system
Priority:Normal» Major
Issue tags:+Performance
catch’s picture

Views might have hundreds of classes, but they don't all get loaded every request.

100-200 bundle classes is probably going to be as many classes as are currently loaded in 8.x in total during a request at the moment, give or take. I'm sure that number will go up but let's not write off 5% performance regressions as chicken-feed - if I put a 100kg bag of chicken feed on your back you would not walk very fast.

While I agree that we should focus on generic autoload performance improvements (and avoiding regressions in the autoloader performance we already have), this is a specific area where we know there is additional autoload overhead without any real functionality being added yet, so it deserves to be tracked separately even if it's eventually fixed elsewhere.

catch’s picture

Just to confirm, registering bundles currently accounts for a third of autoload calls in core at the moment, 32 out of 91 calls to Symfony\Component\ClassLoader\UniversalClassLoader::loadClass are from class_exists() and 29 of those are from Drupal\Core\DrupalKernel::registerBundles. So I actually overestimated the number of classes used on a normal request to core by about 150.

catch’s picture

One thing I noticed while profiling, every class_exists() call that fails is currently hitting the registry. _registry_check_code() is then taking around 2ms for all of those missed classes.

So, we should be able to reduce the impact of a failed class exists check (which all of these are at the moment) by just removing the registry #1541674: Remove the registry. Another reason to do that sooner rather than later.

catch’s picture

While I'm here, it's worth noting that a failed class_exists() call with a PSR-0 autoloader equates to a failed file_exists()/is_file() call, and failed file_exists() calls are not cached by PHP:

http://php.net/manual/en/function.clearstatcache.php

You should also note that PHP doesn't cache information about non-existent files. So, if you call file_exists() on a file that doesn't exist, it will return FALSE until you create the file.

So we should never knowingly introduce file_exists() calls on runtime, much less if the file might legitimately not exist, that's the same reason as #1055856: Optimize settings.php discovery except the impact here is an order of magnitude worse.

catch’s picture

Category:task» bug
effulgentsia’s picture

My favorite option so far from the issue summary is the 2nd one: not load bundle classes at all unless we're rebuilding the container. We need #1675260: Implement PHP reading/writing secured against "leaky" script to land first, but I think that patch is pretty close to ready.

Also, would be great to get lsmith77 or someone else familiar with Symfony to comment on whether skipping boot() calls on bundle classes is a bad idea. Seems to me like we can do all we need by letting bundle classes add a 'boot' event subscriber (and probably very few will actually need that) rather than needing to check existence / load a class for every module when 90+% won't need it.

katbailey’s picture

Just had a bit of a discussion about this in irc with Crell:

[09:25am] katbailey: chx: Crell: fwiw I have yet to see an example of a symfony bundle that overrides the boot() method - looked at the bundles in symfony-standard and all the ones listed here https://github.com/symfony-cmf Also, this would not be the first symfony-bundle-related thing we are not supporting in Drupal - we don't support bundle extensions either, because they rely on the Config component
[09:26am] Crell: I am fine with skipping it for now and documenting such, provided we aren't also saying that we can never use it if we did find a use in the future.
[09:26am] Crell: Like, say, would that be the proper place to port hook_boot(), rather than a request listener?
[09:32am] katbailey: Crell: well if you're talking about the far future, as in D9, then we can always revisit this - and I'm thinking that moving module hook implementations to something that belongs in bundles is in the far future, right? For D8, the only discussion around hook_boot() will probably be about where to invoke it from, i.e. kernel::init() or kernel::boot()
[09:35am] Crell: katbailey: hook_boot and hook_init() need to die now, in Drupal 8.
[09:35am] Crell: They need to not be hooks.

catch’s picture

I'd forgotten this issue existed.

I'd always assumed we'd remove hook_boot() (if we even need it, we were discussing completely removing any PHP execution when pages are returned from a PHP page cache except settings.php at one point, and that's the only reason anyone could think of to have hook_boot() at all). hook_init() feels like a request listener, don't really see what the pseudo-hook hard coded method name gets us at all.

Crell’s picture

If the consensus is to move hook_init() to "you've got request listeners, use 'em" and move hook_boot() to "Alas poor hook, I knew it well, Horatio!", then I don't fully see the point of boot() either.

catch’s picture

Status:Needs work» Postponed

Postponing this on #1831350: Break the circular dependency between bootstrap container and kernel - assuming that issue gets committed with its current approach, it also fixes this one.

effulgentsia’s picture

Title:Optimize DrupalKernel::registerBundles()» Do not boot bundle classes on every request
Status:Postponed» Fixed

Looks to me like that other issue fixed this one, though by overriding boot() to not call registerBundles() rather than by optimizing registerBundles() itself, so retitling.

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