Add static cache to module_load_include()

#1: drupal-1443308-1-add-static-cache-to-module_load_include.patch queued for re-testing.

No testing results for a month??

So, there are plans to apply it to next release or you need more testers?

Can you explain what this patch does, I don't get it. Thx.

In general, the share is growing of hosting environments where the VPS/user VMs are on relatively light and inexpensive servers and files are on SAN/NAS/dedicated *raid storage machines, so this patch still makes sense, right?

p.s. I am just curious, because found link to this patch in many discussions, where drupal is running slow because of quantity of files and similar stuff.

Well, if we can prove that this improves performance on some environments without hurting it (noticably) on others, that would be a strong case to get this in. Especially if those environments are shared hosters, which Drupal generally does not perform well for.

I'm pretty sure this applies to Drupal 8 as well, and hence should go there first, but could be backported all the way to Drupal 6. Adding tags accordingly.

Updated issue summary.

9: drupal-1443308-9-module_load_include-static-cache.patch queued for re-testing.

Re D7 #9 Commerce Views listing page on a site that has ~375 enabled modules.

I love this module it saves > 1sec load time on cold caches after registry rebuild.

+++ b/includes/module.inc
@@ -320,17 +320,24 @@ function module_load_install($module) {
+  $key = $type . ':' . $module . ':' . $name;

Do we need to deal with $name = NULL? xmlsitemap module has an example of this use case.

Here's the patch for D8.

@mikeytown2 I know that much, I guess my question was too generic there, sorry.

I was just wondering if there would be any cache collisions with 'type:module:'?

I think so too, thought I'd ask. Let's see if we can get this into D8/7/6:)

I would suggest using module if name is empty (from docblock: If omitted, $module is used). This way we hide the implementation detail AND increase the hit rate if we have code that explicits name=module and name=NULL.

