This removes the major performance regressions blocking #1599108: Allow modules to register services and subscriber services (events) from going in.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

effulgentsia’s picture

Crell’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -9,42 +9,59 @@ namespace Drupal\Core;
-      $class = "\Drupal\\{$module}\\{$camelized}Bundle";
-      if (class_exists($class)) {
+      $class = "Drupal\\{$module}\\{$camelized}Bundle";
+      // @todo FALSE is passed so as not to incur autoloader overhead for
+      //   modules without a bundle class. However, this requires the module
+      //   to require_once its bundle class. Remove this requirement when it's
+      //   possible to do so performantly.
+      if (class_exists($class, FALSE)) {
         $bundles[] = new $class();
       }

I am very much not OK with this. This is a premature optimization. Once we have the DIC compiled, this problem goes away anyway. And we're not shipping without an uncompiled DIC. This is a premature optimization that harms DX for all the wrong reasons. We really need to stop doing that.

It's really hard to tell how invasive the compiled-vs-not switch is with this patch, since it also mixes in all sorts of comment fixes. Can we get those separated out to see how much more work we're making for ourselves (since we know we're putting the compilation back in anyway, which will resolve the issue)?

effulgentsia’s picture

Title: Remove DIC compilation and bundle class autoloading until they can be optimized » Remove DIC compilation until it can be dumped to disk
Status: Needs work » Needs review
FileSize
6.46 KB

Ok, here's the minimal patch for removing the compile step. I split off bundle class autoloading into #1716048: Do not boot bundle classes on every request, and can submit related cleanups at a later time once a decision is made on these two directions.

effulgentsia’s picture

This is a premature optimization that harms DX for all the wrong reasons.

The question of how to balance interim DX annoyances vs. interim performance regressions I leave to Dries and catch. However, we don't know how long it will take to resolve #1706064: Compile the Injection Container to disk. A week? A month? In the meantime, adding 30% to requests is also a harm. It creates a larger denominator when benchmarking other issues, potentially leading to those issues getting committed with a perception they cause less performance harm than they really do. It also confuses people who aren't tracking the WSCCI issues in-depth as to why D8 HEAD is so much slower than D7 and may lower their morale. We want to keep everyone who works on D8 moving smoothly, which means unblocking incremental WSCCI patches as much as possible, while simultaneously keeping HEAD in a state that allows for smooth progress elsewhere.

katbailey’s picture

Status: Needs review » Fixed

Committed to the bundles branch and included in the latest patch at #1599108: Allow modules to register services and subscriber services (events).

Status: Fixed » Closed (fixed)

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