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

deviantintegral’s picture

StatusFileSize
new2.61 KB
deviantintegral’s picture

Status: Active » Needs review
dcam’s picture

Issue tags: +Performance
bleen’s picture

Do 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...

deviantintegral’s picture

According 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.

e0ipso’s picture

+++ b/includes/bootstrap.inc
@@ -835, +835, @@ function drupal_get_filename($type, $name, $filename = NULL) {
+  $use_cache = (!defined('MAINTENANCE_MODE') || MAINTENANCE_MODE != 'update') && variable_get('cache_drupal_get_filename', FALSE);

Maybe

variable_get('cache_' . __FUNCTION__, FALSE);

for consistency?

+++ b/includes/bootstrap.inc
@@ -837,18 +837,35 @@ function drupal_get_filename($type, $name, $filename = NULL) {
+  if ($use_cache) {
+    if (empty($files) && $cached = cache_get(__FUNCTION__)) {

Can we condense these two into a single if?

e0ipso’s picture

A 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.

deviantintegral’s picture

StatusFileSize
new3.55 KB

Updated from the review in #6, along with documenting the variable in default.settings.php.

deviantintegral’s picture

Version: 7.x-dev » 8.0.x-dev
StatusFileSize
new7.97 KB

Here'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.

anavarre’s picture

Issue tags: +D8 cacheability
mikeytown2’s picture

class DrupalCacheArray might make the cache writes simpler. Don't know the equivalent in D8.

Status: Needs review » Needs work

The last submitted patch, 9: 2442383.9-get-filename-cache-8.x.patch, failed testing.

deviantintegral’s picture

Version: 8.0.x-dev » 8.1.x-dev
Status: Needs work » Needs review
StatusFileSize
new7.81 KB

Reroll against 8.1.x.

Status: Needs review » Needs work

The last submitted patch, 14: 2442383.14-get-filename-cache-8.x.patch, failed testing.

deviantintegral’s picture

This needs to move the config settings into setup / teardown methods so they don't persist across test cases.

deviantintegral’s picture

Status: Needs work » Needs review
StatusFileSize
new8.15 KB

Status: Needs review » Needs work

The last submitted patch, 17: 2442383.17-get-filename-cache-8.x.patch, failed testing.

deviantintegral’s picture

Status: Needs work » Needs review
StatusFileSize
new8.25 KB

Turns 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?

david_garcia’s picture

Status: Needs review » Needs work

This 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.

wim leers’s picture

I'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.

joelpittet’s picture

Status: Needs work » Postponed

I 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?

David_Rothstein’s picture

So 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?

wim leers’s picture

#2351919: Replace uses of drupal_get_path() with __DIR__ where possible landed in D8, unpostponing.

Can anyone explain what's going on here exactly?

+1

David_Rothstein’s picture

Status: Postponed » Needs work

Actually unpostponing :) But maybe "needs more info" would be a better status.

wim leers’s picture

Status: Needs work » Postponed (maintainer needs more info)

Haha, oops. :P

And, good point!

almaudoh’s picture

The plan is to replace drupal_get_(path|filename) with the new ExtensionList methods ::getPath and ::getFilename #2208429: Extension System, Part III: ExtensionList, ModuleExtensionList and ProfileExtensionList. The performance improvements can be made over there (code needs profiling).

deviantintegral’s picture

Status: Postponed (maintainer needs more info) » Needs work

I 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:

  1. Refactor the system_list() prepopulation of the static cache. I had no idea that was happening, and it's not clear looking through drupal_get_filename().
  2. See if there's a better way to handle when files don't exist in drupal_get_filename(). Perhaps we use a cache, but only for files that don't exist? Having to do a cache clear in that case seems OK. Or, throw an exception if a file doesn't exist?

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

David_Rothstein’s picture

I 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.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

quietone’s picture

Component: base system » file system

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.