We rebuild the registry a lot during the installation process, and most of the time in this rebuild is currently spent calling hash_file() on the content of the files all over again.

I suggest we ignore the files we already have parsed previously in the same request. In my testing, this significantly improves the performance of the installation process (executed through Drush), and it is likely to increase the performance of the automated testing too.

Files: 
CommentFileSizeAuthor
#14 drupal-1470656-14.patch1.75 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 40,766 pass(es).
[ View ]
#9 1470656-registry-parse-files-multiple-times-9.patch1.74 KBamateescu
PASSED: [[SimpleTest]]: [MySQL] 36,661 pass(es).
[ View ]
#7 1470670-drupalcachearray-destruct-invalid.patch2.57 KBDamien Tournoud
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1470670-drupalcachearray-destruct-invalid_1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#6 xhprof-before.txt1.3 MBDamien Tournoud
#6 xhprof-after.txt1.3 MBDamien Tournoud
#1 1470656-registry-parse-files-multiple-times.patch1.96 KBDamien Tournoud
PASSED: [[SimpleTest]]: [MySQL] 35,112 pass(es).
[ View ]

Comments

StatusFileSize
new1.96 KB
PASSED: [[SimpleTest]]: [MySQL] 35,112 pass(es).
[ View ]

Proposed patch.

Status:Active» Needs review

Status:Needs review» Reviewed & tested by the community

Not sure we should develop this against D8 since the registry will go away soon, see also #1541674: Remove the registry.

The approach looks sane and I can see that every call to module_enable() will trigger the parsing of all files, which is quite ridiculous.

Could you post some performance benchmarks for your use case? Would be interesting to see how much faster an installation of commerce kickstart or recruiter can be.

RTBC to me.

Issue tags:+needs backport to D7

Tagging.

Status:Reviewed & tested by the community» Needs work

Please post xdebug or xhprof output for the installer, or even time drush si would do if it's significantly different.

+  // During susequent rebuilds, we only add new files. It makes the rebuilding

Typo for subsequent.

The static flag then calling out to registry_get_parsed_files() meant I had to read the patch twice as well as digging out the function to see what was going on, wouldn't it be simpler to keep a static array of files that have already been parsed? If the answer is "no it's not" then I don't mind though.

NIce to see performance work happening on the installer again.

StatusFileSize
new1.3 MB
new1.3 MB

A typical installation of Commerce Kickstart using drush site-install commerce_kickstart --verbose.

Before:

real 7m10.698s
user 2m46.366s
sys 1m10.124s

After:

real 6m7.733s
user 2m27.961s
sys 1m3.828s

Attached the XHProf traces of the two Drush sessions.

Status:Needs work» Needs review
StatusFileSize
new2.57 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1470670-drupalcachearray-destruct-invalid_1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Reroll with the typo spotted in #5 fixed.

The static flag then calling out to registry_get_parsed_files() meant I had to read the patch twice as well as digging out the function to see what was going on, wouldn't it be simpler to keep a static array of files that have already been parsed? If the answer is "no it's not" then I don't mind though.

It might, but I want to change the structure of this function as less as possible. Every change we make we risk breaking stuff, and there is no point in fine tuning a component that is going away anyway. This patch is purely self-contained and cannot possibly break anything.

Status:Needs review» Needs work

The last submitted patch, 1470670-drupalcachearray-destruct-invalid.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.74 KB
PASSED: [[SimpleTest]]: [MySQL] 36,661 pass(es).
[ View ]

The wrong patch was rerolled in #7, this one should be better :)

Status:Needs review» Reviewed & tested by the community
Issue tags:+Testing system

Nice one. Good to go, I think.

oh, and this likely makes WebTestBase::preloadRegistry() obsolete...? (We can figure that out in a separate follow-up issue)

this looks good to me. i tried to think of how this would break, but came up with nothing.

Version:8.x-dev» 7.x-dev
Status:Reviewed & tested by the community» Patch (to be ported)

Thanks. This was RTBC already so I've gone ahead and committed/pushed to 8.x, moving to 7.x for backport.

Status:Patch (to be ported)» Needs review
StatusFileSize
new1.75 KB
PASSED: [[SimpleTest]]: [MySQL] 40,766 pass(es).
[ View ]

Rerolled.

Status:Needs review» Reviewed & tested by the community

Nice and clean reroll :)

Has anyone tested this patch with Drush commands that download/update new code? For example:

$ drush help pm-update
Display available update information for Drupal core and all enabled projects and allow updating to latest recommended releases. Also apply any database updates required (same as pm-updatecode + updatedb).....

I don't know much about the internals of how that command works, but if it's downloading new versions of a module and then trying to execute the updated code in the same request, it could run into problems with this patch, no?

Or actually even some of the simpler Drush commands also: If there's any Drush code that does "rebuild registry + download new code + rebuild registry again" in one request, that seems like this patch would leave the registry out of date until the next cache clear.

Besides the above, I suppose this is pretty safe.

Status:Reviewed & tested by the community» Needs review

That sounds like a "needs review."

This patch could be improved a bit: the static variable could be called $first_call for example, and a if ($first_call) {} could surround the first 5 lines (requiring database stuff), which would avoid some unnecessary require_once and static Database object calls.

Any progress here? Patch seams to work very nice.

14: drupal-1470656-14.patch queued for re-testing.