Compile the Injection Container to disk

Just putting this here for when #1675260: Implement PHP reading/writing secured against "leaky" script lands ;-)

Oops. I thought I was following this issue, but I guess I wasn't, so I missed #1, and added an alternate implementation in #1675260-69: Implement PHP reading/writing secured against "leaky" script. About half of it is the same, but at least a few hunks from #1 do some fancier Symfony kernel integration, so not closing this as a duplicate yet.

Discussed with effulgentsia this morning - this issue should not be committed unless the patch provides for a way to compile in development, deploy to production, and disable any writing of PHP in production.

OK, rerolled with the actual PhpStorage component that got in. I've also ripped out the stuff about metadata and file resources that I had borrowed from the Symfony implementation in the previous version of the patch, as well as the cache warmer stuff, because we need to figure out the whole compiled container invalidation issue and then we'll know what makes sense to borrow from Symfony.

So this is a very basic implementation and we'll still need to:
- figure out the logic around compiled container freshness (e.g. when modules are enabled/disabled that provide bundle classes)
- write tests

What about this?

Some cleanup and a very important fix: each test method needs its own container class or the world ends (my Apache segfaults for eg.). So the important change is:

    $class = $this->getContainerClass();
    $class_original = $class;
    $prefix = 0;
    while (class_exists($class)) {
      $class = $class_original . $prefix++;
    }

When you switch from the protected implementation to FileStorage one, it crashes: the protected implementation will write a SomeClass.php/ folder, with the protected file inside, and the FileStorage::exists() method will return TRUE on this folder detection if you switch meanwhile, and crash because it won't be able to either load it or delete it. This might be the source of testbots confusion if the php folder is not erased between various tests.

Yeah, we need to patch pifr to chmod recursively much like core does. That has nada to do with the fails in #10 thought, that is about drupalGetTestFiles scanning public:// and file_scan_directory freakin' out at the dirs it cant access. Trivial to fix. I have also moved the "make the class unique" logic into getContainerClass where it belongs but that's no functional change. Should be ready to go.

+++ b/core/includes/bootstrap.inc
@@ -2400,22 +2400,24 @@ function drupal_get_bootstrap_phase() {
- * @param $reset
+ * @param $new_container
  *   A new container instance to reset the Drupal container to.
+ * @param $load
+ *   Whether to create a new container.

Name of 2nd param is $reset. Descriptions of both params is confusing.

+++ b/core/includes/file.inc
@@ -2025,7 +2025,7 @@ function file_scan_directory($dir, $mask, $options = array(), $depth = 0) {
-  if (is_dir($dir) && $handle = opendir($dir)) {
+  if (is_dir($dir) && $handle = @opendir($dir)) {

A comment explaining why we need to suppress errors here would be nice.

+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -61,15 +63,39 @@ class DrupalKernel extends Kernel {
+    if (!$storage->exists($cache_file)) {
+      $container = $this->buildContainer();
+      $this->dumpDrupalContainer($cache_file, $container, $class, $this->getContainerBaseClass(), $storage);
+    }
+
+    $storage->load($cache_file);
+
+    $this->container = new $class();

Can we avoid a potentially expensive $storage->exists() call here by just doing a load() and then checking class_exists()?

Also, it would be nice if this could be robust to a storage implementation that can't write: i.e., if after load(), class_exists() returns FALSE, then set $this->container to $container.

Also, I think module_implements_reset() (and maybe drupal_flush_all_caches()) needs to do a $storage->delete().

+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -61,15 +63,39 @@ class DrupalKernel extends Kernel {
+  protected function getContainerClass() {
+    $class = parent::getContainerClass();
+    $class_original = $class;
+    $prefix = 0;
+    while (class_exists($class)) {
+      $class = $class_original . $prefix++;
+    }
+    return $class;
+  }

Even better than this kind of prefix (ahem, suffix), would be if we could get a hash of the enabled modules, since the container should always be the same for a given set of enabled modules. If that's expensive, perhaps we can cache that hash?

each test method needs its own container class

If the class name is unique to the list of enabled modules, then a class_exists() prior to load() should be sufficient, I think. If the class is already in memory for a subsequent test run, we should be able to just use it.

I also noticed that on my box, the protected files chmod set strange mod bits, and my os displays "? ??? ? ? ??" on ls -l which is not a good sign.

Can we avoid a potentially expensive $storage->exists() call here by just doing a load() and then checking class_exists()?

I agree.

Also, it would be nice if this could be robust to a storage implementation that can't write: i.e., if after load(), class_exists() returns FALSE, then set $this->container to $container.

I'm not sure if this is linked, but I got bugs due to failed file write and I was going WSOD because of not existing container. class_exists() calls are mandatory here to provide a fallback behavior.

Thanks for the very insightful review, effulgentsia! The results are a lot, lot cleaner I hope.

#15, the chmod we use are bog standard, if it does not work for you, that's a separate issue, please file a proper bug report.

I didn't try to reproduce the failure I experienced yet with #17 but I think it's better reading the code, it provides an ultimate fallback in case the container class does not exists which should fix it. This fallback case is not something that should be silent IMO, I don't know if we have a logger yet this early during bootstrap, but it might worth it to log it somewhere that the container did a rebuild, and that is not something that should happen in normal/production runtime.

Also, I think module_implements_reset() (and maybe drupal_flush_all_caches()) needs to do a $storage->delete().

I do not agree for the module_implements_reset(), but regarding drupal_flush_all_caches(), it is sometime revelant, sometime not. I don't think you want to mess up with generated PHP files on a production server, even when you clear the caches (which can arbitrarily triggered by some modules, while compiling really make sense to be triggered when you enable or disable modules, but not really when you flush the caches).

I do think that the cleanup is a followup too :) #1759582: Figure out when to delete compiled DIC files the amount of garbage left behind is really small, these are like 10K files, even a couple hundred of them takes no significant disk space.

Indeed. As I said #17 looks better, needs some love and testing, I'm quite sure there are use cases we didn't think about, such as, for a example, a "production" or "development" boolean in configuration to disable compilation (for devel environments).

If you look at the changes it's rly hard to swap between compiling and not compiling. The entire logic is different. Even the bundles need to be build differently. So I don't see a possibility for such a switch. Why not a drush command that refreshes the compiled container? Or a UI button in the performance settings.

All you have to do is skip the load and fallback in building the compiler at runtime the exact same way it's being done (and not dumping it).

Thats *not* true
1) Adding subscribers differs if you compile or not
2) Compilerpass systems don't work if you don't compile
3) ....

It's more than just "to compile or not to compile"
Symfony wants you to compile the container they build a lot of their logic on top of that principle.

Why would we else have those ugly workarounds in code?

An example from the patch

$container->register('bundle_test_class', 'Drupal\bundle_test\TestClass')
       ->addTag('kernel.event_subscriber');
-
-    // @todo Remove when the 'kernel.event_subscriber' tag above is made to
-    //   work: http://drupal.org/node/1706064.
-    $container->get('dispatcher')->addSubscriber($container->get('bundle_test_class'));

Those two things do the same thing but one is based on compiling the other is not.

EDIT:
Maybe I am confused by your comment. Do you propose to compile the container and just not dump it? In that case it is possible but I'm not sure that is a good idea either (but I don't have any technical reasons for that)

Ok, I did not use the right word, by compiling I meant "compile AND dump it". I meant no dumping to file of course.

EDIT: Real definition of a compiler is a program that transform any language code to any other language code. Symfony abusively uses the term compiler, because if you disable the dumping phase, you are not generating code in another language, but just manipulating objects into memory. Plus Symfony Compiler actually do not parse any file (in our use case particularly) because all the symbols are generated using PHP (see Drupal\Core\DependencyInjection\Compiler\RegisterKernelListenersPass::process() for example). So you are just doing a subset of compilation. That's what brought my wrong wording that brought confusion (wrong according to Symfony terminology). See http://en.wikipedia.org/wiki/Compiler

Maybe I am confused by your comment. Do you propose to compile the container and just not dump it? In that case it is possible but I'm not sure that is a good idea either (but I don't have any technical reasons for that)

