Problem/Motivation
Using a PSR-0 class loader requires a file_exists()
check on paths to make sure the class exists when it's to be loaded. APC can be used to help speed it up, but performance still could be improved, even with/without APC present.
Proposed resolution
Composer can generate a cached 1:1 classmap of classes to be loaded. This means that when a class is to be loaded, it will already know exactly where the file is, without having to do a file_exists()
check on the PSR-0 path.
To generate this static classmap, you download Composer and run a dump-autoload --optimize
on core:
$ cd core
$ curl -s https://getcomposer.org/installer | php
$ php composer.phar dump-autoload --optimize
This optimization is already provided in the attached patch. For contributed modules, and new classes introduced during development, the PSR-0 paths are still registered so that classes are still loaded properly. They just won't gain benefit of the static classmap.
Remaining tasks
- Get a patch up and running
- Benchmark and compare against #1658720: Use ClassLoader instead of UniversalClassLoader
User interface changes
No user interface changes.
API changes
Switches drupal_classloader()
to use Composer\Autoload\ClassLoader.
Comment | File | Size | Author |
---|---|---|---|
#71 | classes.txt | 105.63 KB | Mile23 |
#71 | interdiff.txt | 1.54 KB | Mile23 |
#71 | 1818628_71.patch | 3 KB | Mile23 |
#66 | use_composer_s-1818628-65.patch | 23.61 KB | timmillwood |
#44 | use_composer_s-1818628-44.patch | 363.99 KB | Fabianx |
Comments
Comment #2
RobLoachWas missing the annotation namespace fix.
Comment #3
Crell CreditAttribution: Crell commentedI'm not sure I'm down with an autoloading mechanism that makes contrib a second class citizen. Admittedly with Views in core that eliminates about 2/3 of all classes in contrib right now, but presumably that will change over time. :-)
Comment #4
Fabianx CreditAttribution: Fabianx commentedImprovement for non-APC:
Can we instead of using Composers add a ClassMap cache loader in front of APC / Symphony class loader as Decorator?
And then use drupal_save / drupal_load and generate class map once on the fly so it only works for added contrib modules?
Comment #5
RobLoachThanks for posting the benchmarks, Fabian. Having Drupal generate a classmap for us is an interesting idea. Do you have any additional information on how that would be accomplished?
Comment #6
Fabianx CreditAttribution: Fabianx commentedWell, could we just implement our own dump command called from inside of Drupal:
http://drupalcode.org/project/composer.git/blob/c7d084dafe1a5a81a6bf68af...
If we set the target_dir to a temp dir (instead of composer) and write the files from /tmp/... via drupal_php_storage('composer')->write('filename') I think we could then load the classmap file directly via drupal_php_storage('composer')->load('classmap.php') and then implement our own Decorator.
That was the idea.
Comment #7
catchUsing a class map including all classes on a Drupal site is going to mean it has a lot of classes in it that will never be used - think of views plugins from contrib and similar - even for enabled modules, let alone disabled ones. It might be quicker than the universal classloader we're using now, but we should check it against Symfony's APC classloader for comparison.
I'd like to look at building the class map on-demand and writing a single entry to APC at the end of the request if new classes are found - similar to how CacheArray works. That means one APC get per request, occasional sets, no compile step and only fetching the classes that are used. We could then use that class entry to write a classmap if we wanted too. I mentioned this to Seldaek at Symfony Live London and he said he'd consider something like that with benchmarks so it probably makes sense to do it in a PR to Composer.
Comment #8
xjmComment #9
quicksketchOver in #1971198: [policy] Drupal and PSR-0/PSR-4 Class Loading, I did some basic profiling on our current situation. This is essentially a duplicated post from over there.
In our current ClassLoader.php file, here are the numbers for calls to file_exists():
Using a simple script to test calls to file_exists() benchmarks the importance of file_exists():
For files that do not exist, 100,000 calls takes an average of 0.55 seconds. For files that do exist, 100,000 calls takes an average of 0.35 seconds after the first call. The first call itself if the files exist take the longest of all, 2.5 seconds. Calling 100,000 calls on the same file is essentially free, taking 0.007 seconds. Running this script with APC turned on or off has no significant impact.
So basically, file_exists() has a small amount overhead, and only slightly more if the files are not found, but at the scale we're currently seeing, it's not enough to worry about. I had expected thousands of calls and a lot more missing files. In total, we're only spending 0.00035 seconds per page load in the Symfony autoloader's findFile() method. So it's not slow in the least bit.
---
So in this situation, it appears that the ClassLoader is not at all expensive, even in its current state. The calls to file_exists() aren't free, but they're not particularly expensive either. One caveat in my benchmarking is that this is done on my localhost, which has an SSD, but even OS-level caching would likely mitigate repeated calls to the same files.
Comment #10
quicksketchHm, upon reflecting on @Fabianx's numbers above, I'm curious how this patch resulted in what looks like a 2% overall improvement(?) What's actually being benchmarked in #4?
Comment #11
effulgentsia CreditAttribution: effulgentsia commentedOver in #1744302-17: [meta] Resolve known performance regressions in Drupal 8, I saw a 10% difference (185ms vs. 166ms) between using Symfony\Component\ClassLoader\ClassLoader and Symfony\Component\ClassLoader\ApcClassLoader. That's on a 2010 MacBookPro with a non-SSD drive. According to #1744302-23: [meta] Resolve known performance regressions in Drupal 8, some of that is not due to file_exists(), but I think a good portion still is, at least on Mac 10.6 with non-SSD.
Comment #12
quicksketchYeah, it's possible that some of it is merely in the looping that the autoloader goes through. On a normal page load it does over 10,000 strpos() checks. With a straight namespace-to-location map, you can skip all of those.
Comment #13
donquixote CreditAttribution: donquixote commentedQuicksketch (#12):
Exactly.
I did some experiments with http://github.com/Seldaek/autoload-bench.
From #2023325-25: Add interface for classloader so it can be cleanly swapped.
My experiments basically confirmed this observation. It is definitely possible to write a customized version of Composer's loader that is optimized for Drupal, and significantly faster.
(Of course, class map is still ahead of that.)
Every additional predictor character adds a small overhead, esp because we need to support the case where the class name or prefix is too short for the additional predictors. To further optimize this, we really need to do benchmarks. E.g. if you have 400 moudules and themes enabled, then additional predictors will be a blessing. Otherwise, additional predictors will cost more than they help.
The Krautoload loader is also immune to the loop problem, but it is slowed down if class names get too long, and it has an additional level of indirection. If predictor indices are chosen wisely, the predictor technique is more effective than what Krautoload does atm.
(Currently my fork of autoload-bench is a mess, but I am going to improve it and share my results.)
For the class map:
It will clearly be useful, but I think the better solution is to ship Drupal without a classmap by default, and then have a command that can generate a classmap for everything (including modules), not just core.
Comment #14
donquixote CreditAttribution: donquixote commentedHere's the benchmark fork:
https://github.com/donquixote/autoload-bench
I am not going to post numbers here, because it would be a lot, and even more to actually explain what they mean...
Just some quick conclusions:
- As expected, classmap is always the fastest. There are different loaders that support classmap, and there is hardly any relevant difference between them.
- findFile() adds a significant overhead. It is better to directly do stuff in loadClass(). Method indirection is expensive.
- Composer (the old one without predictor) and Symfony are generally ok, but get on their knees if there are too many namespaces/prefixes registered.
- A modified composer loader with additional predictor characters clearly beats other loaders in many-namespaces-same-vendor scenarios, and the predictor stuff adds only a tiny overhead in scenarios with fewer namespaces.
- We can add PSR-4 support with very little overhead.
Comment #15
RobLoachThe benchmarks posted in #2038135: Use the Composer autoloader to make everything simpler show that using the optimized class loader, without APC, is much faster than without the optimized class loader. This is true even without having the optimizations done on the modules. So, let's change the scope of this issue to only run the optimized class loader on
Drupal\Core
andDrupal\Component
. We could push having it run on core/contributed modules in a follow up.Comment #16
donquixote CreditAttribution: donquixote commentedCool.
Will we keep an option to run it without classmap?
As I understand, we want to commit the generated classmap into the repository, but the developer still has the option to run composer install/update again without --optimize, thus reverting to non-classmap loading (*). Then this developer can add the composer folder to .git/info/exclude, and that's it.
Seems like a decent solution.
And everything else can be done in followups.
(*) The only reason to disable the class map would be if you (as a core developer) remove or rename a class, and then want file_exists() on that class to return false without crashing, without rebuilding the classmap. This could happen if the class is still somewhere in a plugin registry or similar.
It might not be an important case, but it is still good to know that you can opt out of the classmap.
Comment #17
donquixote CreditAttribution: donquixote commentedOh, I only just now realized we are adding all core module namespaces, even for disabled modules!
Will this have any side effects?
One side effect we get, if we don't pay attention, is enabled core module namespaces will be added twice to the the class loader. One time by Composer itself, another time by DrupalKernel.
This might not bother us at all, because it only means that
$prefixes['D']['Drupal\system']
will have two entries instead of one. Unless we have 1x$prefixes['D']['Drupal\system']
and 1x$prefixes['D']['Drupal\system\\']
.This will not have an effect as long as no contrib is in play, because the loader will never hit the point where it has to iterate over those namespaces.
Only whenever the loader wants to load a contrib class, it first has to iterate over the core module namespaces, which if we don't pay attention will be twice as long.
Btw, it should be preferable to always do
'Drupal\system\\'
*with* trailing slash, to avoid false positives. E.g.'Drupal\menu'
also matches classes beginning with'Drupal\menu_link'
, which is not intended.Otherwise, side effects only occur if some piece of code abuses class_exists() to check whether an extension is installed. Not a good idea in the first place, and I don't think we are currently doing that anywhere in core.
I'd say the approach taken here is ok for now, but we definitely should work on a solution that builds a dynamic class map depending on which modules are enabled.
Comment #18
RobLoachNo, this issue is not for modules. Just the
Drupal\Core
andDrupal\Component
namespaces. The patch is old and needs to be updated, is post-poned for the Composer issue. Let's handle modules in a separate issue.Comment #19
catchOn https://drupal.org/node/2023325#comment-7858687 RobLoach showed that /core classes (no modules at all) means a classmap of around 288k. Default APC maximum file size is 1mb. So just doing it for /core classes we won't run into issues, but I'd expect us to go over that if we tried to cover core and contrib modules with a classmap (if we were able to figure out how to do that).
Comment #20
catchComment #21
moshe weitzman CreditAttribution: moshe weitzman commentedTHis was discussed by the Performance team and we agree that this is a good idea. If anyone would take it on, that would be welcome.
Comment #22
neclimdulI assume this just needs a new dump since the "Use composer for everything" issue has been in for a while.
I think for now we just cover /core/lib since that'll be a easy win and there are no problems of scale like catch mentioned.
The follow up I guess would probably be to use
Composer\Autoload\ClassLoader::addClassMap(array $classMap)
manually like we manually register each namespace to register additional class maps which could exist in separate files including compiled files in secure storage.Comment #23
effulgentsia CreditAttribution: effulgentsia commentedPatch in #22 includes /core/vendor in addition to /core/lib. Since this is Composer, that kind of makes sense, but I don't see comments in this issue that explicitly discussed whether that's desirable. Especially for libraries not needed by most requests: like Doctrine, Guzzle, and Zend\Feed, I wonder whether the added memory usage to every request is worthwhile.
Removing the Needs Profiling tag since that was done in #15 and earlier comments.
If adding /core/vendor as #22 does is acceptable, is there anything left to do here before this is RTBC?
Comment #24
neclimdulI don't think its been discussed because as far as I can tell there isn't really a option to do that sort of thing. We'd have to manually maintain the dump outside of composer which doesn't seem desirable.
https://getcomposer.org/doc/03-cli.md#dump-autoload
Comment #25
sunYou can specify individual directories to add to the auto-generated class map via
composer.json
in this way:That's how PHPUnit does it. — And btw, because PHPUnit does it, PHPUnit's full/total classmap clutters our runtime classmap in substantial ways...
Comment #26
neclimdulNeeded a re-roll. Interdiff does identify downside to this. We'll need to run this every-so-often as new classes get added.
Comment #27
Fabianx CreditAttribution: Fabianx commentedSo we can do something much cooler in addition as discussed today with @donquixote:
We can add a minimal class map to core using core/lib and symfony like done here for all things we need to load the container, which nicely avoids the downside of rebuilding class map again and again ...
and for modules and the rest we can store the classmap in ....
TADA! - the container
The container is loaded from disk anyway, large anyway, does not need to be cached and a rebuild of container rebuilds the class map.
- During container rebuild => persist new class map in container using compiler pass, dump contrib module classes, too as part of that.
- After container loading -> call addClassMap - as simple as that.
If we are really worried about size of the file (opcache has no file limit, its configurable in APC), we could output a warning once container gets too large if APC is used and setting is too low. #fastbydefault
Just need a flag in settings.php to not use class_map so development works, add to example.settings.local.php and we are done.
Comment #28
Crell CreditAttribution: Crell commentedThat really sounds like over-optimization to me, frankly...
Comment #29
andypostI need at least a method to add my class-map, for example using some components from symfony's bundles
Comment #30
Fabianx CreditAttribution: Fabianx commented@andy: You can add your class map - to the container, too. If we add like an alter hook or event or whatever so that you can react on the class map building,
@Crell: No, its not. It is solving the core vs. contrib problem. The actual implementation detail does not matter that much. (e.g. directly in container vs. in seperate PHP storage files chunked via array_chunk - as discussed with berdir).
It is just an array in memory. Some other structures in core are _way_ larger ... So that is not a real concern, IMHO.
But yes need definitely profile how long the rebuild of container takes before / after.
We would also nicely avoid commit conflicts as our "root" class map only needs to have what is needed to build the container.
In terms of complexity:
20 lines of code I guess plus around 30 to port the broken class map dumper to be only PSR-0 / PSR-4 to where Drupal would find things. Plus tests obviously.
Comment #31
EclipseGc CreditAttribution: EclipseGc commentedI think there are actual hooks in composer that could be used to get an accurate class map. I've not studied enough to know for sure, but check before you reinvent that wheel.
Eclipse
Comment #32
catchTalked to donquixote today about the APC max_file_size issue (only an issue on PHP4, Opcache has no limit apparently) and this seems like a good workaround for that! Didn't think of that this afternoon but solves that blocker at least.
Overall I'm not sure about this issue compared to trying to further optimize the APCu classloader, but if we do this by default and still let people swap in the APCu loader and other variants if they want to, that's completely fine. Would be great to fix the 'fast by default' issue without APCu being a hard requirement for that if we can. Then it's just a question of profiling to ensure it really helps.
Fixing the contrib issue is not an over-optimization, it's fixing a fundamental weakness of the classmap approach which was promised early-on as a perfect solution to the slow default autoloader, despite there being no viable plan to handle modules at all. The default composer autoloader is still one of the worst out of the box performance issues in 8.0.x and it would continue to be bad if we did a half-solution of adding only core classes to the classmap and falling back for everything else.
Even with the optimization of chunking files, there's still the potential for hundreds of unused classes being included in the classmap. See also #2315545-11: Install composer dependencies to D8 when packaging.
Comment #33
Fabianx CreditAttribution: Fabianx commentedComment #34
effulgentsia CreditAttribution: effulgentsia commentedIf this improves performance, I think this is more of a task than a feature request.
Comment #35
Fabianx CreditAttribution: Fabianx commentedI took another look at that and here is a quick patch that lets us use a classmap until APCClassLoader takes over.
Note for all of core/lib/Core and core/lib/Component and what is in the patch, it is only 162k. I am holding off on that however, because this will mean any rename, will need a composer classmap re-generate, which would be a pain with out default process.
The aim of this minimal initial patch is:
a) Ensure our most used external components are in the class map (could discuss to add routing, too)
b) Ensure Drupal is fast for PageCache - case
c) Ensure everything is class mapped that is needed to install the APCClassLoader fast.
This is the MVP and gives us performance for free, autoload_classmap.php is afterwards 129 kB, which should be okay.
(Ah, yes, it saves about 1-2 ms for the page cache case of 10-12 ms - obviously depending on the performance of file_exists().)
It turns out the APCClassLoader is not optimal though, because apc_fetch() always happens, even though it would not be necessary as its in memory.
The optimal class caching class loader that works in correspondance with composers classmap:
- Gets the class map from composer when constructed
- implements getClassMap and addClassMap calls, so it can update the internal classmap
- Just if it not finds a match in class map reaches out to apc_fetch() / apc_store()
The attached patch makes Drupal faster for both APC classloader and non-APC classloader cases, so a win on both fronts.
BTW.
Even for _all_ of current core/ and vendor/ we only have 591 kB in the classmap file ...
We might consider at least vendor/ (442 kB) for now.
Comment #36
Fabianx CreditAttribution: Fabianx commentedSleeping over this again, I am very positive, we can use a pure class map based approach in core:
- We have the Symfony class-loader component
- Composers dump-class map is nothing besides a copy of Symfony's dumper
- It is perfectly feasible to dump all modules during container rebuild
In comparison all of core/ and vendor/ takes 1.2s to dump and a peak of 10.03 MB.
That means by just re-building the classmap sometimes in core with 'composer dump-autoload -o' is enough for core and the initial class map. (426 kB as of now)
For all modules to do it during composer dump is simple.
I would suggest we store a list of classmaps in the container:
%classmaps = array('php-storage:
', 'module/mymodule/extra-classmap.php')
This first classmap is auto-generated from all modules (when we do the Yamls parsing, etc.)
Then on container load, we load all the classmaps stored in the %classmaps into the autoloader.
With all of core/ and vendor/ being less than 500 kB, I am positive array_chunk will not be needed for a long time, also having classmaps be in the container makes it as easy as registering a Service Provider to add a new class map to the container (and hence fixing the concerns of contrib authors).
Modules using composer with its own vendor/ can even point to the generated composer classmap and not having to create their own.
Also as we have all standardized on PSR-4, we obviously can optimize by only searching src/, also renames lead to class rename and such are not conflicting with a class map.
We just need to re-generate core's classmap whenever we ship a new (beta) release, which is one command and one git commit and thats it.
Comment #37
Fabianx CreditAttribution: Fabianx commentedIt can be _way_ simpler.
The problem with the 1 MB APC limit still remains, but a container parameter is the most clean and easiest solution as it also solves the dumping problem nicely.
Container before is 392k, afterwards 604k, class map generation time is minimal.
In this patch:
- Composers optimize flag used to generate classmap for all known static things
- Classmap generated for all modules src/ automatically
- Can be changed by service providers if they want (e.g. also removed things they don't want)
- Can be disabled via settings.php via $settings['classloader_use_container_classmap'] = FALSE, which is suggested for APC classloader, too.
This shows my original idea of #4 is feasible.
Edit:
- The composer generated optimized classmap does not contain any modules, so the classmap of the container complements the the composer classmap.
- I don't think the problem of unused classes is a big one and its good enough performance for most sites that I would consider it okay to enable by default, but could be disabled for really low memory requirements.
Comment #39
Fabianx CreditAttribution: Fabianx commentedEven with that many exceptions it seems is 2 minutes faster for test bot time! (13 min vs. 15 min).
Comment #40
Fabianx CreditAttribution: Fabianx commentedThis approach is pretty much finished, but is missing docs:
In this patch:
- container.classmap is only used for modules wanting to add their own libraries, etc. to the class map
- Explicit functions to dump / load the classmap using the same php storage, but a different class name
- Using a real class with a get() function
- Compressing if zlib extension is supported.
Compression brings the class map down to 43 kB on disk and compressing takes 6 ms, while decompressing takes less than 1 ms, which should be acceptable.
Therefore we should never hit the 1 MB problem and this is solved.
Lets see what testbot says.
Comment #41
Fabianx CreditAttribution: Fabianx commentedComment #42
Fabianx CreditAttribution: Fabianx commentedLast testbot was 43 min, oops, lets see what different enabled vs. disabled makes.
In this patch:
- Used container.classmap_files[] instead of classmap to avoid big data stored in container
- This is merged into the global classmap
- Used container.use_classmap so that load() / store() are not called needlessly
- Renamed the setting to classloader_dump_modules_classmap, which accurately describes what it does
Lets see what testbot says now.
Comment #43
neclimdultestbots run times are not very consistent. Nor are they representative of real world metrics so I don't think we can use that as a valid benchmark for anything so lets not focus on that.
Comment #44
Fabianx CreditAttribution: Fabianx commented#43 true, but 43 min as default configuration is likely a problem just for the test suite, because the container is re-build again and again.
Here are the benchmarks of the latest patch:
This compares core with APC classloader vs. this patch with no APC classloader.
This is on PHP5.5 with opcache and apcu profiled the default frontpage anonymously for 1000 times each.
This means the static classmap is on par with the APC file loader.
What takes time (0.5 ms) is adding the class map to the class loader and loading the file in (0.2 ms), but the APC requests take in sum around 0.5 ms, too.
I disabled compression unless the file got too big now, because it made the class map class loader slower in benchmarks.
I am pretty happy with this now, for around 1 MB of memory the performance of the normal class loader is on par with the APCu Class Loader.
I will open a follow-up issue to enable the APCClassLoader by default when APCu is detected - as there is no reason not to.
With this we would have:
- APC for APCu available.
- ClassMap for all other environments and static seeding wanted.
The only thing I think we could still do (but that might also be over-optimization) is to add those 12 classes needed before APCClassLoader to the default autoload_classmap.php via composer.json AND then put the current contents of the autoload_classmap.php to e.g. core/classmap.php and add that as file in 'container.classmap_files', so we only load one big file and don't load a big classmap for core/lib/* and vendor/* when APCu class loader is active.
This is ready for some serious review, but should give the power to the user.
PS: Again the default is disabled here to have a fast test bot, so dumping is not really tested here ...
Comment #45
BerdirThere are existing issues, and some reasons to do this. One is that sometimes you can do something that breaks the apc classloader, so that it expects a class to exist in a certain file but it doesn't. And then you need to restart apache to fix it. In those issues, I think it was suggested to clear the apc cache in rebuild.php to solve this.
Comment #46
Fabianx CreditAttribution: Fabianx commentedComment #47
Fabianx CreditAttribution: Fabianx commentedhttps://www.drupal.org/node/2296527#comment-8934569 has a really nice unit test to ensure this does not regress for vendor or core/lib.
Still needs reviews.
Comment #48
Fabianx CreditAttribution: Fabianx commentedComment #49
Mile23+1 on #28 and #31.
Trade file_exists() for gobs of memory use as an optimization in a beta phase? Bad idea.
The current
core/composer.json
file has classmap literals already, ostensibly for this reason:So this issue should be: Create a metric for determining which classes should be in this existing classmap array, and then add classes which meet this metric.
That means performing an analysis of class use, rather than speed of execution or memory use. Has such an analysis been done in this issue or another one?
Also, once this classmap is in
core/composer.json
, it means that the optimization goes away when the user usesDRUPAL_ROOT/composer.json
, since the core one would then no longer be the project root. Note that this is all use-cases where anyone uses composer to manage anything at all with Drupal.One way around this would be to create an autoload map file in a global location that looks a lot like the classmap from
core/composer.json
. This solution is simlar to #2406681: Add an autoload.php in the repo root to control the autoloader of front controllers which has us chasing our tails because we're doing Composer wrong. But we could, and it would be far desirable to eating up gobs of memory with classmaps that remain unused in most cases.After that classmap is either in composer.json or in an external classmap file, it becomes a question of which classes should eat memory in favor of speed, and that's a whole other process.
As for modules: If we try to avoid loading classmaps for modules which are disabled then we end up with a worse problem than the original issue: We are dependent on the database to discover which modules are enabled.
file_exists()
looks attractive by comparison.One of many reasons phpunit should be 'require-dev' instead of 'require'.
Comment #50
Mile23Comment #51
Fabianx CreditAttribution: Fabianx commentedPlease read the actual patch in #44. This is for sites not using the APC class loader and its not that much memory.
The patch in #44 was created before that optimization took place.
The original classmap for core/ and vendor/ has been proposed by fabpot of Symfony fame as well.
Comment #52
Mile23Comment #53
neclimdulThat is an interesting observation. We've got 2 testing libraries in the patch and when is updated 5 because mink and goutte libraries where added recently for the mink testing issues. Having those in production classmaps is not ideal obviously. Moving them to require-dev definitely allows us to build a classmap that only contains actual production code but doing so requires a composer update(which updates a lot of libraries no related to this patch) and then removing the dev libraries and dumping the autoloader(composer.phar install -o --no-dev). All of that is going to be a big pain while we insist on tracking /core/vendor in git. It seems like our time would be better spent solving that problem and then agreeing to package with -o --no-dev.
Comment #54
Fabianx CreditAttribution: Fabianx commentedThe difference currently is 42 KB if I move phpunit to require-dev.
This is probably something we should just do and its simple to do with just a simple command.
Do we have an issue for that already?
Comment #55
Fabianx CreditAttribution: Fabianx commentedOpened #2444615: Move testing packages to require-dev in composer.json with a patch for that.
Comment #56
jhedstromComment #57
webchickTagging.
Comment #60
Wim LeersBumping this to major because the performance gain potential is quite large. Of course, we now have #2296009: Use APC Classloader by default (when available), so the additional gain may not be that large. But we should do another round of profiling, figure out whether this is worth it. If it's not, then we can close this. Otherwise, this is another gain.
Comment #61
neclimdulWas there any motion on this at devdays? It seems like we stalled because of needing some policy thought around #2444615: Move testing packages to require-dev in composer.json
edit: fix typo
Comment #62
tstoecklerRe #60: In contrast to the APC classloader this is something that would greatly affect low-fi / shared hosting / ... environments, right? So definitely seems worth doing although profiling (with + without APC) would of course be great.
Comment #63
Fabianx CreditAttribution: Fabianx commented#62:
#1818628-44: Use Composer's optimized ClassLoader for Core/Component classes has profiling data against APCu class loader.
It is quite the same overhead and comparable in performance, we neither gain something, but also don't loose much in comparison.
Comment #64
dawehnerWell, but this is great for hosted sites, they would otherwise just have a way slower classloader implementation.
Comment #66
timmillwoodQuickly re-rolled the patch.
Comment #67
Crell CreditAttribution: Crell at Palantir.net commentedNot really WSCCI...
Comment #68
Mile23We don't need a classmap. It's a memory hog for some classes that might not ever be used.
Instead, we can just register an autoloader that knows where
Drupal\
-namespaced classes live on the file system. Since we know their position relative to the autoloader, we can justinclude
them without afile_exists()
. If this fails, then there are bigger problems than this patch.It's called
DrupalNamespaceLoader::autoload()
. It's prepended to the queue of autoloaders, so it gets the first chance at finding classes. We limit this autoloader toDrupal\Core
andDrupal\Component
namespaces, because those are the only ones we know the location for.Other namespaces fall through and are handled by Composer's loader or the loaders in the kernel.
The question is where to start it up... I have it being registered in the constructor of
DrupalKernel
, but it could be the front controller or any of a number of places. We just don't want to register it more than once.The change is included in the patch, but you really should be in the habit of doing
composer dumpautoload
because the patch changes autoloading incomposer.json
.If someone would like to come up with a benchmark for evaluation, that'd be great. :-)
Comment #69
donquixote CreditAttribution: donquixote as a volunteer commented@Mile23:
I suspect you will be faster with str_replace('\\', '/') instead of explode() + foreach().
I expect that with this solution, core classes will load faster than with the Composer loader, but slower than with a classmap.
However, all other classes will be slowed down thanks to the additional loader in the queue.
And I think we should assume that on an average site, core classes are not the majority.
I doubt that the compiled classmap is as much a memory hog as you describe it.
We can get an estimate by counting the line numbers of the classmap php file, and then comparing that with line numbers of other php files.
Maybe there are different memory pools for opcode and variables? But still I expect that we have much larger arrays than the classmap.
EDIT: But sure if someone wants to benchmark this who could complain :)
And of course there is still the APCu key/value store option, as benchmarked in #44.
@Fabianx (#44):
Just saying, there is the theoretical option of a self-updating classmap cache. This means spending more energy on writing to the cache, compared to a key/value store, until the cache is hot, but have the lookup speed of a classmap. Implementation could use a huge APCu array (instead of single values), or a filesystem IO, or the database..
Comment #70
neclimdulThe approaches here probably could use some benchmarks. I looked at this again a couple weeks ago and saw weird numbers so didn't update the patch. Haven't looked into why but we should review. Already has the `Needs profiling` tag just reinforcing that still applies.
Comment #71
Mile23#69:
It would be the same with the classmap. We're adding a classmap which is another lookup that has to occur across a giant array before we fall back to PSR-whatever.
In this case, we're asking if the class name has
Drupal\
in it, and then we're done making a decision.Well, if you look at the issue summary, you'll see that someone thinks it's an issue. :-)
Also, attached is a file called
classes.txt
. It was generated by making an autoloader that writes the name of the class to disk, like this:I cleared all the caches, added it during
index.php
, and then loaded the front page.It's 1191 lines (so 1191 autoloaded classes), and 674 of them are
Drupal\Core|\Component
namespaced, which is ~57%.The autoloader I've made here touches 675, which makes me wonder which count is off by one...
Interesting fun fact: We get a request to autoload many classes many times. If you do
sort classes.txt | uniq -c
you'll see that, for instance,Drupal\Core\Database\StatementInterface
gets autoloaded 3 times.You'll also see this, which is rather disturbing:
That probably needs a followup to filter known keywords out of the annotation system.
But that's a best case of 25 array lookups for
Drupal\Core\Annotation\ingroup
, or 25file_exists()
at worst case.Having *no* array (and no
file_exists()
) for approx 2257 class files incore/lib/Drupal
means we have more room for FAPI and whatever else.PSR-0 is our friend. :-)
Patch uses string manipulation instead of arrays.
Comment #72
Mile23Oops. Left the logging stuff in there. Hopefully the test will still pass.
Comment #73
donquixote CreditAttribution: donquixote as a volunteer commentedNo, because PHP needs to call another autoloader callback. Whereas the classmap is handled by the same autoloader method.
Differences like this do matter.
Also, the classmap lookup in Composer's class loader happens even if the classmap is empty.
Yes, but not at the expense of other classes. E.g. if we have 50:50 core vs other, then speeding one up at the expense of the other is not a win.
Adding stuff to the classmap within the composer class loader does not slow down other lookups, because it has to look into this array anyway.
(except maybe if php can do faster lookups in an empty array.. which theoretically should be the case)
Exactly. And it is going to be less with more contrib modules added.
Which is exactly what we are trying to avoid to speed things up.. otherwise we can simply use the Composer class loader..
But whatever, you should do some benchmarks and see for yourself :)
The above is just educated assumptions based on previous benchmarks and analysis I did myself.
Comment #75
xjmNote that the patch in #68 was repeatedly timing out at the 60 minute limit on testbot, so I cancelled that test.
Comment #76
donquixote CreditAttribution: donquixote as a volunteer commented@Mile23 (#71)
You have one more serious problem.
If someone calls
class_exists('Drupal\Core\NonExistentClass');
, it will crash, because you don't callfile_exists()
before including the file. The classmap solution does not have this problem, because the class would not be in the classmap.Comment #79
MixologicWhatever happened in #68 horribly destroyed both the old bots and the new drupalci ones. That patch kept zombie patching on the old bots, and one of the new bots has been stuck at running out of memory for the last four days. https://dispatcher.drupalci.org/job/default/6591/consoleFull.
Im going to hide it so the old bots stop trying it.
Comment #81
Mile23Added: #2557433: Add internal, event, and property to the list of ignored annotations in the plugin annotation system
Comment #82
mgiffordComment #84
heddnIs this still necessary with PSR-4?
Comment #85
Mile23PSR-4 still does
file_exists()
. Do we care? :-) Anyone could runcomposer install -o
as part of their deployment.Also we still have a bunch of classes set up to be autoloaded in the classmap here: http://cgit.drupalcode.org/drupal/tree/core/composer.json#n152
You can verify that the classmap ends up in
vendor/composer/autoload_classmap.php
, through the wikimedia merge plugin process.If you look at
vendor/composer/autoload_classmap.php
you'll see that the lion's share of that classmap is PHPUnit classes, because that's what PHPUnit does. This could be solved for the tarball distribution here: #2745355: Use "composer install --no-dev" to create tagged core packages and put the safety railing on here: #2780093: Have simpletest, run-tests.sh enforce their dependency on PHPUnit#2557433: Add internal, event, and property to the list of ignored annotations in the plugin annotation system is still NR. :-)
Comment #86
neclimdulThe real question at least related to the proposed solution/title is "is this still necessary now that we don't keep vendor in git?" Its more of a question of package building and documentation instead of a policy and patch.
Comment #89
Mile23So this is a really old issue.
It's originally from back in ye olden days when we didn't rely on Composer.
We could move forward here by adding
Drupal\Component
namespaces to our list of classmap optimizations inComposer::preAutoloadDump()
, if we thought it was worth it.We could also note that our Component composer.json files are broken: #2867960: Merge Component composer.json files to account for them during build
Or we could just say closed (outdated).
Comment #90
fgmSeems even more relevant now that Composer has become mainstream with current workflows than it was back when it was created, so it's hardly outdated, IMHO.
Comment #91
Wim LeersThis sounds sensible.
Comment #92
Mile23Which classes should we classmap, then?
Comment #93
Crell CreditAttribution: Crell commentedData point: For Platform.sh, we've been building all Composer-based sites with --optimize-autoloader for a year or two now. We've never run into an issue with that, and I've never seen a bug report that was in any way related to it.
So concerns about "optimize all the things" seem to not be coming to pass, in our experience.
Comment #95
andypostLooks this issue needs another round of work
1) Symfony deprecated
ApcClassLoader
https://api.symfony.com/3.4/Symfony/Component/ClassLoader/ApcClassLoader...2) Composer has own apcu based dumper https://getcomposer.org/doc/articles/autoloader-optimization.md#optimiza...
PS: on one of client sites I see 8k calls to
apcu_fetch()
Comment #96
Fabianx CreditAttribution: Fabianx as a volunteer commentedSo that means 1.5k classes. I think that sounds feasible to dump.
With all the composer changes around building D8, I am all for just optimizing the class map at build time -- also now taht we have php-dev + using APCu class loader from composer as fallback.
Also with PHP 7 memory usage of arrays has dramatically gone down ...
+1 from me to just implement that.
Comment #97
Fabianx CreditAttribution: Fabianx as a volunteer commentedRegarding the modules installed discussion:
- The class map is just a _cache_, the same as APCu
If the modules installed discussion is a problem, we should just roll our own, decorate composers and check PSR-4 / PSR-0 namespace exists before loading it from either class map or APCu or file system.
Comment #98
BerdirNot sure if everyone has seen that but somewhat related: https://wiki.php.net/rfc/preload + https://github.com/composer/composer/issues/7777, so we could preload all the classes that are required on a majority of the requests to always be around and skip quite a bit of IO completely, with the downside of having to restart the server but on many managed environments like platform.sh, that's the case anyway.
Comment #99
andypostMy exp still tells that we should use #2704571: Add an APCu classloader with a single entry
The main reason is to prevent apcu call for each class (composer's class loader also query each class with single entry)
Comment #100
Mile23Just a heads-up that the plan from the Composer initiative is that drupal/drupal will be a core-dev-only package, and that drupal/drupal-project-legacy and others will be the way people generate their codebase. #2982680: Add composer-ready project templates to Drupal core
So whatever decision is made here for general-purpose optimization might need to be replicated in those other templates.
Then, individual projects could amend their autoloading config as needed, including
composer install --optimize-autoloader
or--apcu-autoloader
in their deploy script.It seems like one stopgap for the autoload-modules problem would be to add PSR-4 entries in
core/composer.json
for all the core modules. Thencomposer install -o
at least generates the classmap for all the classes.Then, in an ideal world, contrib would include their PSR-4 in their composer.json and we'd have the whole thing.
Comment #101
catchfwiw #2704571: Add an APCu classloader with a single entry is still my preference here as well. That could then function as a source for (optional) class preloading in PHP 7.4, since it would be based on real-life site usage. There are tonnes of classes in core/lib which hardly ever get used and some classes from modules will be used constantly.
Comment #102
BerdirA classloader with a single apcu cache entry might be faster once it is fully populated, but I think that getting to that point is going to be quite painful and slow. It's similar to our cache collector pattern, which uses a pretty complex lock pattern to prevent unclean caches and in this case, there would be hundreds of possibilities for clashing read/write cases.
I've been testing things a bit recently and composer has apcu support too, you just have to opt-in there as well, so using optimize for all non-module classes and apcu for module-provided classes worked quite well for me.
Comment #103
andypostOn my side it removed 8K calls for each class to apcu_fetch
Comment #104
BerdirDid you test it in comparison with the composer classloader with apcu support? (composer install -o --apcu-autoloader)
I would expect the that the difference is much smaller then.
Comment #105
andypost@Berdir I did not after checking https://github.com/composer/composer/blob/master/src/Composer/Autoload/C...
Comment #106
BerdirI know that it's separate cache entries, my point is that in combination with the optimized classloader, it's *only* used as a fallback for module-provided classes, anything in vendor/ and core/lib will be fetched from that.
Comment #107
andypostAh, yep! Makes sense
We still need a drupal classloader as bridge
Comment #108
Gábor HojtsyReparenting to the Drupal 9 issue tree, since this may need to be done as part of moving to Symfony 4 as per #3020296: Remove Symfony's classloader as it does not exist in Symfony 4.
Comment #111
bradjones1Can I ask, what is the impact here regarding tarball installations? I do not personally have any sites installed only from tarball, but it seems there's some distribution-management concerns here?
AFAIK we do not currently ship any composer artifacts in the tgz download? Or am I missing something very obvious?Answered my own question here... of course we do, I just never use the tarball...I know there's been a lot of work on the composer integration since this was last active, so perhaps that also changes the perspective a bit.
Comment #112
Mile23Re: #111:
The impact on a user of the tarball as of 8.8.0+ is that you can say
composer dumpauto -o
or--apcu
as part of your deploy process, and it will work.Followup on #100:
As of Drupal 8.8.0, the tarball is constructed by running
composer create-project drupal/legacy-project
.You can see the 8.8.0 drupal/legacy-project composer.json file here: https://git.drupalcode.org/project/drupal/blob/8.8.0/composer/Template/L...
It does not use any special autoloader directives, though the generating script might add -o or somesuch. That's doubtful however. @mixologic would be the source of truth about that.
It's also true that as of 8.8.0, the drupal/drupal composer.json file (the one at the root of the repo) is considered a development setup. So if we change the root composer.json file, that will only affect the autoloader generated for core developers doing core development, and not users of the product.
I'm tempted to close this issue as outdated, but I'll leave that to a maintainer.
Comment #113
alexpottI think that the resolution on #3020296: Remove Symfony's classloader as it does not exist in Symfony 4 means that this is now outdated. With Drupal 9 it you want an optimised classlaoder in production you can do
composer dump-autoload --optimize
and away you go. No other changes are necessary.As to whether we should recommend people do that. Well that feels like a different question. We're still enabling the APCu optimisation automatically and on DrupalCI there doesn't appear to much benefit from doing this - see #3117410: Use an optimised autoloader in testing so we need more perf testing and work to understand what we should recommend.