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
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. |
Comment | File | Size | Author |
---|---|---|---|
#53 | interdiff-2296009-41-52.txt | 1.27 KB | fgm |
#53 | 2296009-52-apc_classloader.patch | 7.08 KB | fgm |
#29 | 0001-Issue-2296009-Use-APC-Classloader-by-default-when-av.patch | 3.9 KB | fgm |
#33 | interdiff-2296009-29-33.txt | 1.94 KB | fgm |
#39 | interdiff.txt | 1.08 KB | AjitS |
Comments
Comment #1
BerdirI'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 :)
Comment #2
Wim Leers#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.)
Comment #3
dawehnerWell, 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.
Comment #4
BerdirNo, 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.
Comment #5
sunI 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. Rewritessettings.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 asenv.php
. Shows a simple list of (enabled) checkboxes for suggested performance optimizations (which can be toggled off). (Re)writessettings.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 toenv.php
.Bottom line: Think about site instances and varying environments, instead of "a" single site.
Comment #6
Fabianx CreditAttribution: Fabianx commentedHere 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.
Comment #7
Fabianx CreditAttribution: Fabianx commentedLittle problem in detection logic, need to use extension_loaded('apcu') as apcu_enabled is just available in 4.0.7.
Comment #8
BerdirDo 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?
Comment #10
BerdirThat 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.
Comment #11
BerdirUhm, 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.
Comment #12
Fabianx CreditAttribution: Fabianx commentedI 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.
Comment #13
Fabianx CreditAttribution: Fabianx commentedComment #14
Fabianx CreditAttribution: Fabianx commented#8:
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.
Comment #15
Wim LeersSilly nit: 80 cols.
Comment #16
BerdirRe #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.
Comment #17
Fabianx CreditAttribution: Fabianx commentedAgree, 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.
Comment #20
fgmPatch no longer applied, rerolled on current HEAD.
Comment #21
Wim LeersGo fgm! Thanks for tackling this one!
Comment #22
Fabianx CreditAttribution: Fabianx commentedCNW for the documentation updates and a 'sane' way to say we don't want to use the default classloader detection.
Comment #23
fgmHow about this wording ?
Comment #24
Wim LeersQuick round of nitpicks
80 cols.
s/APC/APCu/
s/It/If/
Comment #25
fgmRerolled.
Comment #26
Fabianx CreditAttribution: Fabianx commentedSorry 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.
Comment #27
dawehnerIt 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.
Comment #28
fgmRerolled without the extension_loaded('apcu') checks, pending #2447753: APCu detection erroneously succeeds if apc.enabled is "0" resolution.
Comment #29
fgmForgot to upload patch.
Comment #30
damiankloip CreditAttribution: damiankloip commentedEchoing 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.. :)
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!
Isn't extension_loaded() enough?
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?
Seems like this is a candidate for example.settings.local.php too?
Comment #31
Fabianx CreditAttribution: Fabianx commenteddawehner 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:
I think we should also update the example in default.settings.php.
**brrr**
Comment #32
Fabianx CreditAttribution: Fabianx commentedOh, 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?
Comment #33
fgmRerolled 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.
Comment #34
Fabianx CreditAttribution: Fabianx commentednit - not that it matters, but it was 'drupal.' before.
And should be consistent to the below ...
Removing that line will break this?
Thanks!
Comment #35
Fabianx CreditAttribution: Fabianx commentedShould all be APC instead of APCu.
Should be: ... "APC user cache to ensure APC classloader is reset."
Comment #36
Fabianx CreditAttribution: Fabianx commentedAnd lol!
How to get passing tests with just one line added (the test time was suspicious):
Can we add the
&& Settings::get('hash_salt', FALSE)
back for now?
Comment #37
AjitSI 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.
Comment #38
Fabianx CreditAttribution: Fabianx commentedAdd here:
nit - classloader => class loader
Sentence should read:
"Clear the APC user cache to ensure APC class loader is reset."
nit - 'drupal.' for the 2nd 'drupal string, too.
Comment #39
AjitSThanks! Implemented the suggested changes.
Comment #41
fgmRerolled 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".
Comment #42
Fabianx CreditAttribution: Fabianx commentedComment #43
Fabianx CreditAttribution: Fabianx commentedThanks, great catch!
Comment #44
webchickTagging.
Comment #45
Fabianx CreditAttribution: Fabianx for Acquia commentedThis is ready for RTBC by someone else now.
Comment #46
Wim LeersNit: s/APC/APCu/
Comment #47
Fabianx CreditAttribution: Fabianx for Acquia commentedOh, 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.
Comment #48
fgmRerolled accordingly. I also removed all other reference to APCu in comment text in other files, for consistency.
Comment #49
Fabianx CreditAttribution: Fabianx for Acquia commentedI 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).
Comment #50
fgmDoes 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.
Comment #51
Fabianx CreditAttribution: Fabianx for Acquia commentedfgm, 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
Comment #52
fgmOK, 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 ?
Comment #53
fgmGrrr, patch and interdiff upload lost again...
Comment #54
Fabianx CreditAttribution: Fabianx for Acquia commentedYep, 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 :).
Comment #55
fgmComment #56
fgmComment #57
Fabianx CreditAttribution: Fabianx for Acquia commentedLooks 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.
Comment #58
catchThe single hash() call doesn't concern me - just doing that once per request.
Comment #59
Wim LeersIS & 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.Comment #60
Fabianx CreditAttribution: Fabianx for Acquia commentedRTBC + 1, Thanks catch!
Comment #62
catchCommitted/pushed to 8.0.x, thanks!
Comment #63
znerol CreditAttribution: znerol commentedFollow-up: #2474817: DrupalKernel::classLoader not updated when switching to apcu either through settings.php or automatically.
Comment #64
Wim LeersThis is really going to make a huge (~5%) difference for all hosts with a supporting environment: https://groups.drupal.org/node/464283#comment-1100578
Comment #65
mpdonadioWhy is this only done in rebuild.php and not drupal_flush_all_caches() and/or DrupalKernal::rebuildContainer()?
Comment #66
alexpottComment #68
david_garcia CreditAttribution: david_garcia commentedCreated 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