Some Symfony developers actually do not dump their compiler at all during active development. Let me give you a user story: As a developer, I don't care about clicking a button to clear my cache, I just want no cache at all.

Then use a storage backend which is not writeable. One is even included in core. It will rebuild on every page load. It'll be slower but it'll do just that.

What Alex meant is this: because the classname is not dependent on the list of modules we need to either delete the file (so it gets rebuilt) on module changes or we need to make it the name of a hash of enabled modules. I did the latter, resolving a @TODO in DrupalKernel while doing so.

Rerolled against HEAD.

The only change is that I needed to add a character in front of the hash because it might start with a number and that's not a valid PHP identifier.

#33: 1706064_31.patch queued for re-testing.

I have one question though, why hashing the compiler name using the module list?

Because the DIC only depends on the bundles available which depends on the modules.

Something that hits me is that the module list is not supposed to change on a site. The only cases where you don't have the same module list could be in CLI scripts, or during install and update. In those cases, and because you use them only for maintainance tasks, I'd prefer not having a dump at all instead of dumping a new file. That's a matter of taste I guess, but I think ofuscating that much the file name sounds a bit too much IMO, because if the module list change, you won't reuse the old kernel ever, so you'd better delete it and create a new one with the same name.

Not everyone uses CLI. For them the module list will absolutely change on module enable and disable. You might want to say that "hey let's just write a new DIC on module enable" but the timing of that is insanely tricky -- there is an already existing issue about the DIC and module_enable so I wanted to desperately avoid that here. You might call it obfuscating the flilename, but let's face it, that's hardly something you need to bother with -- you will never edit it or anything. And, if as you say, it's not changing anyways, you will know that the hash-named file in the backtrace is the DIC, anyways.

Then comment it in the patch please, it seems arbitrary when just reading the code as-is.

Added.

Anyway your explaination is fair enough, I agree to keep the hash.

Possibly needs a doc review. I found these tiny issues. Is there anything that has to be done before we can get this in?

a new kernel.. On the oth

start with a letter. and hashes don't

+ // While the default Synfomy class name only depends on the environment,

+ // While the default Symfony class name only depends on the environment,

;)

This fixes #45

But not #44 it seems , at least the kernel two dots is still in there.

+++ b/core/lib/Drupal/Core/DrupalKernel.phpundefined
@@ -61,11 +71,37 @@ class DrupalKernel extends Kernel {
+    // While the default Symfomy class name only depends on the environment,

Symfomy -> Symfony

fix #50

all doc fixes from #44

#52: 1706064_compile_the_injection_container__52.patch queued for re-testing.

#55: 1706064-compile-the-injection-container-55.patch queued for re-testing.

I asked katbailey her opinion and she had 2 valid questions:

1) Do we ant to compile while testing?
2) Do we test the DIC compiling process itself somewhere?

The whole idea in testing is having a mock container with only the services needed for the test (especially in unit testing), we shouldn't compile that, and use a basic container (no compiler pass, etc...).

For functional tests, we have two containers, one is the testing site/script and the other is the tested site. The interesting one is the tested site's one, it should be compiled normally after the fake site install as it would be on a normal site so we ensure tests are the closest possible to what happens in real life.

Does that make sense?

We do not have a non-compiling version any more, or that seems to be the point? However, there are twice as many test files (257 : 493 ) NOT containing the string 'static $modules' than those containing it and so the question rises -- can we get a major speedup by pre-creating the DIC for profiles and copying it in place?

My feeling is that DIC will change and be different for each test. If that was possible, I'd say yes, but we would end up by mocking up a false site that diverge from a normal site and tests wouldn't be 100% reliable. Each test is different and builds a false site accordingly to what it tests exactly.

Plus there is the unit test problem, you need to be able to build DICs for unit tests, and those shouldn't be compiled since the unit test would be run only once and each unit test class would be different.

After patch from #1702080: Introduce canonical FileStorage + (regular) cache landed we have conflict in bootstrap.inc file. I have tried to resolve it but during installation got error:

Please add the class to service "cache.config" even if it is constructed by a factory since we might need to add method calls based on compile-time checks.

I am not quite sure how to fix this conflict.

Regarding having compiled DIC for tests we can have option in test class definition what DIC to use and ship some standard ones. So we will have immediate benefit from using already existing DIC's. Surely this will require tests to have fixed set of modules.

Or are we talking about keeping DICs of each test somewhere in separate place and if next running test builds a list of required modules and sees that DIC with same set of modules exists -- reuse it?

