Problem/Motivation

The APC classloader enables significant speedups over the default classloader, and APC user cache is available to many, maybe most, PHP hosts. Enabling it by default therefore provides "fast by default" to deployments which would not necessarily consider enabling it.

A issue of concern may be the case of multiple web heads not being invalidated. However, as discussed at DDD Montpellier with alexpott, amateescu, berdir, fgm, pwolanin, webchick, wim leers, and xjm, because of the current implementation of the APC classloader as a decorator, misses resulting from new deployments are not a concern, and neither are extra entries after some code is removed because corresponding classes are then no longer loaded, so even deployments with multiple web heads should not encounter specific issues with this use of APCu.

berdir has been running big, complex Drupal 8 sites in production environments with multiple web heads, and confirmed that this works fine.

Proposed resolution

Provide detection of APC classloading ability and enable it by default, providing a killswitch variable to prevent detection for users wishing another strategy.

Remaining tasks

Refresh or create documentation specific to sites with multiple web heads, in a followup (#2471597: Create comprehensive documentation for running Drupal in a "cloud-native" or 12-Factor App model): they are affected by other consistency issues even more than this one.

User interface changes

None.

API changes

A new setting is available as class_loader_auto_detect, defaulted to TRUE. Setting it to FALSE disables this automatic enablement of the APC class loader.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because D8 works without it, and this does not bring a new user-visible feature
Issue priority Major because we want D8 "Fast by default", and this allows significant load time improvements for an estimated 90% or more of all D8 sites (in number of sites, not number of requests). Not critical because it is not the biggest performance gain to be had.
Prioritized changes The main goal of this issue is performance.
Disruption Not disruptive for core/contributed and custom modules, because they should be transparent to the used class loader.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir’s picture

I'm repeating myself, but I still don't believe in "Fast by default" and would still prefer an explicit production.settings.local.php or something like that.

I do have it enabled locally and sometimes when you update code and move classes around, *really* weird stuff can happen. I know that I have to restart apache if that happens, others might not know or even able to do that (shared hosting). Yes, we could possibly add a special case to rebuild.php and maybe even should, but still :)

Wim Leers’s picture

#1: You're not against using the APC class loader by default, but against the general principle of "Fast By Default"? (Which manifests itself in current HEAD by having errors not being shown by default and CSS and JS being aggregated by default.)

dawehner’s picture

Well, if shared hosting will have problems we should rather not do it right? You can for sure always serve nothing really fast, but from my perspective safety is more important than performance.

Berdir’s picture

No, I meant I'm against it by default, and sorry about the tone. I've seen too many developers helplessly looking at the white "An error occurred" screen and not being able to figure out what's going on. And explained how to enable settings.local.php too often ;)

That said that's probably a good argument the other way round too, if there would be an opt-in fast settings.php, people wouldn't find that either ;)

Still, I personally prefer performance settings that have drawbacks to be opt-in, with explanations about what those drawbacks are. But if we can at least include a force-clear in rebuild.php so that everyone can clear a messed up classloader cache then I'm OK with this I guess.

sun’s picture

I agree with @Berdir's concerns; especially because there is no (easy/built-in) way to recover from wrong defaults (yet). The moment you consider potentially unreliable/unstable options as defaults, there must be facilities to guarantee that users are able to recover from unexpected incidents.

The "By Default" paradigm is conceptually flawed. There is no single default that could be right, and the concept ignores that there are typically multiple instances of a site, for different purposes, having different requirements, and which need to operate on vastly different platforms and environments.

A more sensible approach:

/core/env.php — Maintenance utility. Determines current platform, environment, and available drivers. Shows a simplistic UI to reconfigure all backends and drivers that are used in this site instance. Also allows to toggle caching and debugging options. Rewrites settings.php and rebuilds the container upon form submission. This script even works if the current config instructs the app to use e.g. APC even though it is not available. Enables you to adapt (and optimize) an existing site [instance] for a particular host/environment.

