Problem/Motivation

This is a follow up for #2296009: Use APC Classloader by default (when available).

Using in-memory cached classloader represents a very nice performance improvement. Although this can be manually configured in settings.php, core added the ability to autodetect the availability of APCu user cache and use symfony's ApcClassLoader by default.

Symfony has also several other optimized class loaders and the original autodetection implementation was only APCu specific.

It would be nice if core supported autodetection of optimized classloaders for other major environments, to make Drupal fast out-of-the-box on as many platforms as possible.

Proposed resolution

Refactor the autodetection mechanism to make it easier to detect the availability of other optimized class loaders, and leverage the fact the symfony's class loader implementations already detect if the required environment setup is available (extensions and functions) instead of doing specific checks for APCu.

Remaining tasks

User interface changes

None.

API changes

None.

Data model changes

None.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because it is very short minded to only support APC and hardcode it into core without being pluggable - making Drupal barely usable on platforms that do not support APC/APCu.
Issue priority Major because according to the original issue the performance gains are noticeable.
Prioritized changes The main goal of this issue is performance

Comments

david_garcia created an issue. See original summary.

david_garcia’s picture

Status: Active » Needs review
FileSize
2.72 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 106,859 pass(es). View

Here comes a patch.

Overally I am seeing a tendency towards platform specific code in Drupal core (mainly APC/APCu usage) in such a ways that it is not easy to replace for alternative backends.

Just take a look at rebuild.php, where specific platform user cache clearing is hardcoded and impossible to override/replace.

Considering that most user caches (APC, APCu, Wincache, XCache) work in a very similar fashion some level of abstraction could be introduced here (similar to the database abstraction layer) so that core can interact with in memory user caches in a more generic and pluggable way.

andypost’s picture

Status: Needs review » Needs work

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

fgm’s picture

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.

david_garcia’s picture

Status: Needs work » Needs review
FileSize
2.73 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 106,877 pass(es). View

Fixed coding issues.

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

I don't like how these classes are hardcoded either, but they were before. Taking into account that in-memory storage is extremely important for performance on a shared nothing architecture and that Drupal should rely more and more on it's availability I believe it should be abstracted in a pluggable way as cache backends are.

Autodetection can be disabled in settings.php anyways.

One thing to keep in mind is the fact that any such optimizations are liable to have problems on multi-web-head configurations.

It does not quite matter that some very frequently used caches get duplicated in multiple heads, such as bootstrap, the classloader, or even other ones... as long as they are not too expensive to generate and store. In memory user caches are way faster than network based shared storage such as memcache, redis or similar - even orders of magnitude faster on some setups.

david_garcia’s picture

Issue summary: View changes
joelpittet’s picture

Version: 8.0.x-dev » 8.1.x-dev
Status: Needs review » Postponed

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.

david_garcia’s picture

Status: Postponed » Needs review

You can override this in settings.php so I don't really care.

But I am sure many people will miss that setting, and given the performance gains of using a classloader (+25% more RPS on some scenarios) that leverages caching they will have a painful D8 experience (as if it was not slow enough as it is now...).

I've just always felt that issue priorities a little bit messed up because they do not take into account if the change itself can break things. That's how you end up having hundreds of one-line no brain melter patches lying around in the issue queue dead for months or years, degrading the overal Drupal experience.

I remember at some point having > 100 patches from the issue queue applied to a D7 core fork. How are not those 100 patches all together as critical as one single "critical" issue :)

And I am sure that some patches can be easily evaluated with a 0% probablity of error that they will break nothing.

I don't agree with the postponed status:

Postponed (#)
May mean either 1) that the issue is valid and should be fixed, but other related issues (blockers) need to be dealt with first, or 2) that the issue is removed from current active work but the intent remains to return to it. In the first case, please tag the issue "blocked" and say what issue(s) it's blocked on in a comment.

Even if moved to 8.1.x-dev, this still needs review (or work).

joelpittet’s picture

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.

Fabianx’s picture

#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 ...

xjm’s picture

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.

Status: Needs review » Needs work

The last submitted patch, 5: 2556025-improve-class-loader-autodetection.patch, failed testing.

david_garcia’s picture

joelpittet’s picture

Status: Needs review » Needs work

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.

The last submitted patch, 13: 2556025-improve-class-loader-autodetection-8-1-x.patch, failed testing.

david_garcia’s picture

Fixed some of the style issues and the bug that made the tests fail.

Should this be configurable in some way to extend this list?

These three classloaders are the only ones with platform specific caching that are implemented in symfony, and I believe that by using those three we are covering somewhat 99% of use cases, if not even more.

And as I stated before, you can indeed configure a specific classloader in settings.php, the problem is that the "autodetection" happens before settings.php is processed, so there is actually a measurable edge in adding these class loaders as early as in this autodetection.

And the more Drupal works out of the box - on as many platforms as possible - the better to foster adoption.

david_garcia’s picture

Status: Needs work » Needs review
Fabianx’s picture

Status: Needs review » Needs work

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!

david_garcia’s picture

Status: Needs work » Needs review
FileSize
78.2 KB

@Fabianx The use of try/catch is there in order not to reimplement the logic these classloaders already have in their constructors, where they check if what they need to run is available.

I implemented another approach but really, I believe that performance impact of both changes are negligible. Talking about the try{} catch{} "issue":

1. Users with APC enabled: because that is the first element in the loop an exception will never be triggered. Most of the overhead in using try/catch blocks comes when exceptions are thrown (that's why relying on try/catch for logic implementation such as it is being done here is not ideal...)
2. Users with other class loaders: at the most this is a 2 element loop... should still be neglibile.

In the new patch I'm using "extension_loaded" instead of "function_exists" to respect symfony's implementation.

david_garcia’s picture

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

david_garcia’s picture

Patch did not apply anymore.

Only rerolled the first version.

daffie’s picture

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].

