Move include statements from DrupalKernel::boot() into DrupalKernel::preHandle()

Some more numbers, based on my testing, a page cache response in HEAD is 4.2ms (with the page cache no config thing enabled), with #2392433: Stream wrappers are registered before page cache, the same change as here + the removal of the injected logger from ControllerResolver (this part will be part of the next patch), it is 3.1ms

With all those changes, we're executing 2 queries, for the page cache entry and the cache tags checks for it.

That's an improvement of 25%.

99% of this patch is trivial moving around of code. If testbot says it's green, and I can still install Drupal manually, then it's good to go.

The only potentially contentious change is this one:

+++ b/core/core.services.yml
@@ -400,7 +400,7 @@ services:
-    arguments: ['@class_resolver', '@logger.channel.default']
+    arguments: ['@class_resolver']

This is okay because it is only necessary for this code in \Symfony\Component\HttpKernel\Controller\ControllerResolver:

    public function getController(Request $request)
    {
        if (!$controller = $request->attributes->get('_controller')) {
            if (null !== $this->logger) {
                $this->logger->warning('Unable to look for the controller as the "_controller" parameter is missing');
            }

… which is done only for DX, but that in fact also happens in HttpKernel::handleRaw():

        // load controller
        if (false === $controller = $this->resolver->getController($request)) {
            throw new NotFoundHttpException(sprintf('Unable to find the controller for path "%s". The route is wrongly configured.', $request->getPathInfo()));
        }

So if this costs us performance, and we have an equivalent to maintain a good DX, this doesn't actually change anything.

Should we remove the logging code in ControllerResolver completely? If we never inject the logger, then keeping that code is a bit pointless, especially as we already deal with it one layer above?

That's a good point. Minor, but good. Let's do that.

Yes, exactly like that. Thanks!

+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -436,6 +419,22 @@ public function getContainer() {
   public function preHandle(Request $request) {
+    require_once $this->root . '/core/includes/common.inc';
+    require_once $this->root . '/core/includes/database.inc';
+    require_once $this->root . '/core/includes/path.inc';
+    require_once $this->root . '/core/includes/module.inc';
+    require_once $this->root . '/core/includes/theme.inc';
+    require_once $this->root . '/core/includes/pager.inc';
+    require_once $this->root . '/core/includes/menu.inc';
+    require_once $this->root . '/core/includes/tablesort.inc';
+    require_once $this->root . '/core/includes/file.inc';
+    require_once $this->root . '/core/includes/unicode.inc';
+    require_once $this->root . '/core/includes/form.inc';
+    require_once $this->root . '/core/includes/mail.inc';
+    require_once $this->root . '/core/includes/errors.inc';
+    require_once $this->root . '/core/includes/schema.inc';
+    require_once $this->root . '/core/includes/entity.inc';
+

I'm wondering if we should move this into a method so that if we ever get to the point that Drush does not need to fake a request it can do something cleanly. Tying all this includes into the internal implementation of preHandle feels wrong.

RTBC +1 once #17 is addressed, a helper function makes a lot of sense.

+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -433,9 +416,33 @@ public function getContainer() {
+   * Load legacy include files.

This should now become {@inheritdoc}.

Re #17: Not objecting, but it is still the case that (among others) the UrlGenerator expects a RequestContext and just fatals if it's not set, so I think Drush (and any other external scripts) not having to call preHandle() is a pipe dream at this point. We just have to accept the fact that preHandle() is the equivalent of DRUPAL_BOOTSTRAP_FULL now...

Updated summary.

xpost that somehow deleted @dawehner's patch :(

Wow, according to the issue summary, this sounds great! :) Moving over to catch for sign-off since it deals with bootstrap performance.

This looks good, although loading those files on page cache requests is a bad regression from Drupal 7 so it's sadly not an improvement as such.

This doesn't need profiling, just common sense.

Committed/pushed to 8.0.x, thanks!

Note: see #1 and #6 for profiling. This was also profiled as part of #2392433: Stream wrappers are registered before page cache, which is where this problem was discovered IIRC.