speedup simpletest by preloading the registry [#340052]

starting this issue following from some discussion in #drupal. not 100% sure on the best way forward in terms of implementation yet.

select bits from #drupal below.

DamZ noted that simpletest spends a long time rebuilding the registry:

(10:50:20) DamZ: chx: I just ran a profile on simpletest... did you know that nearly 70% of the time is spent rebuilding the registry?
(10:50:43) beejeebus: DamZ: yes!
(10:50:50) DamZ: chx: more precisely skipping body of functions (46% of the time is spent in _registry_skip_body())
(10:54:07) beejeebus: DamZ: i was suggesting targeting the registry when i saw this http://drupal.org/node/323477
(10:54:09) Druplicon: http://drupal.org/node/323477 => Tune SimpleTest => Drupal, simpletest.module, normal, patch (code needs work), 11 IRC mentions

later, chx suggested we use a prebuilt registry for "every module out there", and use that to rebuild:

(13:02:44) chx: beejeebus: hi
(13:02:49) chx: beejeebus: i just read backscroll
(13:02:53) beejeebus: chx: hi
(13:03:18) beejeebus: chx: speeding up simpletest?
(13:03:27) chx: beejeebus: we can have a registry prebuilt for every module out there and do an insert select join with system table, thansk to m-i patch we have a module column
(13:04:10) beejeebus: chx: ooh, that sounds tasty
(13:04:36) chx: beejeebus: my hands are more than full with the boot from sqlite patch. you are free to run with the idea though.
(13:05:01) beejeebus: chx: when talking to boombatower, i was just suggesting a copy of the whole registry for a default profile or similar
(13:05:22) beejeebus: chx: your approach sounds a bit more complex, but much more powerful
(13:05:58) beejeebus: chx: i'll start an issue
(13:07:17) chx: beejeebus: thankie

Comment	File	Size	Author
#18	simpletest.speedup.patch	1.72 KB	Anonymous (not verified)
#18
#17	cache_registry_parse_3.patch	12.99 KB	dmitrig01
#17
#14	cache_registry_parse.patch	10.74 KB	Anonymous (not verified)
#14
#13	cache_registry_parse.patch	11.8 KB	Anonymous (not verified)
#13
#12	cache_registry_parse.patch	11.79 KB	Anonymous (not verified)
#12
#7	cache_registry_parse.patch	11.44 KB	Anonymous (not verified)
#7
#4	cache_registry_parse.patch	9.38 KB	Anonymous (not verified)
#4
#2	speedup.patch	8.92 KB	chx
#2

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

chx CreditAttribution: chx commented 28 November 2008 at 02:25

yeah module_rebuild_cache returns every module be it enabled or not so, throw them to DWTC::setup, pick up the registry & registry_files tables and later on skip the registry rebuilds just INSERT SELECT from those tables.

Comment #2

chx CreditAttribution: chx commented 29 November 2008 at 08:43

Title:	speeding up registry rebuild	» simpletest speedup
Assigned:		» chx
Status:	Active	» Needs review

File	Size
speedup.patch	8.92 KB

Here we come. We build the all-registry once and then select from it as necessary. I am not particularly fond of the system_schema hack but I be damned if I change the install system just for this to check for the existence of every table. I was toying with the reuse of registry_rebuild but I do not think it worths making that function ugly. The patch, aside from the system_schema trickery is entirely contained in dwtc.php. Despite Damien's finding I am not seeing the tests flying but let's see what others can see. Edit: I have neither tested heavily nor debugged this heavily to see whether it indeed skips registry body skips as we wanted.

Comment #3

29 November 2008 at 08:55

Status:

Needs review

» Needs work

The last submitted patch failed testing.

Comment #4

Anonymous (not verified) CreditAttribution: Anonymous commented 29 November 2008 at 12:37

File	Size
cache_registry_parse.patch	9.38 KB

patch attached that takes a different tack to chx patch.

i'm seeing a speed up of ~1.5 second per test in DWTC->setUp with this patch with a naive 'just copy the whole thing' registry preload.

not setting to code needs review until i've had a chance to discuss with chx.

Comment #5

chx CreditAttribution: chx commented 29 November 2008 at 15:38

Title:	simpletest speedup	» simpletest speedup (and more)
Status:	Needs work	» Needs review

That's a pretty good idea! We need to make sure that the registry queries are indexed but I think they are and then having a few disabled rows in there matter little IMO.

Comment #6

29 November 2008 at 16:05

Status:

Needs review

» Needs work

The last submitted patch failed testing.

Comment #7

Anonymous (not verified) CreditAttribution: Anonymous commented 29 November 2008 at 20:37

Status:

Needs work

» Needs review

File	Size
cache_registry_parse.patch	11.44 KB

new patch, makes registry tests pass. setting to code needs review.

i'll try to get some benchmarks on funning the full test suite, and on normal pages to make sure this isn't a regression.

Comment #8

Anonymous (not verified) CreditAttribution: Anonymous commented 29 November 2008 at 21:46

i'm seeing about 35% speed up for a full test run with this patch.

running all tests in the browser, i got 19 mins 46 seconds on HEAD, 12 mins 55 sec HEAD + patch.

would love some other confirmation of this.

Comment #9

catch

he/him

English

CreditAttribution: catch commented 29 November 2008 at 22:02

Why is the testbot saying only 105 passes?

Comment #10

Dave Reid

he/him

English

Nebraska USA

CreditAttribution: Dave Reid commented 29 November 2008 at 22:15

Wow....drastic improvement! Worked great on MySQL, PostgreSQL, and SQLite. Upgrade worked perfectly as well.

Comment #11

catch

he/him

English

CreditAttribution: catch commented 29 November 2008 at 22:55

retest seems to have worked, hopefully just a glitch. Very nice improvement.

Comment #12

Anonymous (not verified) CreditAttribution: Anonymous commented 29 November 2008 at 22:57

File	Size
cache_registry_parse.patch	11.79 KB

updated patch, added some comments, and took out a superfluous menu_rebuild.

Comment #13

Anonymous (not verified) CreditAttribution: Anonymous commented 29 November 2008 at 23:47

File	Size
cache_registry_parse.patch	11.8 KB

rerolled now that #338184: Don't avoid serialize() in the registry is in.

Comment #14

Anonymous (not verified) CreditAttribution: Anonymous commented 30 November 2008 at 01:21

File	Size
cache_registry_parse.patch	10.74 KB

rerolled now that #340500: don't init db twice has landed.

Comment #15

webchick

she/they

English

Vancouver 🇨🇦

CreditAttribution: webchick commented 30 November 2008 at 02:20

Hm. This patch has brought to light that I don't quite grok this registry stuff atm. I had assumed that the registry was already only storing function definitions for enabled modules, and that that's why we require contributed module authors to do the files[] stuff in their .info file now. chx and justin explained that no, the registry scans through ALL code of ALL modules, regardless if they're enabled or not, and adds them to the table. That seems incredibly wasteful to me. There is a huge trend in contrib to ship "packages" of modules (think Ubercart as an extreme example) and the entire package might have 80 modules with 50,000 lines of code, of which you're maybe using 5 modules and 2,000 lines. User expectation is "if a module is disabled, Drupal should forget this code exists." not "if I *really* want Drupal to forget that code exists, I need to not only disable it but also remove it from the disk." So the addition of the "enabled" column is a red flag to me, as I feel this is how the registry should already be working -- only indexing currently-enabled modules.

When I inquired further as to the reason for this "all code everywhere" approach, they said this was necessary in order for SimpleTest to be performant; if it had to constantly be re-building this table on every test run to reflect the currently enabled modules for that test run, performance would die a horrible nasty death. If that's the only reason though, we should special-case SimpleTest to bloat the registry table to do what it needs to do, not bloat the registry tables of 100% of the Drupal 7 sites out there for the 0.5% that are actively running SimpleTest.

Am I wrong about this?

Comment #16

Anonymous (not verified) CreditAttribution: Anonymous commented 30 November 2008 at 02:35

sometime multi-person IRC chats don't quite work as planned ;-)

currently, the registry only scans files in modules.info:

  foreach (module_rebuild_cache() as $module) {
    if ($module->status) {
      $dir = dirname($module->filename);
      foreach ($module->info['files'] as $file) {
        $files["$dir/$file"] = array('module' => $module->name, 'weight' => $module->weight);
      }
    }
  }
  foreach (file_scan_directory('includes', '/\.inc$/') as $filename => $file) {
    $files["$filename"] = array('module' => '', 'weight' => 0);
  }

webchick: the registry does work as you expect right now - only scans files for enabled modules, and only ones defined in .info.

this patch takes out the if ($module->status) check, and scans all files defined by .info, even if the module is disabled. basically, if a module is returned by module_rebuild_cache, we scan its files.

Comment #17

dmitrig01 CreditAttribution: dmitrig01 commented 30 November 2008 at 03:16

File	Size
cache_registry_parse_3.patch	12.99 KB

would this work?

Comment #18

Anonymous (not verified) CreditAttribution: Anonymous commented 30 November 2008 at 06:15

Title:

simpletest speedup (and more)

» speedup simpletest by preloading the registry

File	Size
simpletest.speedup.patch	1.72 KB

ok, here goes, without the kitten deaths.

just preload the registry. thats it.

queries in DrupalWebTestCase::setUp go from 4250 to 2601 for aggregator.

haven't benchmarked yet.

Comment #19

dmitrig01 CreditAttribution: dmitrig01 commented 30 November 2008 at 06:16

Dead kitten count: 0

Comment #20

webchick

she/they

English

Vancouver 🇨🇦

CreditAttribution: webchick commented 30 November 2008 at 07:06

Oh. My. God.

I chose the DB tests as a means of testing this. On my machine they take almost exactly 20 minutes to run. I then applied the patch and re-ran the tests. 5 minutes. No, I am not making this up. :O

Committed to HEAD with extreme glee and pleasure. THANKS!!!