Problem/Motivation

drupal_system_listing() is called by four separate branches of logic during Drupal 7 installation, but provides no static caching for the results of its filesystem scan. Profiling with cachegrind shows this is a good candidate for improving Drupal installation performance (see #686196-107: Meta issue: Install failures on various hosting environments), which may alleviate the fact Drupal installation times out on some hosts.

Proposed resolution

Add static caching to drupal_system_listing().

Remaining tasks

User interface changes

None.

API changes

None.

Original report by @catch

See http://drupal.org/node/686196#comment-2668760 and downwards for why. I doubt we have time to be doing heavy refactoring of the installer, so we should at least avoid the duplicate work, if not the duplicate function calls.

Comments

Crell’s picture

Subscribing.

carlos8f’s picture

me too

das-peter’s picture

Priority: Normal » Major
Status: Active » Needs work
StatusFileSize
new1.13 KB

We should work out something asap. here.
Adding a simple static cache to file_scan_directory() speeds up a 404 page load by 50% on my test environment.
Attached patch contains the code I used to test this.

The question is, if there are any functions / constructs which use file_scan_directory() or drupal_system_listing() to detect/fetch changes that are made within the same request.
Anyway I think we could add an additional parameter to reset the cache to provide an easy way to get a fresh listing.

das-peter’s picture

Status: Needs work » Needs review
StatusFileSize
new14.87 KB

Attached patch adds static caching also to drupal_system_listing().
Now there's also the $reset parameter and I tried to figure out where I've to force a fresh list - mostly on install / update actions.
This patch doesn't claim to be complete - but it's hopefully a beginning. :)

Status: Needs review » Needs work
das-peter’s picture

Status: Needs work » Needs review
StatusFileSize
new15.39 KB

Fixes related to the failed tests.

Status: Needs review » Needs work

The last submitted patch, drupal-add-static-cache-to-file_scan_directory-733308-6.patch, failed testing.

das-peter’s picture

Status: Needs work » Needs review
StatusFileSize
new15.4 KB

*grml* Sometimes I'm feeling stupid. Hope the tests pass now.

moshe weitzman’s picture

IMO this is off the table for D7. Lets refactor for D8.

Anyway, what is scanning directories during a 404 page?

das-peter’s picture

@moshe I don't know what triggered the rescan on 404. Atm. I can't reproduce it :| But I think it was related to this issue #1081266: Avoid re-scanning module directory when a filename or a module is missing

catch’s picture

Version: 7.x-dev » 8.x-dev
Category: bug » task
Issue tags: +Performance

Moving to 8.x, tagging for backport just in case, changing to a task since since it's not a show-stopping performance issue, but still major since it can cause timeouts and similar on some setups.

catch’s picture

Issue tags: +Needs backport to D7

.

mfb’s picture

As I mentioned in #1081266: Avoid re-scanning module directory when a filename or a module is missing, drupal_get_filename() already has a static cache of $files found via drupal_system_listing(). So it'd be a good idea to analyze memory usage of what seems like redundant static caches.

catch’s picture

Status: Needs review » Closed (duplicate)
j0rd’s picture

Same problem exists in D6 when clearing the caches. is_dir is called on my install roughly 10,000 times 90% of them being from drupal_system_listing(). This adds roughly 3 seconds to page load on my VM.

j0rd’s picture

Issue summary: View changes

Added issue summary.

iainp999’s picture

Issue summary: View changes

EDIT: apologies, just saw the comment re: duplicate :)

Sorry to bump this, but we found this to be an issue when profiling of feature revert on a D7 site that we're working on.

I know this is a D8 issue with potential to backport, but any reason why the issue was not progressed?

Thanks in advance.