+ if ($container->has('request') && $container->has('content_negotiation') && $container->isScopeActive('request')) {

I think the following should be enough

+ if ($container->isScopeActive('request') && $container->has('content_negotiation')) {

Sigh. Most definitely won't be enough:

http://api.drupal.org/api/drupal/core!vendor!symfony!dependency-injectio...

Returns whether this scope is currently active

This does not actually check if the passed scope actually exists.

Oh, and here is how I found it -- I was unsure so I have thrown isScopeActive into Google and looked at the first result...

Am I very negative / hostile / etc again?

Am I very negative / hostile / etc again?

Yes, not to mention unfair.

The 'request' service is a synthetic service that is defined in the CoreBundle - so if we are dealing with the full DIC (not just the bootstrap DIC) the $container->has('request') check will always return true even when outside of request scope. And if $container->has('content_negotiation') returns TRUE then the check for the request service will certainly return TRUE.

At any rate, this certainly needs more reviews.

+++ b/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.php
@@ -673,7 +673,8 @@ abstract class WebTestBase extends TestBase {
+    drupal_container(NULL, TRUE);

TestBase::prepareEnvironment() rebuilds a fresh container already (identical call) and this call in WebTestBase::setUp() is executed only shortly after. I wonder whether we can remove this second/duplicate container rebuild?

+++ b/core/vendor/symfony/dependency-injection/Symfony/Component/DependencyInjection/Container.php
@@ -252,6 +252,9 @@ class Container implements IntrospectableContainerInterface
+              if ($id == 'request') {
+                $x=0;
+              }

huh?

If this change to Symfony's code is intended, then we should add an inline comment what the hack is doing and whether and when it will be obsolete at some point?

If this change to Symfony's code is intended

Oops. No, that was just left over from me debugging.

Then I apologize -- I will try to avoid in the future (I recognized myself, that's something). Edit: https://twitter.com/chx/status/243888508949901313

As for the code,

     $container
-      ->register('cache.config')
+      ->register('cache.config', 'Drupal\Core\Cache\CacheBackendInterface')

Why? There is a factory anyways.

Re #71, see #63. I think it has something to do with being able to reflect during compilation if method calls are added to the service definition.

So, I think it would be helpful to have a test or even test setup that set core to use FileReadOnlyStorage - so we can verify that nothing in core has a hard dependence on the success of writing out to disk.

Also, I think this needs work:

+      if ($storage->writeable()) {
+        $this->dumpDrupalContainer($cache_file, $this->container, $class, $this->getContainerBaseClass(), $storage);
+      }

I think the dumpDrupalContainer() method needs to return the value of the save(), or return FALSE at the start if the storage is not writable. I don't think the code calling dumpDrupalContainer() should have to test the capability of the storage object.

I would also think we would want some kind of log warning if the call returns FALSE.

We can get rid of writeable, sure, it's not essential since the logic rearrange a couple dozen followups back :)

@chx - I think it's ok to have that method, I just think it's being called in the wrong place. It should be called in dumpDrupalContainer()

@chx - I think it's ok to have that method, I just think it's being called in the wrong place. It should be called in dumpDrupalContainer()

looks like a WSCCI issue

It's all yours.

Taking a pass at this...

OK, this makes the changes suggested by @pwolanin in #73. I've added a unit test for testing the behavior with and without compilation (i.e. in the latter case using the FileReadOnlyStorage implementation). In order to make the kernel unit testable I changed it so that we can optionally pass in a $system_list array, i.e. as opposed to the previous version of the patch which called system_list() and passed that to the kernel. This way, for unit testing purposes you can pass in a fixed list of modules, whereas under normal circumstances the kernel will be responsible for generating that list (which after #1784312: Stop doing so much pre-kernel bootstrapping and #1331486: Move module_invoke_*() and friends to an Extensions class get figured out will not require any crazy bootstrapping).

Actually, there's no need for the unit test to get the container from drupal_container() when it can just get it directly from the kernel. We still need to muck about with drupal_container() unfortunately because it gets called from within the kernel so we need to reset it afterwards.

This looks pretty good to me. I cleaned up a couple comments, but I'm thinking this is RTBC as long as we have follow ups set up for anything missing.

#81: 1706064-compile-the-injection-container-81.patch queued for re-testing.

Symfony\Component\DependencyInjection\ContainerBuilder::merge() must be an instance of Symfony\Component\DependencyInjection\ContainerBuilder, instance of ce97b921f0da78163c923d45216c0a159f8ca74e54712683c6c372d584662dbbc

#81: 1706064-compile-the-injection-container-81.patch queued for re-testing.

#87: 1706064-87.patch queued for re-testing.

looks like the new drupal_container(NULL, TRUE); that @effulgentsia introduced in setUp() in the last patch solved some of the failures but introduced new ones :(. A lot of them are due to the config system not be initialized after that call to drupal_container(), e.g. config('search.settings')->get('active_modules') or any other call to config()->get() returns NULL even after the kernel boot.

Now that the router is in we need another compiler pass to deal with setting up the matcher services - here's an attempt at that. I also added the use of the debug flag to prevent compiling and am setting that to true during installation. For now I just want to see what testbot has to say...

Happy test bot is happy.

+++ b/core/lib/Drupal/Core/DrupalKernel.phpundefined
@@ -61,12 +76,48 @@ public function registerBundles() {
+      $cache_file = $class . '.php';
+  ¶

a small typo - all other looks good for me

+++ b/core/lib/Drupal/Core/DrupalKernel.phpundefined
@@ -76,35 +76,41 @@ public function registerBundles() {
+  ¶

Extra whitespace

Wow, I really wasn't expecting such compliance from testbot.
Ok, so I fixed up some whitespace and beefed up the unit test for DIC compilation.
What I'd love to get people looking at in particular at this stage are

• The logic of the mechanism for writing the container to php using the enabled modules hash - is this robust enough for our needs?
• The compiler pass for the matcher services that I wrote yesterday - it's a bit convoluted. Thoughts, pounard? Crell?
• The unit test for DIC compilation (core/modules/system/lib/Drupal/system/Tests/DrupalKernel/DrupalKernelTest.php) - is this sufficient?

... as well as just general reviews of course - it's just that those are the areas I feel really need scrutiny right now.

This is a really important patch that has repercussions for how we solve other problems relating to the DIC (because without it we cannot use compiler passes).

Rerolled for #1803338: 403 and 404 pages call error_log cluttering server error logs

@katbailey the module list hash sounds robust, as long as there is nothing that dynamically (following variables or such) registers services. But the whole goal of registration is to have something monolithic, so I think it's OK. As long as it is possible to delete it in order to force a rebuild, we're good to go.

I have a not about the writable() method (which probably should be named isWritable() IMHO, but that's another debate) is that the PhpStorage\FileStorage class can be non writeable depending on the directory it's working in, so I think it should return something like is_writable($this->directory) or such (I'm guessing the variable name here). It will force a stat() call I guess, but it's important to reflect reality of the OS here, else we could end up with some weird behaviors in some atypic or misdone site installs.

class RegisterMatchersPass doesn't respect coding standard (missing blank lines at the top and the bottom of the class definition).

Your compiler pass looks OK but I'm not confident enough with SF2 compiler, so I'd like more reviews to confirm that. I'm just reading the patch here not doing a full review by applying it, but I think you can omit the PassConfig::TYPE_AFTER_REMOVING? I'm not sure there but in the SF2 documentation example, which is a use case which looks a lot like ours (I might be wrong, tell me if so) they don't specify any behavior here and leave the default. If I understand it well, the goal is to register the matchers for the chain matcher? Then I guess you are doing it right! Tagging is exactly what we want here.

I'm still not convinced by the procedural system_list() call into the kernel, I think the module list should be either injected either read from the early container. I don't like having two containers, even if they get merged by compile they still are both instanciated on every bootstrap, but as long as we have it, we could inject the module list as kernel parameters when instanciating it?

I'm still not convinced by the procedural system_list() call into the kernel

Is the goal to eventually replace it with an injected Extensions service once #1331486: Move module_invoke_*() and friends to an Extensions class is resolved? In the meantime, we could possibly inject a ConfigFactory service, since as of #1608842: Replace list of bootstrap modules, enabled modules, enabled themes, and default theme with config, system_list() is just a config get, but not sure if that makes sense if we'll eventually switch to an Extensions service.

I don't like having two containers

Right. That's on the chopping block in #1784312: Stop doing so much pre-kernel bootstrapping.

After updating from HEAD, web tests were breaking with DIC compilation because the cachedstorage was getting hardcoded as e.g.

    protected function getConfig_Cachedstorage_StorageService()
    {
        return $this->services['config.cachedstorage.storage'] = new \Drupal\Core\Config\FileStorage('sites/default/files/simpletest/624382/config_active');
    }

So I've added a $rebuild parameter to the Kernel constructor which sets a 'stale' property on it and forces the cached container file to get deleted. Seems to work. It's either that or don't compile the container during test runs, I'm not sure which makes more sense... thoughts?

#104: 1706064.compiled-dic.104.patch queued for re-testing.

OK, here's a version without the stale/rebuild business - it just uses debug mode when running the tests so that the container doesn't get compiled.

Re #103, yes this will all change with #1331486: Move module_invoke_*() and friends to an Extensions class - I think we should retain the flexibility of being able to pass in a fixed list of enabled modules, but it shouldn't be necessary to instantiate anything to inject into the kernel. With the ExtensionHandler patch, the kernel is responsible for instantiating it - and if it receives a fixed list of modules in its constructor it can set that on the ExtensionHandler it creates.

This patch addresses most of #109, but it does not address "the big one" :-/ See #1706064-21: Compile the Injection Container to disk - please oh please can we leave this for a follow-up?
As to some of the other points...

However, shouldn't the mode be in the hash, not just the module list?

It now adds 'Debug' to the class name if we're in debug mode, which is what the Symfony one does - is that ok?

Why can't we just use a try-catch in this method like a normal person?

There's no actual exception being thrown here - we just want to log the fact that the container isn't compilable if we're using the read-only storage mechanism.

Why do we need to strip comments?

That was copied from Symfony code - I've ripped it out pending someone coming up with a reason why we need it.

See the interdiff for how the rest of the points were addressed.

Another change I made here is to not use the $debug property as the basis on which to decide whether or not to compile. Instead I've added a more explicit $useCompiledContainer property and added a parameter for it to the constructor, defaulting to TRUE.

I reviewed this patch and asked for more opinion in irc (mostly from chx). And they didn't see any big problems.
If #111 is done and the folowing nitpicks than I possibly can rtbc.

+++ b/core/lib/Drupal/Core/DrupalKernel.phpundefined
@@ -26,6 +28,43 @@
+   * Whether to use a compiled Container as opposed to a ContainerBuilder for
+   * the DI container.

Please shorten the firsts sentence so it fits on one line.

+++ b/core/lib/Drupal/Core/EventSubscriber/FinishResponseSubscriber.phpundefined
@@ -16,6 +17,12 @@
 
+  protected $languageManager;
+
+  public function __construct(LanguageManager $language_manager) {
+    $this->languageManager = $language_manager;

Comment these things

While on performance testing, applied the patch and saw a nice speed up in XHProf:

100 nodes, node.tpl.php from: 2064 ms down to 1965 ms

So 100ms saved (with overhead of XHProf) :-).

100 ms saved also with Twig ... Not more / not less savings.

Addresses #111 and #112.
Re #113 - nice! :-)

+++ b/core/includes/bootstrap.inc
@@ -2442,7 +2442,7 @@ function drupal_container(Container $new_container = NULL, $rebuild = FALSE) {
-      ->register('cache.config')
+      ->register('cache.config', 'Drupal\Core\Cache\CacheBackendInterface')

I don't quite understand why and how it is possible to register an interface?

+++ b/core/includes/file.inc
@@ -1441,7 +1441,9 @@ function file_scan_directory($dir, $mask, $options = array(), $depth = 0) {
-  if (is_dir($dir) && $handle = opendir($dir)) {
+  // Avoid warnings when opendir does not have the permissions to open a
+  // directory.
+  if (is_dir($dir) && $handle = @opendir($dir)) {

Won't this hide other errors, too? And what if the inaccessible $dir is the only $dir being requested? Shouldn't there be some error handling?

+++ b/core/includes/install.core.inc
@@ -1464,11 +1464,13 @@ function install_bootstrap_full(&$install_state) {
+  // @todo The first two constructor parameters for the Kernel class are for
+  //   environment, e.g. 'prod', 'dev', and a boolean indicating whether it is
+  //   in debug mode. Drupal does not currently make use of either of these,
+  //   though that may change with http://drupal.org/node/1537198.
+  $kernel = new DrupalKernel('prod', FALSE, NULL, FALSE);

I think this lengthy @todo can be removed (and should go into DrupalKernel's constructor phpDoc instead).

+++ b/core/includes/module.inc
@@ -195,6 +195,9 @@ function system_list($type) {
+      // Store a hash of the enabled modules for use when compiling the DIC.
+      $lists['module_enabled_hash'] = hash('sha256', implode(',', array_keys($lists['module_enabled'])));

+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -26,6 +28,42 @@
   /**
+   * Holds the list of enabled modules.
+   *
+   * @var array
+   */
+  protected $systemList;
...
+    if (isset($system_list)) {
+      $this->systemList = $system_list;
+    }
+    else {
+      // @todo This is a temporary measure which will no longer be necessary
+      //   once we have an ExtensionHandler for managing this list. See
+      //   http://drupal.org/node/1331486.
+      $this->systemList = system_list();
+    }

The system list holds more than enabled modules. It contains all enabled extensions, so the hash needs to be based on the entire $list, and DrupalKernel phpDoc/comments should be adjusted accordingly.

+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -26,6 +28,42 @@
+  public function __construct($environment, $debug, $system_list = NULL, $use_compiled_container = TRUE) {

Let's type-hint $system_list as array, and also add phpDoc for all the parameters here.

+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -43,9 +81,7 @@ public function registerBundles() {
+    $modules = array_keys($this->systemList['module_enabled']);
     foreach ($modules as $module) {
       $camelized = ContainerBuilder::camelize($module);
       $class = "Drupal\\{$module}\\{$camelized}Bundle";

1) The array_keys() is no longer necessary.

2) Since you're skipping the call to system_list(), I wonder who and what calls it -- we need to make sure that drupal_get_filename() is still primed with the file paths of extensions. Otherwise, that will trigger very nasty full filesystem scans.

3) This effectively adds yet another cache to system_list(), which means that its data is cached in:
- cache_bootstrap:bootstrap_modules
- cache_bootstrap:system_list
- cache_config:system.module
- cache_config:system.theme
- PhpStorage:service_container

+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -59,14 +95,57 @@ public function registerBundles() {
   protected function initializeContainer() {
...
+    if (!$this->useCompiledContainer) {
+      // In debug mode we do not use a compiled container.
+      $this->container = $this->buildContainer();
+    }
+    else {
...
+      else {
+        $this->container = $this->buildContainer();
...
+      }
+    }

The logic in initializeContainer() can be vastly simplified by reversing the if/else conditions.

+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -59,14 +95,57 @@ public function registerBundles() {
+      $class = 'c' . $this->systemList['module_enabled_hash'] . ucfirst($this->environment) . ($this->debug ? 'Debug' : '');
...
+      // First, try to load.
+      if (!class_exists($class)) {
...
+      // If the load succeeded or the class already existed, use it.
+      if (class_exists($class)) {
+        $this->container = new $class;

I think $class should be either rooted by prefixing \ or the compiled class should be in a certain namespace that is explicitly used and referenced. Otherwise, the $class name is relative and the classloader will try to find the class name in all registered namespaces.

#1708692: Bundle services aren't available in the request that enables the module merged into this one.

Wrong interdiff.

Disregard #116 / #117 I will just move to the other issue.

> I don't quite understand why and how it is possible to register an interface?

It doesn't register an interface, as it uses a factory class. AFAIK the interface is for @return documentation only.

> if (is_dir($dir) && $handle = @opendir($dir)) {
> Won't this hide other errors, too? And what if the inaccessible $dir is the only $dir being requested? Shouldn't there be some error handling?

Perhaps -- but is there a point? It was already not doing anything if opendir() failed. If errors arent displayed these warnings went into watchdog , quite possibly unseen. I added an else - watchdog.

> I think this lengthy @todo can be removed (and should go into DrupalKernel's constructor phpDoc instead).

Moved.

> The system list holds more than enabled modules. It contains all enabled extensions, so the hash needs to be based on the entire $list, and DrupalKernel phpDoc/comments should be adjusted accordingly.

Except we do not use those. We only use the bundles of modules. Edit: none the less I fixed this.

> The array_keys() is no longer necessary.

Removed. Edit: it'll be in next the patch.

> Since you're skipping the call to system_list()

But module_load_all calls module_list which calls system_list.

> The logic in initializeContainer() can be vastly simplified by reversing the if/else conditions.

Sure.

> I think $class should be either rooted by prefixing \

Done. I do not think the PhpDumper we use supports namespace.

Ah, OK.

The primary reason for all this compiling of the container is performance - however, testbot durations on the full core test suite are ranging from neutral impact to 4 minutes performance decrease (no solid data available).

Can we do some benchmarks on cold + warm environments here to make sure that the entire extra amount of code and operations is worth the buck?

Tests are supposed to rebuilt the site at almost each and every test being run (or at least for each test case class), so caching the compiler in that specific context doesn't serve any purpose (or almost none). I think this would explain why the testbot doesn't see its performances improved much.

I just want to note that we turn off the compiling for testing purposes, so the testbot wont notice any improvements. Correct me if I'm wrong and I noticed a 32 min testbot in other issues. Apparantly it depends on the bot.

Bumping priority since #1708692: Bundle services aren't available in the request that enables the module was postponed on this, and tagging since that issue blocks VDC.

If benchmarks is the only concern left then we are good to go: #1673162: Analyze performance of uncompiled and compiled DI containers. Benchmarking this right now, without having a lot of services is not resulting in a meaningful speedup. I benchmarked the frontpage with ab and it's neither slower nor faster.

Agree with the RTBC, I tested with unpatched core and core with TWIG, and as said in #113 got a 100ms speedup (with XHProf enabled) and rendering 100 nodes on /node compared to non-compiled DIC.

I think it makes sense for catch to look at this one.

Updated the issue summary. If you find any issues or feel it needs more, then please feel free to be bold.

dfsa

fdsa

Copy of the revision from October 15, 2012 - 10:47.

+++ b/core/includes/module.inc
@@ -249,6 +249,9 @@ function system_list($type) {
+      // Store a hash of the enabled modules for use when compiling the DIC.
+      $lists['system_list_hash'] = hash('sha256', implode(',', array_keys($lists['filepaths'])));

Stale comment?

+++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/RegisterMatchersPass.php
@@ -0,0 +1,25 @@
+class RegisterMatchersPass implements CompilerPassInterface {
+  public function process(ContainerBuilder $container) {

+++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/RegisterNestedMatchersPass.php
@@ -0,0 +1,25 @@
+class RegisterNestedMatchersPass implements CompilerPassInterface {
+  public function process(ContainerBuilder $container) {

As a Drupal developer who did not participate in this issue, it is not clear to me

- why this class is needed
- what it is doing
- what data it operates on
- from where it is called
- what the process() method code is doing, and why
- etc.

+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -59,14 +108,55 @@ public function registerBundles() {
+      if (!class_exists($class)) {
...
+      if (class_exists($class)) {
+        $fully_qualified_classname = '\\' . $class;
+        $this->container = new $fully_qualified_classname;

The fully qualified class name is not always used.

The primary reason for all this compiling of the container is performance - however, testbot durations on the full core test suite are ranging from neutral impact to 4 minutes performance decrease (no solid data available).

I suspect the reason for performance degradation in testbot environment is that this patch runs compiler passes, and those are very slow, because they're by definition not designed for runtime. So, running them to setup each test is quite a hit. Perhaps in a follow up, we can explore a precompiled container for the 'testing' profile, so that at least all tests that don't enable any test-specific modules can be faster?

Can we do some benchmarks on cold + warm environments here to make sure that the entire extra amount of code and operations is worth the buck?

Unlike #130, I saw a 2% improvement with this patch for the front page on a warm environment (49ms vs. 50ms). That's *in spite of* doubling the number of services CoreBundle adds to the DIC with this patch (in HEAD, all the event subscribers are not services, because of the lack of compiler pass support). While 2% is perhaps not that exciting at this time, it will grow as more services get added/used.

For cold environment, I configured my settings.php to use FileReadOnlyStorage as the PHP storage implementation and commented out $error = 'Container cannot be written to disk'; from DrupalKernel (benchmarks are useless in the presence of watchdog() statements). Here, I saw 20% performance degradation (60ms vs. 50ms) due to the slow compiler pass process. This is an edge case though: a site that wants to use read-only storage can add precompiled DICs.

Updated comment in system_list

Added missing doxygen to classes and methods. Didn't replicate Symfony handbook pages so the why / from where / etc is not

+        $fully_qualified_classname = '\\' . $class;
+        $this->container = new $fully_qualified_classname;

> The fully qualified class name is not always used.
I must be blind. I thought that the fully_qualified_classname is used immediately, unconditionally right on the next line that is even quoted.If this is a coded complaint for "we are not smashing words together' that's a valid one, changed to class_name.

I wonder if "The fully qualified class name is not always used." was meant to say "why are we calling class_exists() on a non-qualified class name"?

Can we also pass FALSE as the 2nd param to those class_exists() calls, since we don't want autoloading there?

class_exists does not take the namespace it is inside into account. It just doesn't. Run:

namespace foo;
class bar {}
var_dump(class_exists('bar'));
var_dump(class_exists('foo\bar'));
namespace bar;
var_dump(class_exists('bar'));
var_dump(class_exists('foo\bar'));

get false, true, false, true. Now, adding FALSE, sure, here it comes. And, apparently I forgot to re-run diff when I found class_name, so that's also present in this one only.

To sum up: PhpDumper does not support namespaces. Therefore these classes are always in the root. class_exists operates always as if it would be ran from root therefore it is not necessary to add a \ in front. The FQCN passed to new is not necessary either because the chances of the Drupal\Core namespace containing a class called ca4f15de4f70a3e1786208893a9ac779ebeb6d97550400a99162c84aac5718909Prod is quite low. But, I was asked so I added it. Now I wish I didn't. I didn't expect such a storm of followups :(

@#135

I saw a 2% improvement with this patch for the front page on a warm environment (49ms vs. 50ms).

Which version of PHP are you running, and fast is your machine? Working on my dev box (core i3 running on Gentoo, custom configured PHP 5.3.x, running PHP-FPM with APC, tuned MySQL connected via socket as database) a normal front page run right after install with D8 is between 100 and 150ms (3x your figures). As a comparison, it's ~70ms for D7. I will try to benchmark with this patch as soon as I have it back.

I'd be happier if someone did some before/after profiling of this with xhprof (including a compiler pass so we can see how bad it is) before it goes in. The benchmarks here aren't very useful, but profiling should show what's being saved and we can extrapolate that for when a lot more services are registered. Profiling could be done by me but not likely today.

tagging

Which version of PHP are you running, and fast is your machine?

MacBookPro6,2, i7 dual core 2.66GHz. MAMP 2.0, PHP 5.3.6, APC enabled, XDebug disabled. Benchmarking with ab -c1 -n100 (i.e., anonymous user, all caches warm (but Drupal page cache not enabled), after a new Standard profile install). And also for reference, I get 30ms on D7 HEAD vs. 50ms on D8 HEAD, so we have some work cut out for us on optimizing D8, but we know that already.

Ok, thanks, makes a lot of sense you're outrunning me that much :)

Did some benchmarks too, but the differences between each run were bigger than the improvements from this issue. Page requests take about 45-50ms here (80ms with 10 nodes listed on frontpage, that decrease worries me) with and without the patch.

@effulgentsia Did you run ab multiple times an these numbers were consistent?

As expected, not seeing a difference when page cache is enabled, because this code is never run in that case. What might be problem there is that we will add more and more stuff to the bootstrap container (database, caches, ...), any chance we can improve that too? (later, not in this issue).

Same numbers as @effulgentsia when disabling using the compiled container but always dumping it. However, testing showed that disabling/enabling the watchdog() call didn't really change anything, runs varied between 58 and 61ms, with and without that watchdog call.

Also looked at it with xhprof, but not really sure what exactly I should be looking for, some notes:
- on HEAD, ContainerBuilder::get() takes about 4-5ms (~7%) half of that is spent in createService(), if compiling really saves us most of that, then this would actually be consistent with @effulgentsia's numbers.
- With the patch, Container::get() is around ~2.6ms (~4.5-5ms), more than 50% of that time is spent in getHttpKernelService() and getRouterListenerService(), both require 10x as much time as all other calls. kernel does this: "new \Drupal\Core\HttpKernel($this->get('dispatcher'), $this, $this->get('resolver'))". Which means that it also includes the whole dispatcher stuff, which probably explains this.
- However, note that I'm still seeing 5 calls to ContainerBuilder::get(), which is the bootstrap stuff, that take up ~0.5ms (almost 1,5ms in one of 4 requests, might be a fluke). This will only go up once we add more stuff here, and we'll have to.
- dumpDrupalContainer() takes around 10-11ms (11%) here, 95% of that is in PhpDumper, but this is on a fast SSD, might be slower with a slow disk. Not sure about sites with multiple webservers, in this case, it might be save to have these files in a local file system, but will that be true for all stuff that we will put into the protected directory? If it needs to be shared, could it be that loading it from the network might be slower than building them?
- ContainerBuilder::compile() is another 14ms, but our two current compiler passes are rather irrelevant, it looks like this runs 15 default compiler passes, which make up 90% of the time or something around that. See screenshot in next comment.

@catch: Is that enough data for you?

Edit: Added another note about compile().

Attaching screenshot for the compile() execution, see above for explanation, can't upload an image to an existing comment.

However, note that I'm still seeing 5 calls to ContainerBuilder::get(), which is the bootstrap stuff, that take up ~0.5ms (almost 1,5ms in one of 4 requests, might be a fluke). This will only go up once we add more stuff here, and we'll have to.

With #1331486: Move module_invoke_*() and friends to an Extensions class this will never happen on a normal page request - at that point it will no longer be a "bootstrap container" but more of a "maintenance-mode container", only used when there is no kernel. And then we'll be more likely to be taking stuff out of it than putting anything else in.

Got it. Nice.

Also, don't rely too much on the testbot runtime. This seems to vary quite a bit across differrent bots, the latest 8.x run for example took 33minutes and I've also recently seen runs that only took 26 minutes.

I've recently noticed that my new system (i7 quad core 3.5ghz) is able to run the test suite quite fast, so if necessary, I could do a before/after testrun to get some more comparable results. Or parts of it, the differences should be more or less consistent for all test runs I think.

More benchmarks (XHProf) with 100 nodes and a view-source: on /node | frontpage

BASE: Symfony\Component\DependencyInjection\ContainerBuilder::get - 121-142 ms
ALREADY COMPILED: Symfony\Component\DependencyInjection\Container::get - 57 ms
IN COMPILE STAGE: Symfony\Component\DependencyInjection\ContainerBuilder::compile - 50 ms
IN COMPILE STAGE: Drupal\Core\DrupalKernel::dumpDrupalContainer - 35 ms
( building takes 57 ms, which includes 50 ms of compile time)

Site load matches that:

BASE With builder: 2,086,362
With compiled: 2,009,212 ( around 76 ms faster, 57 + 76 == 133 ms, which is within measured range)
With need to compile then run: 2,223,836 ( == around 137 ms slower than BASE, 90-100 ms is building and dumping in Drupal\Core\DrupalKernel::initializeContainer, the rest seems to be running and measurement tolerance)

--

Before (measured with XHProf on) with page timer: 21[20-60]
After (measured with XHProf on) with page timer: 20[40-80]

Hope that helps.

--

AB Benchmarks:

Compiled DIC

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:   560  573   9.2    572     624
Waiting:      544  556   8.0    555     598
Total:        560  573   9.2    572     625

Core

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:   569  583   8.7    581     628
Waiting:      553  566   8.3    565     610
Total:        569  583   8.7    581     628

Without XHProf I am also seeing speedup of 10ms, which is around 2%.

( ab -c1 -n100 against 100 nodes on /node )

Tested several times and numbers where totally consistent on runs.

Compiled DIC around 560 (min), Core around 568 (min) and the rest matches that.

@effulgentsia Did you run ab multiple times an these numbers were consistent?

Yes, while any single request can easily vary by many ms, averages of runs of 100 are consistent within 0.2ms.

Not sure about sites with multiple webservers, in this case, it might be save to have these files in a local file system, but will that be true for all stuff that we will put into the protected directory? If it needs to be shared, could it be that loading it from the network might be slower than building them?

Sites that scale to multiple dedicated servers will need to decide how to configure their infrastructure, but my guess is that for many, the best choice will be to have compiled PHP storage configured to use the local system of each web server. This means 1 slow request per server after enabling or disabling a module. That's nothing compared to optimizing the millions of requests that follow that. The reason drupal_php_storage() takes a $name argument is so that only dynamic php that can't be easily regenerated per web server needs to be mapped to a networked file system. Neither DIC nor Twig are use cases of that, but if we at some point want to replace Update Manager to use drupal_php_storage() instead of FileTransfer, that might be.

Thanks for the additional benchmarks and profiling, that's really handy!

So it looks like

- we do get a small performance improvement from compiling, but it's not exciting at all.
- the cost of compiling itself isn't bad at all (nothing compared to a menu/theme/registry/schema rebuild).

I was expecting a bigger performance increase here, so profiled the front page as well. It looks like we still run Drupal\Core\DrupalKernel::registerBundles on every request, which took around 3ms that profile run (attached the screenshot).

I was under the impression that once we compiled the container, we'd be able to avoid doing that every request. Additionally, if we're still doing it every request, then why do we care if the module list is different in terms of the compiled container? If this patch just enables that, but doesn't actually integrate this bit yet, than that might be fine but could use a follow-up at least. Would be good to understand this a bit better since it seems we're missing out on a 3ms improvement at the moment.

Related to this, the patch is currently relying on the module list changing the unique hash of the dumped container, which means that if you enable or disable a module you'll get a new cache ID and hence new rebuilt container. However I don't see any logic to handle the following:

- a module adds a bundle class that previously didn't have one
- a module removes a bundle class when it previously did have one
- changes to core bundle registrations

I understand there's no garbage collection here yet, but shouldn't there be a line added to drupal_flush_all_caches() to wipe the entire directory or similar?

At the moment a drush cc all or similar won't affect any of these at all. And following on from that, if you do have multiple webservers, a drush cc all (should that force a container rebuild) will only work local to that server - this might be a problem for people with multiple webservers to figure out rather than core but it probably needs documenting.

I have a question thought: does this patch effectively compile the pre-kernel container (the one hardcoded in drupal_container())? Because if not, we are compiling only half of the stuff we could compile.

Re #152, kindly read #147. Thanks!

The real important point of my semi-question was: we are compiling only half of the stuff we could compile: this just means that benchmarks here are only half-significant here since things are going to be improved later.

No the point is that you do not read the comment which says the other half is going away.

Edit: how that benchmarks, I guess we will see there. This patch is more held up by the fact that it does not react to code changes than this.

Now, we wipe.

There are several documentation and/or coding style issues that need to be addressed in #156. Is this the correct time to roll a patch that incorporates such changes?

@Lars Toomre, if it's covered by http://drupal.org/core-gates#documentation-block-requirements, yes. I didn't spot any missing docblocks, datatypes, etc. on a quick scan. If it's not covered by that (e.g. the Test rather than Tests), please provide the reroll yourself with an interdiff, or file a followup after the patch goes in. This is a critical blocking another critical which is blocking all progress on Views in Core.

That said, the change in #157 should probably be reviewed. (It looks fine to me, wiping the compilation on a full cache clear.)

No the point is that you do not read the comment which says the other half is going away.

God, you are just unbelievable. Just stop answering me if that makes you angry.

I was expecting a bigger performance increase here

Keep in mind that in addition to pounard's point that this patch doesn't improve the performance of services registered or retrieved during bootstrap (per #147, to be fixed in #1331486: Move module_invoke_*() and friends to an Extensions class), this patch also makes CoreBundle register all the event listeners as services: had those been registered as services in HEAD (even without compiler passes), HEAD would have been slower, so this patch's gains would have been more impressive: we kept those out of uncompiled service registration to minimize the interim performance impact of the earlier WSCCI patches. In general, the way to think of this patch is that it lowers the overhead of DIC services to the point that we can put things in the DIC without worrying about overhead, vs. otherwise having to pay a penalty for every DIC service (details in #1673162: Analyze performance of uncompiled and compiled DI containers).

I was under the impression that once we compiled the container, we'd be able to avoid doing that [registerBundles] every request.

Yes. The follow up for that is #1716048: Do not boot bundle classes on every request.

if you do have multiple webservers, a drush cc all (should that force a container rebuild) will only work local to that server

#156 handles single server flushing, but doesn't address the multi server case. Can we defer the multi server case to #1759582: Figure out when to delete compiled DIC files? One possibility is adding an extra random component to the hash similar to how we handle cache busting for JS and CSS files, but maybe there's better ideas than that we can explore in the other issue.

explanation.

OK I spoke to chx in irc. I'd misunderstood the bundle dependency here a bit, we don't ever register a bundle to the DIC, but the bundles themselves can register services which affect the DIC. This is why we can't get rid of the slow class_exists() calls and bundle registration from the patch just by having a compiled DIC :(

It would be good if we could only load the bundles during a compiler pass. I understand that may break things because bundles can have a boot() method which gets called every request, not sure what that's needed for or why we'd need to support it though - if we need a Symfony-esque equivalent to hook_boot() that could be done via EventDispatcher couldn't it? The hard-coded method on the bundle class with an empty implementation feels like a poor regression from the existing hook system. I also couldn't find any examples of where it's supposed to be useful.

We don't necessarily have to fix that here, but if we don't, then I'll want to open a new major task to fix the performance of bundle registration for modules, that 3ms is 3ms too much.

Having figured that out, that means that basing the container on the list of modules enabled isn't going to work, or not by itself, since it doesn't take into account code changes at all.

+      // While the default Symfony class name only depends on the environment, for
+      // testing purposes we can't use that because there would be a collision as
+      // each test method creates a new kernel. On the other hand, the container
+      // only depends on the enabled modules (and only on those that provide
+      // bundles) so we base the name of the container class on the hash of the
+      // enabled modules. We can't directly use the hash though because PHP
+      // identifiers always start with a letter and hashes don't so we add a
+      // character to the beginning. This mechanism also avoids the problem of
+      // needing to rebuild the DIC at the right time on module enable: simply on
+      // the next request the hash will change and so the container will be
+      // rebuilt.
+      $class = 'c' . $this->systemList['system_list_hash'] . ucfirst($this->environment) . ($this->debug ? 'Debug' : '');
+      $cache_file = $class . '.php';
+

This is fine as it is, but if you add stuff to your bundle, or update a module that has a bundle that previously didn't, then nothing is going to happen to the compiled container. I think we probably need to fix that before this goes in, or it'll need to be critical bug report if there's something this is absolutely blocking and it turns out to be very hard to get right.

Massive cross-post, I haven't reviewed #156 yet.

@Crell, the reason why it would be a critical, is because there was no way to clear the cache, nor cache clearing, in the patch. It's OK to have to clear the cache, it's not OK to have a cache that can't be cleared (at least not without manually locating the PHP files on the file system and rm-ing them).

#158 apparently adds that though so needs reviewing for this, I think the CNW was a cross-post.

OK I spoke to chx in irc. I'd misunderstood the bundle dependency here a bit, we don't ever register a bundle to the DIC, but the bundles themselves can register services which affect the DIC. This is why we can't get rid of the slow class_exists() calls and bundle registration from the patch just by having a compiled DIC :(

It would be good if we could only load the bundles during a compiler pass. I understand that may break things because bundles can have a boot() method which gets called every request, not sure what that's needed for or why we'd need to support it though - if we need a Symfony-esque equivalent to hook_boot() that could be done via EventDispatcher couldn't it? The hard-coded method on the bundle class with an empty implementation feels like a poor regression from the existing hook system. I also couldn't find any examples of where it's supposed to be useful.

I think this is not an issue, then answer you are looking for is in question you asked: if we register a compiler pass that effectively register all existing bundles that implement boot()[EDIT: Sorry, boot() method is in the interface... Sad Symfony is sad, ZF2 actually relies of introspection for this, no interface] into a list as a container paramater, we can then call boot() on existing bundles on a normal query (with compiled the DIC) without too much penalty: the list is hardcoded as a bundle parameter and allows us to instanciate those classes (and minimal since there will be only a few classes, since boot() is a very uncommon operation to implement)[EDIT: See upper edit] and without using class_exists().
This is possible because no bundle related operations at all are done after the DIC has been initialized (see the original Symfony Kernel::boot() method).
If Drupal does otherwise, and override the boot() method and reverse the order at some point, then we are doing it wrong™. Symfony does build the container before running any bundle operation.

EDIT: Anyway, giving that not all modules will implement a bundle, this is still fine to do this, we are not yet at the point where all module will do that, it gives us some time to think, meanwhile, we can implement the compiler pass magic trick.

@pounard: Did you meant to change the status or was that xpost again?

Oh sorry, cross-post...

The test failures are a problem though, so back to needs work.

Blargh.

So if we just delete all the files when doing this, then is it necessary to add the system_list hash to the cid then? It makes me very uncomfortable that figuring out which DIC to use requires having the system list available. Seems like the comment relating to testing is no longer true either.

EDIT: Stupid comment.

Yes since the latest patch flushes it manually, I don't think we need either.

Oups you were faster than me.

#177 contains documentation style nitpicks:

• Inline comments wrapped at 80 characters.
• Third-person verbs for class and method summaries.
• "Contains" instead of "Definition of" for the @file docblocks for classes; that standard was adopted earlier this summer.
• Moving descriptions for @param, @return, @var into the right places.

There are two things that need updated documentation, though. The system_list() docblock needs to be updated to explain the new behavior (update the list for $type, update to describe both possible return values, and explain more about the new behavior that's added). Also, I punted on the summary for the DrupalKernel constructor; that docblock could explain more what we're doing in the overridden constructor.

Two typos I missed:

+++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/RegisterNestedMatchersPass.phpundefined
@@ -12,12 +12,12 @@
- * Add servies tagged 'nested_matcher' services to the tagged_matcher service.
+ * Adds servies tagged 'nested_matcher' services to the tagged_matcher service.
...
-   * Add servies tagged 'nested_matcher' services to the tagged_matcher service.
+   * Adds servies tagged 'nested_matcher' services to the tagged_matcher service.

Missed this on the first pass, but there's another typo here ("servies") and the second word "services" should be removed.

+++ b/core/lib/Drupal/Core/DrupalKernel.phpundefined
@@ -49,17 +49,24 @@ class DrupalKernel extends Kernel {
+   *   Boolean indicating whether we are in debug mode; Used by

That semicolon was supposed to be a period.

So if we just delete all the files when doing this, then is it necessary to add the system_list hash to the cid then?

Suppose a use case where someone wants to use FileReadOnlyStorage as the implementation on a production site, but still allow some modules to be enabled or disabled on prod, and therefore wants to place multiple precompiled DICs into the storage bin.

Is that a valid use case though? I'm not sure it justifies the dependency on the module list at all.

In the interest of moving this along and unblocking #1708692: Bundle services aren't available in the request that enables the module, I'd be fine with removing the system_list hash and deferring discussion of #179 to #1759582: Figure out when to delete compiled DIC files.

Oh, wait. One reason for the hash is related to #1708692: Bundle services aren't available in the request that enables the module. If in the same PHP request, we need a new DIC, then it also needs a new class name, since PHP can't dump a class from memory and reload new code for it.

Trying to build a new container en route seems conceptually wrong somehow. But that belongs to the other issue.

@catch: what's your resistance to the system_list() dependency? If the compiled container doesn't exist, then DrupalKernel needs to iterate the enabled modules in order to build the container. Since DrupalKernel needs to be able to initializeContainer() regardless of whether the compiled container already exists, it already needs a way to access system_list. If the objection is just one of performance rather than decoupling, would it be okay to figure out how to optimize the system_list() away in a follow up? We could, for example, name the DIC class based on the hash of its contents (which would remove the need for deleting in response to a code update), but then we'd need a state() variable to let DrupalKernel know the current DIC name, and I'm not sure that a state() dependency is any better than a system_list() dependency.

Here's an idea: we can hash on DIC contents and maintain the value of that hash in cache('bootstrap'). It doesn't need to be state(), since losing it is no big deal, it just means we need to spend a bit of time rebuilding. And meanwhile, we're pretty much guaranteed to have already needed cache('bootstrap') by the time we instantiate DrupalKernel. I'll try this out and post a patch. Ping me in IRC if you want to dispute this before seeing the patch.

This implements #185, reverting the need for system_list() to store a hash or support a null $type.

Fixes a PHPDoc.

In IRC, chx said that if we're reverting some system_list() changes, we can revert all of them, so this does that. Also, this adds an index.php change which I mistakenly left out of prior 2 patches.

Since there's been a lot of small rerolls since #124, roughly the last patch that was substantively RTBC'd, here's the full interdiff from that to #188.

Good point. But even in that issue, ExtensionHandler::__construct() requires $bootstrapCache. However, if we want to get to a point where DrupalKernel::initializeContainer() can run with neither an $extensionHandler nor a $cacheBackend, then I'm at a loss for anything other than a hard-coded class name for the DIC. For that, we can get around #182, by making DrupalKernel always use an uncompiled container if it detects that the compiled class already exists in memory (and is therefore suspect), but then we'll still have the problem that a cache clear would need to delete the compiled DIC from all web servers, since a DrupalKernel::initializeContainer() call on all other web servers would have no persistent information in Drupal that it could access to know that its local compiled container is stale. Given that, my suggestion is to proceed with #188's approach of allowing DrupalKernel::__construct() to receive a cache backend object, and bang our head on how to decouple that away in a follow up. But I'm open to other ideas if anyone has any.

#191 is reasonable. This is a major feature that needs to get in ASAP and then minor tuning can happen past Dec 1 should we find we do not want a bootstrap cache any more. But, given how even the new system_list rides on that and how much Drupal depends still on modules, I do not see a chance of bootstrap cache dying.

@effulgentsia I'm uncomfortable with the system list dependency for the following reasons:

1. It doesn't actually do what it says (container contents can change even if module list is the same, or they might not change with a different module list)
2. It introduces a hard dependency on the Extensions class (when we have it), which is a fairly high-level concept in Drupal compared to most things registered to the DIC.
3. It just feels bad.

The hash of the contents doesn't have any of these problems though. This also ought to fix the following without depending quite so much on the explicit clear.

Enable a module that provides a bundle. (hash 1)
Disable the module (hash 2)
Remove the bundle from the disabled module.
Enable the module again (back to hash 1, but in this case a different container).

Then this is good to go. #194 says that the hash approach in #188 is good if I read it correctly.

+++ b/core/lib/Drupal/Core/DrupalKernel.phpundefined
@@ -26,6 +29,60 @@
+   * Constructs a DrupalKernel object.
+   *
+   * @param string $environment
+   *   String indicating the environment, e.g. 'prod' or 'dev'. Used by
+   *   Symfony\Component\HttpKernel\Kernel::__construct(). Drupal does not use
+   *   this value currently. Pass 'prod'.
+   * @param bool $debug
+   *   Boolean indicating whether we are in debug mode; Used by
+   *   Symfony\Component\HttpKernel\Kernel::__construct(). Drupal does not use
+   *   this value currently. Pass TRUE.
+   * @param array $module_list
+   *   (optional) The array of enabled modules as returned by module_list().
+   * @param Drupal\Core\Cache\CacheBackendInterface $compilation_index_cache
+   *   (optional) If wanting to dump a compiled container to disk or use a
+   *   previously compiled container, the cache object for the bin that stores
+   *   the class name of the compiled container.

The @todo in this went away, but the docs for $environment and $bool still say only one value is allowed. Is it still accurate?

Meanwhile, rerolling to cleanup the typos mentioned in #178.

RTBC++... Would love to get this in soon as we could then move a lot of the service registrations that are directly in ->build() over to use Compiler Pass to better take advantage of compilation.

Committed/pushed #197, thanks all!