Make optimized class loader detection more generic to support class loaders other than ApcClassLoader

Overall +1
I think hardcoding classnames into core is not a good idea, maybe better to make them configurable via settings somehow

1. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -964,16 +964,31 @@ protected function initializeSettings(Request $request) {
   -    // If the class loader is still the same, possibly upgrade to the APC class
   +    // If the class loader is still the same, possibly upgrade to an optimized class
   

   more then 80 chars in line

2. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -964,16 +964,31 @@ protected function initializeSettings(Request $request) {
   +        \Symfony\Component\ClassLoader\ApcClassLoader::class, ¶
   +        \Symfony\Component\ClassLoader\WinCacheClassLoader::class, ¶
   

   trailing whitespace

One thing to keep in mind is the fact that any such optimizations are liable to have problems on multi-web-head configurations. This should not be the case for just the classloader with the rebuild-stamped and shared-filesystem issues being solved, but there's probably not much more which can be done without accounting for multiple heads.

Unfortunately, this feature didn't make the cut before RC. There may be a case for it to be included as a bug from the beta evaluation but it looks like a feature request and is marked as a task.

Feel free to disagree, I'm just triaging the queue.

Postponed because the 8.1.x branch is not open and tests won't work on it. Tagging to see what the product manager thinks maybe it can still get in 8.0.x.

#8: However, 80% will have APCu for that already, xcache has gotten much less popular in the last years.

And Drupal on windows, while nicely supported is still a niche and also those people usually know more what they are doing than someone on a VPS or such ...

For the record, which branch a change can be committed to and when is ultimately a release management decision, not a product management one. :) Product managers are in charge of the user-facing feature set and user experience. Reference: http://cgit.drupalcode.org/governance/plain/drupal-core.html#product-man... versus http://cgit.drupalcode.org/governance/plain/drupal-core.html#release-man....

Since this issue would be adding additional functionality in the form of additional class loader support, I think it it is correct to discuss it as a 8.1.x issue according to https://www.drupal.org/core/d8-allowed-changes#minor. (Not weighing in on whether or how the change should be made, just which branch would be correct if we decide to go forward with it.) Thanks!

Also, testing against 8.1.x should work now, so I'll try adding a test run.

Reviewing mostly some minor coding standards fixes but wondering about extensiblity on this:

1. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -963,16 +963,30 @@ protected function initializeSettings(Request $request) {
   +      $classloaders = array(
   +        \Symfony\Component\ClassLoader\ApcClassLoader::class,
   +        \Symfony\Component\ClassLoader\WinCacheClassLoader::class,
   +        \Symfony\Component\ClassLoader\XcacheClassLoader::class);
   

   Should this be configurable in some way to extend this list?
   And also a little nit: last item needs a comma and a linebreak.

2. +++ b/core/rebuild.php
   @@ -42,9 +42,17 @@
   +  // Clear user cache for all major platforms
   

   nit: Period.

3. +++ b/core/rebuild.php
   @@ -42,9 +42,17 @@
   +  $user_caches = array(
   +      'apc_clear_cache' => array('user'),
   +      'apcu_clear_cache' => array(),
   +      'wincache_ucache_clear' => array(),
   +      'xcache_clear_cache' => array(),
   +    );
   

   nit: Indenting can be moved back 2 spaces and short array syntax on the new hunks could be nice.

I agree with this patch, but I disagree with try { catch on every page load - that seems very excessive.

Lets just use a quick and fast function_exists for the loader as well.

apcu_fetch, apc_fetch, wincache_..., etc.

Btw. And thanks David Garcia!

Users @andypost and @Fabianx are for this issue and to me this seems to be a good idea. I would like to get this issue fixed, but I have some remarks/questions:

1. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -1001,16 +1001,32 @@ protected function initializeSettings(Request $request) {
   +      $classloaders = [
   +        \Symfony\Component\ClassLoader\ApcClassLoader::class,
   +        \Symfony\Component\ClassLoader\WinCacheClassLoader::class,
   +        \Symfony\Component\ClassLoader\XcacheClassLoader::class,
   +      ];
   

   Can we make $classloaders a class property. And can you explain why there are only these 3 classes added and why there are ordered in this way.

2. +++ b/core/rebuild.php
   @@ -42,9 +42,17 @@
   +  // Clear user cache for all major platforms.
   +  $user_caches = [
   +    'apc_clear_cache' => ['user'],
   +    'apcu_clear_cache' => [],
   +    'wincache_ucache_clear' => [],
   +    'xcache_clear_cache' => [],
   +  ];
   +  foreach ($user_caches as $name => $args) {
   +    if (is_callable($name)) {
   +      call_user_func_array($name, $args);
   +    }
      }
   

   Is it necessary to clear all 4 caches? Can you give an estimation how much slower this make drupal core. Is there any way to know if some of these calls are not necessary?

3. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -1001,16 +1001,32 @@ protected function initializeSettings(Request $request) {
   +    // If the class loader is still the same, possibly upgrade to
   +    // an optimized class loader.
   ...
   +      // Each one of these optimized loaders will verify that the
   +      // required storage backends are available upon being instantiated
   +      // and will throw an exception if not.
   

   Nitpicks: "Lines containing comments (including docblocks) must wrap as close to 80 characters as possible without going over" according to #1354: [Obsolete] API documentation and comment standards.

+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -46,6 +46,21 @@
+   * ¶
+   * Autodetection tries to initialize these in the
+   * order they appear in the array. They are ordered
+   * in terms of "adoption/usage".
+   * ¶

@@ -1001,16 +1016,28 @@ protected function initializeSettings(Request $request) {
+      // Each one of these optimized loaders will verify that
+      // the required storage backends are available upon ¶
+      // being instantiated and will throw an exception ¶
+      // if not.

These lines are not: "Lines containing comments (including docblocks) must wrap as close to 80 characters as possible without going over". Also 4 lines are ending with a space character.

Yes, the commit lines are below 80 character, but they can be a lot closer to 80 characters without going over.

Edit: see https://www.drupal.org/node/1354.

I agree with this patch. It seems like improvements out of the box in Drupal are exclusive only for non-MS installations, even when there's a big enough community of users running Drupal on Windows that would like some of those too. And also seems a little biased to say that all those projects are being run by experts.

I think that David did a good job pointing that it would be nice if core supported autodetection of optimized classloaders for other major environments, this would improve the experience of a lot of people coming to Drupal from other platform backgrounds.

Btw, I tried the patch and runs pretty good, there's not a noticeable overhead caused by the implentation.

+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -46,6 +46,21 @@
+    \Symfony\Component\ClassLoader\XcacheClassLoader::class,

@@ -1001,16 +1016,28 @@ protected function initializeSettings(Request $request) {
+        try {
+          $loader = new $loader_class($prefix, $this->classLoader);
+          break;
+        }

What about:

$this->classLoaders = array(
  'apcu' => '\Symfony\Component\ClassLoader\ApcClassLoader::class',
  'wincache' => '\Symfony\Component\ClassLoader\WinCacheClassLoader::class',
);

foreach ($this->classLoaders as $extension => $loader_class) {
  if (extension_loaded($extension) {
    $loader = ...;
  }
}

Overall it would be nice to support the different cache systems not just for the class loader, but also for FileCache and the cache backends.

But that can be a follow-up.

Okay, if we really must ...

Testbot problem.

So if you don't have any of those extensions available, won't we try and fail to instantiate three different classloaders, each time with them throwing an exception?

Hi folks

I want to add a change in drupal_rebuild function to remove the type declaration for classloader parameter in the same way the function DrupalKernel::createFromRequest did, to avoid issue with any variation of class loader

I am proposing this change because I am getting this error in my environment using APC loader

Recoverable fatal error: Argument 1 passed to drupal_rebuild() must be an instance of Composer\Autoload\ClassLoader, instance of Symfony\Component\ClassLoader\ApcClassLoader given, called in /Users/enzo/www/DrupalConsole/src/Command/Cache/RebuildCommand.php on line 106 and defined in /Users/enzo/www/drupal8launcher.dev/web/core/includes/utility.inc on line 24

I think we need to profile the difference to an actual Drupal request, or some different ones preferably, for this to be OK.

Generally in core, we have avoided throwing exceptions in the critical path under normal circumstances for performance reasons. Not saying that that always has to stay that way, but this change would have bigger consequences in that it would make it acceptable for exceptions to be thrown generally.

Throwing and catching 40,000 exceptions:

PHP 5.6.x: 0.08s , that would be 6*10exp-6 seconds for 3 exceptions
PHP 7.0.x: 0.04s, that would be 3*10exp-6 seconds for 3 exceptions

How much of the time is from the first exception being thrown, how much is from the other 39,999?

Given that we already hardcode the 3 different classloader implementation, IMHO we could also just do the if/elseif/elseif call here. It is IMHO not worth to care about the exceptions here.

+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -1001,16 +1003,29 @@ protected function initializeSettings(Request $request) {
+      if (function_exists('apcu_fetch')) {
+        $loader = new ApcClassLoader($prefix, $this->classLoader);
+      }
+      elseif (extension_loaded('wincache')) {
+        $loader = new WinCacheClassLoader($prefix, $this->classLoader);
+      }
+      elseif (extension_loaded('xcache')) {
+        $loader = new XcacheClassLoader($prefix, $this->classLoader);
+      }

I think we might still want to wrap this in a try { catch } block as especially wincache can also have the extension_loaded, but still fail to use it as an user cache as David Garcia stated above.

But that feels good to me and we can assume that 99% of the time it will just work :).

Having a couple of if/else is more or less the same here.

Right, which is why I'm arguing that doing that is enough :)

I tested what happens in this situation (wincache.ucenabled=0) and basically wincache fails transparently, so the worst that will happen is that the Wincache classloader will cache miss all the time.

Mh, that's a fair point.

RTBC for #41.

+1 to no-try-catch better to know asap that no cache backend available

+++ b/core/includes/utility.inc
@@ -14,14 +14,16 @@
+ * @param $class_loader

type probably "object", could be fixed on commit

+++ b/core/rebuild.php
@@ -42,9 +42,17 @@
+    'apc_clear_cache' => ['user'],

Is this necessary? In order to use the APCClassLoader we're checking if apcu_fetch exists.

If we want to introduce this change, than not in this issue, IMHO.

+++ b/core/rebuild.php
@@ -42,10 +42,16 @@
+    'apc_clear_cache',

Lets just remove this line, we do use a symfony shim that makes apcu available even if only APC (compat) extension is enabled, so we are fine to always use the apcu functions :).

Sure, let's do that.

RTBC - Looks good now

Committed 561580c and pushed to 8.3.x. Thanks!

Seems worth a release notes mention, so tagging.

#66: Those sites that have an optimized build process and don't care about the memory used can easily set the class loader explicitly in settings.php, too. In the worst case they need to use a wrapper class to avoid the auto-detection from firing, but that is quite doable with a composer plugin.