/core/install.php — Installer. Determines and configures the optimal backends and drivers for the environment upon installation, using the same detection facilities as env.php. Shows a simple list of (enabled) checkboxes for suggested performance optimizations (which can be toggled off). (Re)writes settings.php anyway.

/admin/reports/status — Status report. Verifies whether the site instance leverages all backends and drivers that are available in the current environment; shows informational notes otherwise, pointing to env.php.

Bottom line: Think about site instances and varying environments, instead of "a" single site.

Fabianx’s picture

Priority: Normal » Major
Status: Active » Needs review
Issue tags: +Performance, +APC, +Needs issue summary update
Parent issue: » #1744302: [meta] Resolve known performance regressions in Drupal 8
FileSize
2.51 KB

Here is a quick patch implementing this.

- Added the core/rebuild.php requirement. Clearing the whole apcu cache is probably a good idea anyway.
- Detected the class used and if it changed, used optimized one.
- Allows to opt-out via settings.php to use default class loader.

I found that the kernels class loader is actually never exchanged, but I guess this is by design.

Fabianx’s picture

FileSize
2.51 KB

Little problem in detection logic, need to use extension_loaded('apcu') as apcu_enabled is just available in 4.0.7.

Berdir’s picture

Do we need a separate flag? Isn't it enough to update the existing classloader part and instead provide a default snippet that doesn't use apc?

Status: Needs review » Needs work

The last submitted patch, 7: use_apc_classloader_by-2296009-7.patch, failed testing.

Berdir’s picture

That failure is very interesting, and confirms my idea that that error is related to apc and might therefore be fixable after all. Do we need to clear the apc cache in phpstorage with OpCache? See #2429659: Race conditions in the twig template cache for an issue I opened for this problem.

Berdir’s picture

Uhm, forget the part about opcache, that's obviously not related. We somehow need to invalidate the classloader cache, but when and how, I don't know.

Fabianx’s picture

I know what this and the other one is:

While the class name is the same, the hash of the generated file in PhpStorage differs for security reasons.

Therefore, you have class:

__TwigTemplate_6fe23f1092605e4c72c45513259ad184c1b4ca49c89c827a22fb7afa5ca6461e

(this hash is calculated with the contents of the template)

while the filename used by PhpStorage differs after an update (it is calculated by the mtime of the directory).

Composers class loader uses file_exists() and is such protected against this.

APCClassLoader has the classname cached for this filename, which now no longer exists.

This means indeed that we need to invalidate the class loader cache for any class stored in PhpStorage.

--

Thinking more about this:

- The class loader is not at all involved in class loading here, however checking for a class_exists($cls, FALSE) is probably a good idea.

Uploading a new patch with fixed rebuild.php to see if we get that bug again.

Fabianx’s picture

Status: Needs work » Needs review
FileSize
2.52 KB
370 bytes
Fabianx’s picture

#8:

Do we need a separate flag? Isn't it enough to update the existing classloader part and instead provide a default snippet that doesn't use apc?

It might be that someone would like to use the standard composer class loader for whatever reason, e.g. because they have a class map based approach for their app.

As such its important to disable the auto-detection.

Wim Leers’s picture

