drupal_get_path() has quite a few IO calls that aren't needed on sites after they have been deployed. Even if the static cache is hit, there's a few calls to file_exists() that get called anyways. For sites that are deployed with version control or something similar, we know that code isn't changing under us without an update and cache clear being run.
The attached patch adds optional caching support to drupal_get_filename(). I've kept it off by default as it could be a major DXWTF for local environments, and quite confusing for anyone who might be manually creating code files on servers. On one large code base, I'm getting a 20% performance improvement on an uncached anonymous page after the drupal_get_path() cache is warmed.
Here's ab results against a real site implementation. This is inside a vagrant VM, but the code is sync'ed with rsync so IO is against the local disk image and not over the network.
This is ApacheBench, Version 2.3 <$Revision: 1554214 $>
Copyright 1996 Adam Twiss, Zeus Technology Ltd, http://www.zeustech.net/
Licensed to The Apache Software Foundation, http://www.apache.org/
Benchmarking local.sitename.com (be patient).....done
Server Software: Apache/2.4.7
Server Hostname: local.sitename.com
Server Port: 80
Document Path: /an-awesome-url
Document Length: 90716 bytes
Concurrency Level: 1
Time taken for tests: 256.821 seconds
Complete requests: 100
Failed requests: 0
Total transferred: 9127100 bytes
HTML transferred: 9071600 bytes
Requests per second: 0.39 [#/sec] (mean)
Time per request: 2568.206 [ms] (mean)
Time per request: 2568.206 [ms] (mean, across all concurrent requests)
Transfer rate: 34.71 [Kbytes/sec] received
Connection Times (ms)
min mean[+/-sd] median max
Connect: 0 0 0.1 0 0
Processing: 2450 2568 259.4 2540 5095
Waiting: 2385 2493 257.5 2466 5007
Total: 2450 2568 259.4 2540 5095
Percentage of the requests served within a certain time (ms)
50% 2540
66% 2555
75% 2567
80% 2576
90% 2609
95% 2647
98% 2701
99% 5095
100% 5095 (longest request)
After this patch, and setting $conf['cache_drupal_get_filename'] = TRUE;.
This is ApacheBench, Version 2.3 <$Revision: 1554214 $>
Copyright 1996 Adam Twiss, Zeus Technology Ltd, http://www.zeustech.net/
Licensed to The Apache Software Foundation, http://www.apache.org/
Benchmarking local.sitename.com (be patient).....done
Server Software: Apache/2.4.7
Server Hostname: local.sitename.com
Server Port: 80
Document Path: /an-awesome-url
Document Length: 90716 bytes
Concurrency Level: 1
Time taken for tests: 202.005 seconds
Complete requests: 100
Failed requests: 0
Total transferred: 9127100 bytes
HTML transferred: 9071600 bytes
Requests per second: 0.50 [#/sec] (mean)
Time per request: 2020.047 [ms] (mean)
Time per request: 2020.047 [ms] (mean, across all concurrent requests)
Transfer rate: 44.12 [Kbytes/sec] received
Connection Times (ms)
min mean[+/-sd] median max
Connect: 0 1 11.7 0 117
Processing: 1948 2019 86.4 2009 2819
Waiting: 1877 1945 85.6 1936 2744
Total: 1948 2020 86.9 2009 2819
Percentage of the requests served within a certain time (ms)
50% 2009
66% 2023
75% 2031
80% 2036
90% 2054
95% 2074
98% 2120
99% 2819
100% 2819 (longest request)
I haven't looked at D8 yet to see if this is needed, but if this passes 7.x tests to make sure the idea isn't horribly broken I'll forward port it.
Comments
Comment #1
deviantintegral commentedComment #2
deviantintegral commentedComment #3
dcam commentedComment #4
bleen commentedDo we need to consider the case where someone is using drupal_get_filename for files inside /files? In that case things would always be changing...
Comment #5
deviantintegral commentedAccording to the docs, 'files' isn't a valid parameter for drupal_get_filename. This is adding a cache on top of the existing static cache, so if someone is using files it would likely break mid-request for new or moved files.
Comment #6
e0ipsoMaybe
for consistency?
Can we condense these two into a single if?
Comment #7
e0ipsoA part from two minor suggestions, this code looks good to me. Completely transparent if the cache variable is off. If the variable is present then the added code kicks in cleanly.
Comment #8
deviantintegral commentedUpdated from the review in #6, along with documenting the variable in default.settings.php.
Comment #9
deviantintegral commentedHere's the Drupal 8 version, along with tests. If this passes, the 7.x patch will need to be updated to use drupal_static and the test will have to be backported.
Comment #10
anavarreComment #11
mikeytown2 commentedclass DrupalCacheArray might make the cache writes simpler. Don't know the equivalent in D8.
Comment #14
deviantintegral commentedReroll against 8.1.x.
Comment #16
deviantintegral commentedThis needs to move the config settings into setup / teardown methods so they don't persist across test cases.
Comment #17
deviantintegral commentedComment #19
deviantintegral commentedTurns out that using drupal_static() was breaking code expecting the filename cache to stay consistent. Let's see if this passes, but I'm not really happy with this so any other ideas?
Comment #20
david_garcia commentedThis thing should have it's own cache binary so that it can be routed to somewhere that makes sense. Indeed, this caching only makes sense for APCu/Wincache backends right? Or the performance gains you are seeing are with the database backend?
If afirmative, maybe the Drupal's File Cache class might be better suited to hold this data. Another benefit of using the File Cache class would be that there would be no need to rely on static storage, because APCu/Wincache are blazing fast.
Or consider what @mickeytown said in #11.
Comment #21
wim leersI'm not convinced this is a good idea.
It'd be better to remove all those
drupal_get_(filename|path)()calls, then they can't make things slow in the first place. See #2351919: Replace uses of drupal_get_path() with __DIR__ where possible. This should only be considered after that issue is done and a new round of profiling proves that this is still a problem IMO.Comment #22
joelpittetI agree with @Wim Leers, we should postpone on #2351919: Replace uses of drupal_get_path() with __DIR__ where possible. Feel free to disagree and unpostpone if there is good reason to do so.
Maybe send it back to D7 for more profiling in the real world like the issue summary tests?
Comment #23
David_Rothstein commentedSo I'm confused how another cache here could lead to a significant performance improvement in either Drupal 7 or 8.
In both versions (and I'm more confident about this statement for Drupal 7 but I think it's true for Drupal 8 too) there is code in system_list() which is supposed to prime the existing drupal_get_filename() static cache for all enabled modules. That gets called on every page request when enabled modules are loaded. So on a normal page request, drupal_get_filename() should essentially always return cached results already, whenever it's called (unless you are doing something crazy like loading a file from a disabled module).
Can anyone explain what's going on here exactly?
Comment #24
wim leers#2351919: Replace uses of drupal_get_path() with __DIR__ where possible landed in D8, unpostponing.
+1
Comment #25
David_Rothstein commentedActually unpostponing :) But maybe "needs more info" would be a better status.
Comment #26
wim leersHaha, oops. :P
And, good point!
Comment #27
almaudoh commentedThe plan is to replace
drupal_get_(path|filename)with the newExtensionListmethods::getPathand::getFilename#2208429: Extension System, Part III: ExtensionList, ModuleExtensionList and ProfileExtensionList. The performance improvements can be made over there (code needs profiling).Comment #28
deviantintegral commentedI figured out what was happening - the site I built this on had a typo in a call to drupal_get_filename() for a module that didn't exist. Fixing that gave me the same performance as using this patch. So it's an edge case, but one that kind of sucks if you have a ton of modules to scan through.
I think it's reasonable to postpone this on [#2531919] - but I see two potential followups:
Comment #30
David_Rothstein commentedI can't believe I didn't think to link to this before, but reading the specific cause in #28 reminded me. See #1081266: Avoid re-scanning module directory when a filename or a module is missing. That adds the cache for files that don't exist.
Comment #45
quietone commented