drupal_load() already has a static to prevent it loading a module more than once, and there's no reason not to use it, so we should use include() there instead of include_once().
Here's how it looks in strace, I included the first request immediately after starting apache to strace it (i.e. with a cold start on PHP-level caches), and the third request (where PHP caches are warmed). As you can see the cached version is considerably cleaner. Also note that with 150 modules installed, the strace output will be 50 times as long as it is here, this is just a cached page.
HEAD:
(first request after restart)
8125 lstat("/home/catch/www/7/modules/dblog/dblog.module", {st_mode=S_IFREG|0644, st_size=6768, ...}) = 0
8125 lstat("/home/catch/www/7/modules/dblog", {st_mode=S_IFDIR|0755, st_size=4096, ...}) = 0
8125 lstat("/home/catch/www/7/modules", {st_mode=S_IFDIR|0755, st_size=4096, ...}) = 0
8125 open("/home/catch/www/7/modules/dblog/dblog.module", O_RDONLY) = 11
8125 fstat(11, {st_mode=S_IFREG|0644, st_size=6768, ...}) = 0
8125 fstat(11, {st_mode=S_IFREG|0644, st_size=6768, ...}) = 0
8125 fstat(11, {st_mode=S_IFREG|0644, st_size=6768, ...}) = 0
8125 fstat(11, {st_mode=S_IFREG|0644, st_size=6768, ...}) = 0
8125 mmap(NULL, 6768, PROT_READ, MAP_SHARED, 11, 0) = 0x7fb438363000
8125 stat("/home/catch/www/7/modules/dblog/dblog.module", {st_mode=S_IFREG|0644, st_size=6768, ...}) = 0
8125 munmap(0x7fb438363000, 6768) = 0
8125 close(11) = 0
8125 lstat("/home/catch/www/7/modules/overlay/overlay.module", {st_mode=S_IFREG|0644, st_size=33497, ...}) = 0
8125 lstat("/home/catch/www/7/modules/overlay", {st_mode=S_IFDIR|0755, st_size=4096, ...}) = 0
8125 open("/home/catch/www/7/modules/overlay/overlay.module", O_RDONLY) = 11
8125 fstat(11, {st_mode=S_IFREG|0644, st_size=33497, ...}) = 0
8125 fstat(11, {st_mode=S_IFREG|0644, st_size=33497, ...}) = 0
8125 fstat(11, {st_mode=S_IFREG|0644, st_size=33497, ...}) = 0
8125 fstat(11, {st_mode=S_IFREG|0644, st_size=33497, ...}) = 0
8125 mmap(NULL, 33497, PROT_READ, MAP_SHARED, 11, 0) = 0x7fb43835c000
8125 stat("/home/catch/www/7/modules/overlay/overlay.module", {st_mode=S_IFREG|0644, st_size=33497, ...}) = 0
8125 brk(0x7fb4394a2000) = 0x7fb4394a2000
8125 munmap(0x7fb43835c000, 33497) = 0
8125 close(11) = 0
8125 lstat("/home/catch/www/7/modules/system/system.module", {st_mode=S_IFREG|0644, st_size=138592, ...}) = 0
8125 lstat("/home/catch/www/7/modules/system", {st_mode=S_IFDIR|0755, st_size=4096, ...}) = 0
8125 open("/home/catch/www/7/modules/system/system.module", O_RDONLY) = 11
8125 fstat(11, {st_mode=S_IFREG|0644, st_size=138592, ...}) = 0
8125 fstat(11, {st_mode=S_IFREG|0644, st_size=138592, ...}) = 0
8125 fstat(11, {st_mode=S_IFREG|0644, st_size=138592, ...}) = 0
8125 fstat(11, {st_mode=S_IFREG|0644, st_size=138592, ...}) = 0
8125 mmap(NULL, 138592, PROT_READ, MAP_SHARED, 11, 0) = 0x7fb438312000
8125 stat("/home/catch/www/7/modules/system/system.module", {st_mode=S_IFREG|0644, st_size=138592, ...}) = 0
8125 brk(0x7fb4394e5000) = 0x7fb4394e5000
8125 brk(0x7fb439525000) = 0x7fb439525000
8125 brk(0x7fb439568000) = 0x7fb439568000
8125 brk(0x7fb4395ab000) = 0x7fb4395ab000
8125 munmap(0x7fb438312000, 138592) = 0
8125 close(11) = 0
(third request)
7904 lstat("/home/catch/www/7/modules/dblog/dblog.module", {st_mode=S_IFREG|0644, st_size=6768, ...}) = 0
7904 lstat("/home/catch/www/7/modules/dblog", {st_mode=S_IFDIR|0755, st_size=4096, ...}) = 0
7904 lstat("/home/catch/www/7/modules", {st_mode=S_IFDIR|0755, st_size=4096, ...}) = 0
7904 open("/home/catch/www/7/modules/dblog/dblog.module", O_RDONLY) = 11
7904 fstat(11, {st_mode=S_IFREG|0644, st_size=6768, ...}) = 0
7904 fstat(11, {st_mode=S_IFREG|0644, st_size=6768, ...}) = 0
7904 fstat(11, {st_mode=S_IFREG|0644, st_size=6768, ...}) = 0
7904 fstat(11, {st_mode=S_IFREG|0644, st_size=6768, ...}) = 0
7904 mmap(NULL, 6768, PROT_READ, MAP_SHARED, 11, 0) = 0x7f954fcdf000
7904 stat("/home/catch/www/7/modules/dblog/dblog.module", {st_mode=S_IFREG|0644, st_size=6768, ...}) = 0
7904 munmap(0x7f954fcdf000, 6768) = 0
7904 close(11) = 0
7904 lstat("/home/catch/www/7/modules/overlay/overlay.module", {st_mode=S_IFREG|0644, st_size=33497, ...}) = 0
7904 lstat("/home/catch/www/7/modules/overlay", {st_mode=S_IFDIR|0755, st_size=4096, ...}) = 0
7904 open("/home/catch/www/7/modules/overlay/overlay.module", O_RDONLY) = 11
7904 fstat(11, {st_mode=S_IFREG|0644, st_size=33497, ...}) = 0
7904 fstat(11, {st_mode=S_IFREG|0644, st_size=33497, ...}) = 0
7904 fstat(11, {st_mode=S_IFREG|0644, st_size=33497, ...}) = 0
7904 fstat(11, {st_mode=S_IFREG|0644, st_size=33497, ...}) = 0
7904 mmap(NULL, 33497, PROT_READ, MAP_SHARED, 11, 0) = 0x7f954fcd8000
7904 stat("/home/catch/www/7/modules/overlay/overlay.module", {st_mode=S_IFREG|0644, st_size=33497, ...}) = 0
7904 munmap(0x7f954fcd8000, 33497) = 0
7904 close(11) = 0
7904 lstat("/home/catch/www/7/modules/system/system.module", {st_mode=S_IFREG|0644, st_size=138592, ...}) = 0
7904 lstat("/home/catch/www/7/modules/system", {st_mode=S_IFDIR|0755, st_size=4096, ...}) = 0
7904 open("/home/catch/www/7/modules/system/system.module", O_RDONLY) = 11
7904 fstat(11, {st_mode=S_IFREG|0644, st_size=138592, ...}) = 0
7904 fstat(11, {st_mode=S_IFREG|0644, st_size=138592, ...}) = 0
7904 fstat(11, {st_mode=S_IFREG|0644, st_size=138592, ...}) = 0
7904 fstat(11, {st_mode=S_IFREG|0644, st_size=138592, ...}) = 0
7904 mmap(NULL, 138592, PROT_READ, MAP_SHARED, 11, 0) = 0x7f954fc8e000
Patch:
(first request after restart)
8020 stat("/home/catch/www/7/modules/dblog/dblog.module", {st_mode=S_IFREG|0644, st_size=6768, ...}) = 0
8020 lstat("/home/catch/www/7/modules/dblog/dblog.module", {st_mode=S_IFREG|0644, st_size=6768, ...}) = 0
8020 lstat("/home/catch/www/7/modules/dblog", {st_mode=S_IFDIR|0755, st_size=4096, ...}) = 0
8020 lstat("/home/catch/www/7/modules", {st_mode=S_IFDIR|0755, st_size=4096, ...}) = 0
8020 open("/home/catch/www/7/modules/dblog/dblog.module", O_RDONLY) = 11
8020 fstat(11, {st_mode=S_IFREG|0644, st_size=6768, ...}) = 0
8020 fstat(11, {st_mode=S_IFREG|0644, st_size=6768, ...}) = 0
8020 fstat(11, {st_mode=S_IFREG|0644, st_size=6768, ...}) = 0
8020 fstat(11, {st_mode=S_IFREG|0644, st_size=6768, ...}) = 0
8020 mmap(NULL, 6768, PROT_READ, MAP_SHARED, 11, 0) = 0x7fdc6ff67000
8020 munmap(0x7fdc6ff67000, 6768) = 0
8020 close(11) = 0
8020 stat("/home/catch/www/7/modules/overlay/overlay.module", {st_mode=S_IFREG|0644, st_size=33497, ...}) = 0
8020 lstat("/home/catch/www/7/modules/overlay/overlay.module", {st_mode=S_IFREG|0644, st_size=33497, ...}) = 0
8020 lstat("/home/catch/www/7/modules/overlay", {st_mode=S_IFDIR|0755, st_size=4096, ...}) = 0
8020 open("/home/catch/www/7/modules/overlay/overlay.module", O_RDONLY) = 11
8020 fstat(11, {st_mode=S_IFREG|0644, st_size=33497, ...}) = 0
8020 fstat(11, {st_mode=S_IFREG|0644, st_size=33497, ...}) = 0
8020 fstat(11, {st_mode=S_IFREG|0644, st_size=33497, ...}) = 0
8020 fstat(11, {st_mode=S_IFREG|0644, st_size=33497, ...}) = 0
8020 mmap(NULL, 33497, PROT_READ, MAP_SHARED, 11, 0) = 0x7fdc6ff60000
8020 brk(0x7fdc71997000) = 0x7fdc71997000
8020 munmap(0x7fdc6ff60000, 33497) = 0
8020 close(11) = 0
8020 stat("/home/catch/www/7/modules/system/system.module", {st_mode=S_IFREG|0644, st_size=138592, ...}) = 0
8020 lstat("/home/catch/www/7/modules/system/system.module", {st_mode=S_IFREG|0644, st_size=138592, ...}) = 0
8020 lstat("/home/catch/www/7/modules/system", {st_mode=S_IFDIR|0755, st_size=4096, ...}) = 0
8020 open("/home/catch/www/7/modules/system/system.module", O_RDONLY) = 11
8020 fstat(11, {st_mode=S_IFREG|0644, st_size=138592, ...}) = 0
8020 fstat(11, {st_mode=S_IFREG|0644, st_size=138592, ...}) = 0
8020 fstat(11, {st_mode=S_IFREG|0644, st_size=138592, ...}) = 0
8020 fstat(11, {st_mode=S_IFREG|0644, st_size=138592, ...}) = 0
8020 mmap(NULL, 138592, PROT_READ, MAP_SHARED, 11, 0) = 0x7fdc6ff16000
8020 brk(0x7fdc719da000) = 0x7fdc719da000
8020 brk(0x7fdc71a1a000) = 0x7fdc71a1a000
8020 brk(0x7fdc71a5d000) = 0x7fdc71a5d000
8020 brk(0x7fdc71aa0000) = 0x7fdc71aa0000
8020 munmap(0x7fdc6ff16000, 138592) = 0
8020 close(11) = 0
(third request)
7732 stat("/home/catch/www/7/modules/dblog/dblog.module", {st_mode=S_IFREG|0644, st_size=6768, ...}) = 0
7732 stat("/home/catch/www/7/modules/overlay/overlay.module", {st_mode=S_IFREG|0644, st_size=33497, ...}) = 0
7732 stat("/home/catch/www/7/modules/system/system.module", {st_mode=S_IFREG|0644, st_size=138592, ...}) = 0
There is no real change in benchmarks, however http://tag1consulting.com/blog/nfs-drupal-and-realpath-cache - NFS doesn't cache lstat calls at all, also the faster page caching is (like using APC or memcache as a backend, and with other patches in the queue applied) the more chance we have of seeing an improvement in benchmarks with and without this patch, but I don't think that's necessary since this makes sense by itself.
| Comment | File | Size | Author |
|---|---|---|---|
| #27 | 1068932_include-d8-27.patch | 1.13 KB | mgifford |
| #19 | 1068932_include_once.patch | 1.8 KB | catch |
| #16 | 1068932_include_D7_do_not_test.patch | 1.28 KB | catch |
| #14 | 1068932_include_D7_do_not_test.patch | 427 bytes | catch |
| #14 | 1068932_include.patch | 1.13 KB | catch |
Comments
Comment #2
catchInstalling via the UI is fine, drush doesn't like it though due to this, and a hardcoded require of the install profile.
Attached patch covers the install profile issue, not sure whether this will be enough or not for the test bot.
Patch for drush at #1068964: Remove hardcoded require_once() for system.module.
We will need to move this to D8 since this is going to require a new drush release, although I think we should be able to backport it so adding that tag. Also leaving at D7 so now so the patch gets tested.
Comment #3
catchComment #5
catchFixed those failures.
Comment #6
moshe weitzman commentedPeople do all kinds of crazy things like include .module files from settings.php. They can safely do that because of the include_once. Switching to include() is going to preclude them from doing that. Even more crazy are apps that custom bootstrap drupal like drush. drush can cope with t
I'm not saying that we this patch is bad, but it will cause some pain. I don't think the justification is strong enough yet.
Comment #7
catchsettings.php is loaded from bootstrap.inc, so they ought to be able to use drupal_load() from there.
Comment #8
fabianx commentedSubscribe.
Comment #9
mgifford#5: drupal_load.patch queued for re-testing.
Comment #11
hefox commentedQuick reroll, haven't tested that it works/if any syntax errors
Comment #13
yesct commented(the slash in the i/o tag breaks the autocomplete from adding new tags)
Comment #14
catchReviving this, should have some data from a 7.x site soonish.
Comment #16
catch7.x patch that won't break drush.
Comment #17
catchJust tested this on a site that has the Drupal install on NFS, it reduced time for a full bootstrap from between 500-1200ms to 2-400ms.
Comment #18
mgiffordShould #16 also run on SimplyTest.me? It fails....
Comment #19
catchMissed a bit. Moving to 7.x to trigger a test.
Comment #21
toamit commentedIt would nice to get this patch in drupal core, I am seeing significantly variable NFS performance even with APC holding all the files.
Comment #24
yesct commentedI didn't see a d8 commit so I think this still needs a d8 fix.
Comment #27
mgiffordReroll of #14
Comment #29
mgiffordI suspect this will get the following error but perhaps this is just what SimplyTest.me is giving me because of some server work:
Fatal error: Cannot redeclare seven_preprocess_html() (previously declared in /home/s1006273f91c79d7/www/core/themes/seven/seven.theme:15) in /home/s1006273f91c79d7/www/core/themes/seven/seven.theme on line 25
Comment #43
catchInclude files are nearly gone.