+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -244,6 +245,18 @@ public static function createFromRequest(Request $request, $class_loader, $envir
+    // If the class loader is still the same, possibly upgrade to the APCu class loader.

+++ b/sites/default/default.settings.php
@@ -379,7 +379,12 @@
+ * The Symfony APC class loader is used by default when the APCu extension is detected.

Silly nit: 80 cols.

Berdir’s picture

Re #14: Right, but can't we make it work by being able to provide the snippet that would explicitly use composer.

We have that and the flag now. What happens if you set the flag *and* a custom class loader? At the least, we need to update the By default ... stuff, becaue that's just not true anymore.

Fabianx’s picture

Agree, this needs work, and a new approach for how to do that.

Not sure the auto-detection is good, but its difficult to distinguish as long as its not explicit.

Status: Needs review » Needs work

The last submitted patch, 13: use_apc_classloader_by-2296009-13.patch, failed testing.

fgm’s picture

Status: Needs work » Needs review
FileSize
3.08 KB

Patch no longer applied, rerolled on current HEAD.

Wim Leers’s picture

Assigned: Unassigned » fgm

Go fgm! Thanks for tackling this one!

Fabianx’s picture

Status: Needs review » Needs work

CNW for the documentation updates and a 'sane' way to say we don't want to use the default classloader detection.

fgm’s picture

Status: Needs work » Needs review
FileSize
3.97 KB

How about this wording ?

Wim Leers’s picture

Status: Needs review » Needs work

Quick round of nitpicks

  1. +++ b/core/lib/Drupal/Core/DrupalKernel.php
    @@ -245,6 +246,18 @@ public static function createFromRequest(Request $request, $class_loader, $envir
    +    // If the class loader is still the same, possibly upgrade to the APCu class loader.
    

    80 cols.

  2. +++ b/core/rebuild.php
    @@ -20,7 +20,12 @@
    +// Clear the APCu cache to ensure APC classloader is reset.
    

    s/APC/APCu/

  3. +++ b/sites/default/default.settings.php
    @@ -383,14 +383,23 @@
    + * It the APCu extension is not detected, either because APCu is missing or
    

    s/It/If/

fgm’s picture

Status: Needs work » Needs review
FileSize
3.98 KB

Rerolled.

Fabianx’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -245,6 +246,19 @@ public static function createFromRequest(Request $request, $class_loader, $envir
+        && extension_loaded('apcu')

+++ b/core/rebuild.php
@@ -20,7 +20,12 @@
+if (extension_loaded('apcu') && function_exists('apc_fetch')) {

Sorry for that, but both those _calls_ to extension_loaded() should be removed for now. (I know I wrote them ...)

There is a follow-up for that to solve this generically: #2447753: APCu detection erroneously succeeds if apc.enabled is "0"

After that it would be RTBC, but I can't RTBC that as I have written substantial parts of the logic.

dawehner’s picture

+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -245,6 +246,19 @@ public static function createFromRequest(Request $request, $class_loader, $envir
+    if ($class_loader_class == get_class($class_loader)
+        && Settings::get('class_loader_auto_detect', TRUE)
+        && Settings::get('hash_salt', FALSE)
+        && extension_loaded('apcu')
+        && function_exists('apc_fetch')) {

It is really odd that we need all those checks: a) can't we just fallback to the empty string if there is no hash salt? If you have no hash salt, hell is freezing already! On top of that, you could wonder whether we need to check both for the extension as well as the method. Note: We realized that require apcu leads to not being able to use it for php 5.4, because apcu doesn't exist for that version.

fgm’s picture

Status: Needs work » Needs review

Rerolled without the extension_loaded('apcu') checks, pending #2447753: APCu detection erroneously succeeds if apc.enabled is "0" resolution.

fgm’s picture

damiankloip’s picture

Echoing what some people have already mentioned in this issue, I generally think 'Fast by default' is a bad idea. But that ship has sailed, it's been pushed and there is no stopping it now.. :)

  1. +++ b/core/lib/Drupal/Core/DrupalKernel.php
    @@ -245,6 +246,19 @@ public static function createFromRequest(Request $request, $class_loader, $envir
    +      $apc_loader = new \Symfony\Component\ClassLoader\ApcClassLoader('drupal.' . Settings::get('hash_salt'), $class_loader);
    

    Could just remove the check above and use Settings::getHashSalt() instead, that will throw an exception anyway if it's not there, because yeah, you have problems anyway if you have no hash salt!

  2. +++ b/core/rebuild.php
    @@ -20,7 +20,12 @@
    +if (extension_loaded('apcu') && function_exists('apc_fetch')) {
    

    Isn't extension_loaded() enough?

  3. +++ b/core/rebuild.php
    @@ -20,7 +20,12 @@
    +  apc_clear_cache();
    

    Won't this clear out all apc system cache items? So how would that work out on shared hosting or anywhere running more than one site?

  4. +++ b/sites/default/default.settings.php
    @@ -383,14 +383,23 @@
    +# $settings['class_loader_auto_detect'] = FALSE;
    

    Seems like this is a candidate for example.settings.local.php too?

Fabianx’s picture

+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -245,6 +246,18 @@ public static function createFromRequest(Request $request, $class_loader, $envir
+        && Settings::get('hash_salt', FALSE)
+        && function_exists('apc_fetch')) {
+      $apc_loader = new \Symfony\Component\ClassLoader\ApcClassLoader('drupal.' . Settings::get('hash_salt'), $class_loader);

dawehner is right we can use:

Settings::getHashSalt() here, which throws an Exception if absent.

BUT!

Here and in settings.local.php we should not pass the hash salt as prefix to APC.

That is kinda information disclosure?

Instead we should use a hash, like:

$prefix = 'drupal' . hash('sha-256', 'drupal.' . Settings::getHashSalt());
$apc_loader = new \Symfony\Component\ClassLoader\ApcClassLoader($prefix, $class_loader);

I think we should also update the example in default.settings.php.

**brrr**

Fabianx’s picture

Status: Needs review » Needs work
+++ b/core/rebuild.php
@@ -20,7 +20,12 @@
+// Clear the APCu cache to ensure APCu classloader is reset.
+if (function_exists('apc_fetch')) {
+  apc_clear_cache();
+}

Oh, yes, we need to clear the user cache:

apc_clear_cache("user");

since apcu added BC support for that in 4.0.2.

--

#27:

1. Already proposed above (X-post)
2. Already removed
3. Should be apcu_cache_clear() or apc_cache_clear("user") instead
4. I think that is follow-up as then we should discuss the other class loader parts, too?

fgm’s picture

Status: Needs work » Needs review
FileSize
3.78 KB
1.94 KB

Rerolled for #1, #2, #3.

Regarding #4, I tend to think of settings.local.php file as the "per-instance" git-ignored file, and we probably do not want this to be such a purely local setting, do we ? At any rate, let it go to a followup if discussion is needed.

Fabianx’s picture

Status: Needs review » Needs work
  1. +++ b/core/lib/Drupal/Core/DrupalKernel.php
    @@ -250,12 +250,11 @@ public static function createFromRequest(Request $request, $class_loader, $envir
    +      $prefix = 'drupal' . hash('sha-256', 'drupal.' . Settings::getHashSalt());
    
    +++ b/sites/default/default.settings.php
    @@ -403,7 +403,9 @@
    +  $prefix = 'drupal.' . hash('sha-256', 'drupal' . $settings['hash_salt']);
    +  $apc_loader = new \Symfony\Component\ClassLoader\ApcClassLoader($prefix, $class_loader);
    

    nit - not that it matters, but it was 'drupal.' before.

    And should be consistent to the below ...

  2. +++ b/core/lib/Drupal/Core/DrupalKernel.php
    @@ -250,12 +250,11 @@ public static function createFromRequest(Request $request, $class_loader, $envir
           $apc_loader->register();
    -      $class_loader = $apc_loader;
         }
    

    Removing that line will break this?

  3. +++ b/core/rebuild.php
    @@ -22,7 +22,7 @@
    -  apc_clear_cache();
    +  apc_clear_cache('user');
    

    Thanks!

Fabianx’s picture

  1. +++ b/core/lib/Drupal/Core/DrupalKernel.php
    @@ -245,6 +246,17 @@ public static function createFromRequest(Request $request, $class_loader, $envir
    +    // If the class loader is still the same, possibly upgrade to the APCu class
    +    // loader.
    
    +++ b/sites/default/default.settings.php
    @@ -383,18 +383,29 @@
    + * If the APCu extension is detected, the Symfony APC class loader is used for
    ...
    + * If the APCu extension is not detected, either because APCu is missing or
    ...
    + * loader with another cached solution than the Symfony APCu class loader, as
    

    Should all be APC instead of APCu.

  2. +++ b/core/rebuild.php
    @@ -20,7 +20,12 @@
    +// Clear the APCu cache to ensure APCu classloader is reset.
    

    Should be: ... "APC user cache to ensure APC classloader is reset."

Fabianx’s picture

And lol!

How to get passing tests with just one line added (the test time was suspicious):

  Output: [RuntimeException: Missing $settings['hash_salt'] in settings.php. in Drupal\Core\Site\Settings::getHashSalt() (line 145 of /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Site/Settings.php).].
[03:47:13] Command [/usr/bin/php ./core/scripts/run-tests.sh --php /usr/bin/php --url http://ec2-52-5-226-233.compute-1.amazonaws.com/checkout --all --list 2>&1] succeeded.
[03:47:14] Review complete. test_id=1021283 result code=10 details=Array

Can we add the

&& Settings::get('hash_salt', FALSE)

back for now?

AjitS’s picture

Status: Needs work » Needs review
FileSize
2.73 KB
3.81 KB

I wasn't sure about #36. Could you please mention what needs to be done? Or maybe provide any link.
Implemented the suggestions from #34, and #35.

Fabianx’s picture

Status: Needs review » Needs work
  1. +++ b/core/lib/Drupal/Core/DrupalKernel.php
    @@ -246,15 +246,16 @@ public static function createFromRequest(Request $request, $class_loader, $envir
         if ($class_loader_class == get_class($class_loader)
             && Settings::get('class_loader_auto_detect', TRUE)
    

    Add here:

             && Settings::get('class_loader_auto_detect', TRUE)
    +         && Settings::get('hash_salt', FALSE)
             && function_exists('apc_fetch')) {
    
    
  2. +++ b/core/rebuild.php
    @@ -20,7 +20,7 @@
    -// Clear the APCu cache to ensure APCu classloader is reset.
    +// APC user cache to ensure APC classloader is reset.
    

    nit - classloader => class loader

    Sentence should read:

    "Clear the APC user cache to ensure APC class loader is reset."

  3. +++ b/sites/default/default.settings.php
    @@ -383,18 +383,29 @@
    +  $prefix = 'drupal.' . hash('sha-256', 'drupal' . $settings['hash_salt']);
    +  $apc_loader = new \Symfony\Component\ClassLoader\ApcClassLoader($prefix, $class_loader);
    

    nit - 'drupal.' for the 2nd 'drupal string, too.

AjitS’s picture

Status: Needs work » Needs review
FileSize
1.08 KB
3.87 KB

Thanks! Implemented the suggested changes.

Status: Needs review » Needs work

The last submitted patch, 39: use_apc_classloader_by-2296009-39.patch, failed testing.

fgm’s picture

Rerolled with suggested changes, and changed insteand of "classloader" in comments and variable names to "class loader" (in comments) or class_loader (in variables) for consistency with Fabianx' suggestion.

Also changed the hash name from nonexistent "sha-256" to existent "sha256".

Fabianx’s picture

Status: Needs work » Needs review
Fabianx’s picture

Thanks, great catch!

webchick’s picture

Issue tags: +D8 Accelerate Dev Days

Tagging.

Fabianx’s picture

This is ready for RTBC by someone else now.

Wim Leers’s picture

+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -245,6 +246,19 @@ public static function createFromRequest(Request $request, $class_loader, $envir
+    // If the class loader is still the same, possibly upgrade to the APC class

+++ b/core/rebuild.php
@@ -20,7 +20,12 @@
+// Clear the APC cache to ensure APC class loader is reset.

+++ b/sites/default/default.settings.php
@@ -383,18 +383,29 @@
+ * If the APCu extension is detected, the Symfony APC class loader is used for

Nit: s/APC/APCu/

Fabianx’s picture

Status: Needs review » Needs work

Oh, yes this misses the changes done in https://www.drupal.org/node/2296009#comment-9820047 interdiff.txt.

We should for now speak of APC everywhere.

fgm’s picture

Status: Needs work » Needs review
FileSize
6.17 KB
11.24 KB
11.97 KB

Rerolled accordingly. I also removed all other reference to APCu in comment text in other files, for consistency.

Fabianx’s picture

I think lets hold off on converting all of the code base APCu to APC for now, APCu as in backend means APC user cache, which is correct.

But APC as in here, means the APC class loader (which happens to use the user cache as well, but is called APC class loader by Symfony upstream).

fgm’s picture

Does that mean you want to restore the APCu names in some files, then ? (which ones ?) At this point, they are /all/ converted, which means that our commenting is consistent, which would not be the case otherwise.

Fabianx’s picture

Issue tags: +Needs beta evaluation

fgm, I thank you for all your efforts here.

The idea to make it all consistent is great, but from my experience it can delay patches by weeks or months if a patch changes too much unrelated code. The trick (I learned via Wim Leers) is to concentrate _one_ issue very focused on the changes that are relevant to the issue and if there is an inconsistency to open a follow-up, where then all discussions take place (and usually they will take place ...).

Wouldn't it be great if this patch was committed during the next few days?

I would assume so.

To the issue:

We have a calls called ApcuCacheBackend, but we change all comments to speak of APC, that is also inconsistent, so do we rename the Cache backend then as well?

--

What needs to happen to get this committed quickly (from my experience):

- All code that is added is consistent in itself
- The $classloader vs. $class_loader change is _in_ scope, because the inconsistent naming almost led to bugs, which are hard to spot => in scope
- All other core code is unchanged (leave APCu cache backend alone, but open a follow-up if it bothers you)
- Update the issue summary
- Add a beta evaluation

fgm’s picture

OK, first the patch itself : is this what you meant ?

Regarding the summary : I think there used to be one in this issue, but it is empty now ?

Beta evaluation: is this what is described at https://www.drupal.org/node/2373483 ?

fgm’s picture

Grrr, patch and interdiff upload lost again...

Fabianx’s picture

Yep, that looks great now! :)

For the issue summary and beta evaluation the best is to install http://dreditor.org and use the "INSERT TEMPLATE" / "INSERT BETA EVALUATION" buttons :).

fgm’s picture

Issue summary: View changes
fgm’s picture

Issue summary: View changes
Fabianx’s picture

Looks good now! Generally profiling was always in favor of APC class loader. The added hash() call however should be profiled and re-evaluated by a core committer.

Assigning to catch and pinging Alex Pott for that.

catch’s picture

The single hash() call doesn't concern me - just doing that once per request.

Wim Leers’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: -needs profiling
Related issues: +#2471597: Create comprehensive documentation for running Drupal in a "cloud-native" or 12-Factor App model

IS & beta evaluation look great. Created the follow-up #2471597: Create comprehensive documentation for running Drupal in a "cloud-native" or 12-Factor App model. Thank you so much for pushing this to the finish line, @fgm!

@Berdir and I don't see why this would need additional profiling. Definitely not just for the hash() call: http://3v4l.org/L0XCF/perf#tabs.

Fabianx’s picture

Assigned: catch » Unassigned

RTBC + 1, Thanks catch!

  • catch committed e3573a2 on 8.0.x
    Issue #2296009 by fgm, Fabianx, AjitS: Use APC Classloader by default (...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.0.x, thanks!

Wim Leers’s picture

This is really going to make a huge (~5%) difference for all hosts with a supporting environment: https://groups.drupal.org/node/464283#comment-1100578

mpdonadio’s picture

+++ b/core/rebuild.php
@@ -20,7 +20,12 @@
+// Clear the APC cache to ensure APC class loader is reset.
+if (function_exists('apc_fetch')) {
+  apc_clear_cache('user');
+}

Why is this only done in rebuild.php and not drupal_flush_all_caches() and/or DrupalKernal::rebuildContainer()?

alexpott’s picture

Status: Fixed » Closed (fixed)

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

david_garcia’s picture

Created a follow-up with a patch to allow automatic detection of other optimized classloaders that are already supported by symfony.

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