david_garcia’s picture

Status: Needs work » Needs review
FileSize
3.16 KB

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.

Done. Explanation is in the comments. Note that these class loaders cannot be "parametrized" (such as being configured in settings file) because this autodetection happens early in the bootstrap.

Later on in bootstraping, if the user specifies a classloader in settings.php, that will be loaded and used.

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?

This will make Drupal core 0.000% slower :) These changes only affect calls to rebuild. This code is not part of the normal request/response workflow. The additional overhead is 3 extra calls (assuming that a deployment will only have one of those backends enabled) to "is_callable". 3 calls to is_callable should not be a concern, specially if only triggered when calling rebuild.php which, by the way, is a slow thing by itself making the change even more negligible.

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

Fixed.

daffie’s picture

Status: Needs review » Needs work
+++ 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.

david_garcia’s picture

Status: Needs work » Needs review
FileSize
3.16 KB

True about the whitespaces, yet my editor tells me all these comment lines are below 80 characters long.

daffie’s picture

Status: Needs review » Needs work

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.

izaaksom’s picture

Status: Needs work » Reviewed & tested by the community

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.

Fabianx’s picture

Status: Reviewed & tested by the community » Needs review
+++ 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.

david_garcia’s picture

@Fabianx #30 I already proposed something similar in #19 but it turned out to be too complex because the fact that the extension is loaded (at least for wincache) does not mean that the user cache is setup and available. Winache does many things besides providing a user cache, and they can all be disabled individualy.

Each one of the class loaders "hides" the complexity of veryfing if the required backend is available during instantiation.

Again, the overhead of exceptions only matters if you rely on really catching them. For APC users, no exception will be thrown.

http://stackoverflow.com/questions/104329/performance-of-try-catch-in-php

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

Okay, if we really must ...

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 27: 2556025-improve-class-loader-autodetection-27.patch, failed testing.

daffie’s picture

Status: Needs work » Reviewed & tested by the community

Testbot problem.

catch’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs profiling

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?

-enzo-’s picture

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
david_garcia’s picture

Status: Needs review » Reviewed & tested by the community

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?

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

The overhead of the change, when none of the extensions is available, is in the range of a handful of microseconds.

@-enzo-'s change makes sense, Symfony's classloaders do not inherit from the ClassLoader class.

tstoeckler’s picture

Status: Reviewed & tested by the community » Needs review

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.

catch’s picture

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?

david_garcia’s picture

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

Initializing the loop and throwing just 1 exception yields something between 1.4*10-5s and 1.6*10-5s, that is, aprox. 16 microseconds (on PHP7). Still, the impact is neglibile, a profiler will be unable to grasp such a difference (these results are with XDEBUG off).

Tried with several loop sizes and it is true that the loop initialization and first exception are more costly than the other exceptions.

Not the best way to measure this, but this is the code I used (it more or less emulates the loop introduced in the patch in the sense that it will instantiate an object and an exception is thrown in the constructor itself):

class test {
  function __construct() {
    throw new \Exception("This is it.");
  }
}

$time = microtime(TRUE);

for ($x = 0; $x < 1; $x++) {
  try {
    $a = new test();
  }
  catch (\Exception $e){
  }
}

echo microtime(TRUE) - $time;

exit();
dawehner’s picture

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.

Fabianx’s picture

+++ 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 :).

david_garcia’s picture

I think we might still want to wrap this in a try { catch } block as especially

Not really, because this code is doing the exact same checks that the classloaders are doing upon being instantiated. The only risk here is those checks changing upstream.

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.

I really don't understand the generalized aversion to exceptions. Having a couple of if/else is more or less the same here.

wincache can also have the extension_loaded, but still fail to use it as an user cache as David Garcia stated above.

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.

dawehner’s picture

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.

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

RTBC for #41.

andypost’s picture

+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

david_garcia’s picture

Tested #41 manually to make sure it works as expected. +1 RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
+++ 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.

dawehner’s picture

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

dawehner’s picture

Status: Needs review » Needs work

The last submitted patch, 50: 2556025-50.patch, failed testing.

Fabianx’s picture

+++ 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 :).

dawehner’s picture

Sure, let's do that.

Status: Needs review » Needs work

The last submitted patch, 53: 2556025-53.patch, failed testing.

The last submitted patch, 53: 2556025-53.patch, failed testing.

david_garcia’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 53: 2556025-53.patch, failed testing.

david_garcia’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 53: 2556025-53.patch, failed testing.

david_garcia’s picture

Status: Needs work » Needs review
FileSize
3.96 KB

Again, did not apply due to latest changes. Hope gets in soon so there is no need to roll it again.

Status: Needs review » Needs work

The last submitted patch, 60: 2556025-59.patch, failed testing.

david_garcia’s picture

Status: Needs work » Needs review
FileSize
3.96 KB
Fabianx’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs profiling

RTBC - Looks good now

  • catch committed 561580c on 8.3.x
    Issue #2556025 by david_garcia, dawehner, -enzo-, Fabianx, catch: Make...
catch’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +8.3.0 release notes

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

Seems worth a release notes mention, so tagging.

david_garcia’s picture

Not strictly due to the changes from this issue, but quite related... if using an optimized classloader:

#1818628: Use Composer's optimized ClassLoader for Core/Component classes

The by putting Apc or Wincache on top of it, we are actually making the autoload process slower.

The original benchmarks that motivated using the Apc classloader are from here #2296009: Use APC Classloader by default (when available), but they where not using optimized composer classloaders dumps.

Considering that D8 has moved into something that needs to be "built" it will become more common that as part of this build process optimized loaders are dumped.

Fabianx’s picture

#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.

Status: Fixed » Closed (fixed)

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