+++ b/core/includes/module.inc
@@ -137,17 +137,24 @@ function module_load_install($module) {
+  static $file_name = array();
   if (!isset($name)) {
     $name = $module;
   }
+  $key = $type . ':' . $module . ':' . $name;

@penyaskito we have a check for !isset($name), I'd rather not change functionality to empty() as it could be disruptive, no?

normal feature request is a red flag.
Needs a beta evaluation. instructions are in the summary.

@joelpittet Oops, I miss-looked at it, the key never will look like 'type:module:', so IMHO there is nothing wrong on the last patch.

Thanks, YesCT, this is mis-categorized.

Uhm, that is a task, not a feature request.

Depending if you can show that it saves much, it can even be major.

It would be helpful if somebody could re-run the benchmarks listed in the issue summary on current D8.

With this change, would it also make sense to use require instead of require_once?

Correcting miss-spelled tag.

reroll of #9 against latest

Tested before and after cache clear on a pretty complicated site (154 modules, decent caching mechanisms in place, php / mysql 5.5 / opcache). This is running on a Local CentOS 6.5 Vagrant instance.

Prior to patch

4103

After patch

2394

switching to 7.x temporarily to allow test bot to pick it up correctly

back to 8 since that's where thread was but the patch I rerolled for #37 passed tests

Uhm, RTBC? Looks pretty straightforward to me.

#35 is still unanswered. Could we use require instead of require_once? What is the gain?

performance difference between the two should be nominal on anything running opcache (5ms otherwise maybe, pushing it to suggest that high even)

Just looking through this patch and any reason why it does

if (!empty($file_name[$key])) {
   return $file_name[$key];
}

Instead of

if (isset($file_name[$key])) {
  return $file_name[$key];
}

As currently this will attempt to load multi times when a file is missing.

Back to RTBC - Should profile this again with DB in IS.

This has to be for the D7 version.

This looks interesting, but still needs to go into Drupal 8 first.

Just for fun, it looks like Drupal 8 has two functions that do a similar thing, module_load_include() itself plus ModuleHandler::loadInclude(). Perhaps the static cache would be useful on both.

Ok, too easy.

:P

I have no idea if this will pass or not, I didn't test it but looks simple enough.

ok, start simple this time.

Ok, patch 60 passed, now try this one just for fun.
*EDIT* I cancelled tests for this patch and then wrote a new patch #64

I'd do an interdiff, but it's not that big.

basically patch 62 and 56 missing key value on loadInclude, putting that in, should get better test results

Don't think we can test this ...

Edit: The patch is RTBC. I don't think there is a way to test an internal static, therefore I am marking as RTBC and also rc target triage so it is considered for the RC phase.

@Fabianx, I am confused by your comment #65

1. +++ b/core/includes/module.inc
   @@ -126,17 +126,24 @@ function module_load_install($module) {
    function module_load_include($type, $module, $name = NULL) {
   +  static $file_name = [];
      if (!isset($name)) {
   

   Wouldn't it make more sense to call this $file_names?

2. +++ b/core/includes/module.inc
   @@ -126,17 +126,24 @@ function module_load_install($module) {
   +  $key = $type . ':' . $module . ':' . $name;
   +  if (!isset($file_name)) {
   +    return $file_name[$key];
   +  }
   

   I don't understand how this works. If the static cache is not set, we return the file name without checking if it exists. What would be returned in this case?

Changes:

* Use $files as the static variable name instead of $file_name
* Fixes #67.2 - the logic implemented in module_load_include() is wrong
* Reorder the code to use only a return statement

@jonhattan Nice!

+++ b/core/includes/module.inc
@@ -126,18 +126,25 @@ function module_load_install($module) {
+    $files[$key] = FALSE;
+    if (function_exists('drupal_get_path')) {

This is wrong, we should not say a file does not exist, when the drupal_get_path function is not (yet) in scope. We should ignore the static cache in that case.

I think adding it in an else {} block should be fine though as it is unlikely that drupal_get_path result changes within the same request.

However the safer version is to only populate the cache when an item is found.

Is there a reason we are not using drupal_static()?

#72: Yes, IS states so:

files can only be added not removed during a run, so there is no need to ever clear the static - as the function calls require_once.

ModuleHandler can use a class property, doesn't need to be an actual static.

This is the recommendation in #75. #71 still needs doing.

Thanks for tagging this for rc target triage! If you want committers to consider it for inclusion in RC, add a statement to the issue summary of why we need to make this change during RC, including what happens if we do not make the change and what any disruptions from it are. You can add a section <h3>Why this should be an RC target</h3> to the summary.

@Fabianx Re #71: How do we resolve the path the file reliably without drupal_get_path()?

#78: What I mean is:

We do _never ever_ want to cache the result when drupal_get_path function is not available as that will always be wrong.

Looks great now - RTBC - except for nit, still needs profiling and IS update:

+++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php
@@ -257,20 +264,24 @@ public function loadAllIncludes($type, $name = NULL) {
+

superfluous - space

Fixed the tiny whitespace niggle that Fabianx mentioned in #83

I forgot how amazing this was for D7 and lost this patch. Reapplied #9 and wanted to share:
The scenario is a cart page with cache primed.

I'll see what I can do about getting some profiling results on D8 site.

Of course I don't have as many modules enabled in D8 as I did in D7 so the gain is not as amazing.

Scenario: I turned page/dynamic page caching off, enabled all the core modules + xhprof + devel modules and rebuilt the registry and hit /admin/modules

The function doesn't get called as many times compared to the other but the time % diff is still nice:)

15% wall time improvement with a 7.5% memory regression seems to be a good trade off to me:

Here's an is_file() diff for the same scenario except there were more calls on the admin/content page from what looks like views.

It looks like something else is wrong that we're checking the existence of views.views_execution.inc 164 times on admin/content - do you know what's causing that? Are we really doing that for 164 different views hooks or something?

@catch I think you mis-read that picture. is_file() is called 164 times diff. views.views_execution.inc has 0 diff. Those are only ever checked once.

We checking for the existence of MODULE.views_execution.inc for every installed module multiple times because we fire these...

1. \Drupal::moduleHandler()->invokeAll('views_pre_view', array($this, $display_id, &$this->args));
2. $module_handler->invokeAll('views_pre_build', array($this));
3. \Drupal::moduleHandler()->invokeAll('views_query_alter', array($view, $this));
4. $query->addMetaData('views_substitutions', \Drupal::moduleHandler()->invokeAll('views_query_substitutions', array($this->view)));
5. $module_handler->invokeAll('views_post_build', array($this));
6. $module_handler->invokeAll('views_pre_execute', array($this));
7. $module_handler->invokeAll('views_post_execute', array($this));
8. $module_handler->invokeAll('views_pre_render', array($this));
9. $module_handler->invokeAll('views_post_render', array($this, &$this->display_handler->output, $cache));
10. $substitutions = \Drupal::moduleHandler()->invokeAll('views_form_substitutions');

We also try to load MODULE.views.inc for every installed module multiple times because of the alter $this->moduleHandler->alter($this->alterHook, $definitions); where the alter hooks are

1. views_plugins_field
2. views_plugins_filter
3. views_plugins_relationship
4. views_plugins_area
5. views_plugins_exposed_form
6. views_plugins_query
7. views_plugins_pager
8. views_plugins_join
9. views_plugins_cache

So for every module installed once views is installed you get 17 unnecessary is_file() checks on a cache rebuild.

Yes sorry #95 should've said MODULE.views_extension.inc, thanks for right answers to my wrong question.

This is another reason to get rid of hook_hook_info() per #2233261: Deprecate hook_hook_info() although that's 9.x material unfortunately.

In the meantime we ought to be able to check the file existence once per hook group per module per request, if we do that, I don't see what the static cache gets us in 8.x and would be tempted to move this directly to 7.x. Not changing status just yet though.

I think given how ModuleHandler::buildImplementationInfo() works the current solution to add static caching to ModuleHandler::loadInclude() might be the simplest. I'm not sure we should change module_load_include() - in fact I think we should open an issue to deprecate the function for 9.x and replace all of the usages in core where possible - the only place where it is not possible is the installer and for that we could just copy&rename the function and put it in install.core.inc.

Also we should store the results when is_file() returns FALSE - this way we would get 731 less calls to is_file() on admin/content as opposed to 165.

Here's a patch that stores the FALSE results of is_file()

The difference on a cache rebuild on admin/content:

The other advantage of caching the FALSE results of is_file() is that PHP doesn't cache information about non-existent files so funnily enough this is probably saving more disk access than the patch in #91.

Setting back to RTBC, thanks @alexpott nice extra touch.

re #99 This looks like the removal issue #697946: Properly deprecate module_load_include() and move it into \Drupal::moduleHandler() service

I've committed/pushed this to 8.1.x, without the module_load_include() hunk though since that doesn't seem relevant to 8.x if we cache the not-found files, and if we convert usages away and deprecate it too.

Moving to 7.x for backport, I don't think this is worth an 8.0.x backport (we'd have to deal with @internal-izing the protected property) - but if someone strongly feels it should go into 8.0.x then feel free to set to that version.

If it's good enough for D8, I guess it's good enough for D7 too. I verified that the code matches.

For Drupal 7, there is a possible variant without static cache, which is half slower when xdebug is enabled, but in par with static cache with opcache and without xdebug (did a few local benchmarks involving including something like 400 PHP files 10000 times):

function module_load_include($type, $module, $name = NULL) {
  if (!isset($name)) {
    $name = $module;
  }

  if (function_exists('drupal_get_path')) {
    $file = DRUPAL_ROOT . '/' . drupal_get_path('module', $module) . "/$name.$type";
    if (@include_once $file) {
      return $file;
    }
  }
  return FALSE;
}

@pounard what happens if the included file has a PHP error in? I think this would a change of behaviour.

That's an excellent question, I think that this would mask the warnings only if the code executes directly PHP code, or potentially mask fatal parsing errors (although I'm not sure about those) but it won't otherwise; but it needs maybe some more research or testing about this.

Back to needs review for choosing either option.

@alexpott's was a good one, and I think using the @ operator would indeed mask PHP errors when parsing the loaded files, I think it's a dangerous behaviour change. I'm sad we cannot get rid of all the is_file() calls but I don't see a better alternative than the static cache right now.

Indeed @ operator silences errors when the included file has any syntax error. Back to RTBC

Not exactly, the static cache should only be populated if the drupal_get_path() function exists: in case a file load attempt is done before Drupal is fully bootstrapped, it won't be able to load it once it is fully bootstrapped: so this must be changed in order to ensure that it can.

 function module_load_include($type, $module, $name = NULL) {
  static $files = array();

   if (!isset($name)) {
     $name = $module;
   }
 
  $key = $type . ':' . $module . ':' . $name;
  if (isset($files[$key])) {
    return $files[$key];
  }

   if (function_exists('drupal_get_path')) {
     $file = DRUPAL_ROOT . '/' . drupal_get_path('module', $module) . "/$name.$type";
     if (is_file($file)) {
       require_once $file;
      $files[$key] = $file;
       return $file;
     }
    else {
      $files[$key] = FALSE;
    }
   }
   return FALSE;
 }

should become:

 function module_load_include($type, $module, $name = NULL) {
  static $files = array();

  if (!function_exists('drupal_get_path')) {
    return FALSE;
  }

  if (!isset($name)) {
    $name = $module;
  }

  $key = $type . ':' . $module . ':' . $name;
  if (isset($files[$key])) {
    return $files[$key];
  }

  $file = DRUPAL_ROOT . '/' . drupal_get_path('module', $module) . "/$name.$type";
  if (is_file($file)) {
    require_once $file;
    $files[$key] = $file;
    return $file;
  }
  else {
    $files[$key] = FALSE;
  }
  return FALSE;
}

#106 doesn't populate the cache when drupal_get_path() is not available.

Oh yeah right, I misread, I don't know how I could miss that.

Statically caching the FALSE results gives me some pause, since it's certainly possible to download code and load/run it all in the same page request.... But I think in practice this isn't likely to be a problem, and it's essentially the direction we're headed in #1081266: Avoid re-scanning module directory when a filename or a module is missing anyway.

Committed to 7.x - thanks!

This issue has the "needs backport to D6" tag, but I'm guessing that's not happening at this point. Feel free to reopen for Drupal 6 if you want to try :)

Just a thought,

As we are keeping a static cache now and we can not include a file more than once, why are we still using the require_once instead of require only.

This comes from an old project that had to use Drupal through an NFS mount. And there were this kinds of issues:
https://bugs.php.net/bug.php?id=52312
https://echo.co/blog/speed-php-nfs-turborealpath

We can open this in a follow-up, as it's pretty straight forward.

BR,
Nikolay Dobromirov.

@ndobromirov: I think that's a great idea! Can you open a follow-up? Then we could discuss that change properly. Thanks!

@ndobromirov changing from require_once() sounds problematic. The file might already have been require()d by other code (it's often a good idea but it's by no means compulsory to use module_load_include()).

I agree with #121, it does not sounds very wise to switch to require.

The follow-up is here: https://www.drupal.org/node/2663228
There is a POC patch to see whether this will pass D7 core tests.
@pjcdawkins I did not consider this (#121) but it's a valid point. We will see what the test bot will say :).

follow-up or not, modules might require files by themselves, even if core passes tests, a lot of module may not.

Just wanted to say thank you.