http://api.drupal.org/api/function/drupal_get_schema_versions/7

Get all possible functions, then loop through them, then multiply this by the number of modules installed. With the default profile, this equals 40,000 iterations, it must be well over 200,000 on some actual sites.

sun suggested using the registry, that saves a lot.

Even if we do #497118: Remove the registry (for functions), we could probably parse all .install files and find the update function signatures and it'd still be quicker than this.

HEAD:
3.53 [#/sec]

Patch:
4.39 [#/sec]

CommentFileSizeAuthor
#105 drupal-105-drupal_get_schema_versions-D6.patch2.22 KBmikeytown2
#104 drupal6_schema_versions-521838-102.patch1.06 KBjrchamp
#99 drupal6_schema_versions-521838-99.patch1.06 KBjrchamp
#97 drupal6_schema_versions-521838-97.patch1.24 KBjrchamp
#95 521838-quick-small-change-for-efficiency-gain-95-D6.patch1.27 KBseanburlington
#94 521838-quick-small-chnage-for-efficency-gain-94-D6.patch1.24 KBseanburlington
#78 schema_versions-521838-78.patch1.91 KBjrchamp
#71 schema_versions-521838-71.patch1.33 KBjrchamp
#68 schema_versions-521838-68.patch1.01 KBjrchamp
#58 schema_versions_corni_2.patch2.3 KBCorniI
#56 schema_versions_corni_1.patch2.31 KBCorniI
#55 head.png141.76 KBCorniI
#55 patch.png137.34 KBCorniI
#53 schema_versions_corni_0.patch2.31 KBCorniI
#52 schema_versions-521838-52.patch2.05 KBjrchamp
#46 schema_versions-521838-46.patch2.2 KBjrchamp
#44 schema_versions-521838-43.patch2.17 KBjrchamp
#38 schema_versions.patch2.22 KBcatch
#32 schema_versions.patch1.95 KBcatch
#30 drupal.schema-versions-18-docs.patch1.89 KBsun
#23 implode.png250.67 KBcatch
#22 drupal.schema-versions.patch1.52 KBsun
#18 preg_grep.png225.64 KBcatch
#18 preg_grep.patch1.83 KBcatch
#12 option1.png213.22 KBcatch
#12 option2.png209.25 KBcatch
#12 preg_schema.patch1.79 KBcatch
#10 damz.patch1.44 KBcatch
#10 damz.png203.4 KBcatch
#8 preg_schema.patch1.69 KBcatch
#8 patch.png241.08 KBcatch
#7 schema_versions.patch1.44 KBcatch
#1 schema_versions.patch896 bytescatch
patch.png205.37 KBcatch
HEAD.png108.22 KBcatch
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

catch’s picture

Issue tags: +Performance
FileSize
896 bytes
moshe weitzman’s picture

this looks fine. do you want to go further and add some _multiple logic so we do one query for a bunch of modules?

sun’s picture

Status: Needs review » Needs work

We could also skip

     if (strpos($function, $module . '_update_') === 0) {

by scanning for '%%_update_%%' in the LIKE query condition.

yched’s picture

silly nitpicking, but : is a placeholder needed for the '%update%' litteral ?

catch’s picture

Status: Needs work » Needs review

Added multiple, tested with devel module.

However there's two concerns I have with the current approach - if you update a module's code and forget to run update.php, we want to remind you as soon as possible - this patch requires a registry_rebuild() before you'll start getting prompted. Also this is the same in Drupal 6 http://api.drupal.org/api/function/drupal_get_schema_versions/6 and we can't backport the current patch, so I'd like to look at a backportable version which parses .install files directly itself, and compare performance against the query. We've gone down from 30% of execution time to 0.6%, but even if parsing .install files isn't as efficient it'd be more robust.

Damien Tournoud’s picture

Another option is to refactor http://api.drupal.org/api/function/system_requirements/7

system_requirements() doesn't need to know all the update number available, but simply need to know the number of the top-level update function. What about creating a quick drupal_get_all_update_available() that would return a (module => top level function) array?

catch’s picture

FileSize
1.44 KB

patch.

catch’s picture

FileSize
241.08 KB
1.69 KB

Here's a version using preg_match_all() instead of the direct query on the registry table. Advantages are we don't need to wait until a registry_rebuild() to inform people the module they just upgraded has database updates pending, and it should be backportable to D6. Disadvantage is I'm terrible and regexp, so if you can fix the very basic one here, please go ahead. This now takes 1.3% of page execution time, so not quite as snappy but cutting down 29% of page execution time instead of 30% ain't bad.

I'd originally had a look at doing this with token_get_all() as the registry does, but that was as slow as going through get_defined_functions() (c. 36,000 calls to next()).

Main question is if we can rely on regular expressions here to get the correct list of functions or not, if not, #7 is still good for D7.

Damien Tournoud’s picture

What about something like this?

$updates = &drupal_static(__FUNCTION__, NULL);
if (!isset($updates)) {
  $updates = array();
  $functions = get_defined_functions();
  $regexp = '/(' . implode('|', module_list()) . ')_update_(\d+)/';
  foreach ($functions as $function) {
    if (preg_match($regexp, $function, $matches)) {
      list(, $module, $number) = $matches;
      $updates[$module][] = $number;
    }
  }
  foreach ($updates as &$module_updates) {
    sort($module_updates, SORT_NUMERIC);
  }
}
return isset($updates[$module]) ? $updates[$module] : array();
catch’s picture

FileSize
203.4 KB
1.44 KB

Fixed up Damien's suggestion, 3.5% self, including time spent inside preg_match() it's over 7% though, whereas my last one is 1.8% incl.

Damien Tournoud’s picture

Interesting. I forgot to anchor the regexp, that should make it faster:

+  $regexp = '/^(' . implode('|', module_list()) . ')_update_(\d+)$/';

Another thing to consider: would simplifying the regexp as "/^(.*)_update_(\d+)$/" make it slower or faster?

catch’s picture

FileSize
1.79 KB
209.25 KB
213.22 KB

Both lead to about 4.5% incl.

To keep the patch options updated, attached option 1 along with both webgrinds.

sun’s picture

Weird idea: Can we skip the foreach entirely and do just one preg_match_all() instead?

  $updates = array();
  $functions = get_defined_functions();
  $functions = implode("\n", $functions['user']);
  $regexp = '/^(' . implode('|', module_list()) . ')_update_(\d+)$/';
  preg_match_all($regexp, $function, $matches);
  ...
Damien Tournoud’s picture

Another option is to do a preg_grep() upfront:

foreach (preg_grep($regexp, $functions['user']) as $function) {
  // do the preg_match() and stuff
}
Damien Tournoud’s picture

We want the first regexp to be as cheap as possible, so maybe just filtering the function names that end with figures would be enough?

// First, filter out all the functions that do not end by "_" followed by a number.
foreach (preg_grep('/_\d+$/', $functions['user']) as $function) {
  // do the costly preg_match() and stuff
}
catch’s picture

not with that regexp we can't, matches is empty. Interesting idea though.

Damien Tournoud’s picture

Here is what I meant:

foreach (preg_grep('/_\d+$/', $functions['user']) as $function) {
    if (preg_match($regexp, $function, $matches)) {
      list(, $module, $number) = $matches;
      $updates[$module][] = $number;
    }
}
catch’s picture

FileSize
1.83 KB
225.64 KB

I crossposted, was replying to sun's, but nice update anyway. I think we may have a winner!

Damien Tournoud’s picture

Ok, we need to document why we are doing that, then.

By the way, do we only have 65 update functions in core?

catch’s picture

Yeah we removed all the d5-6 upgrades so it's only 6-7 in there now. Not sure what to say for documentation, mind re-rolling with that?

Note I haven't profiled sun's implode() approach, but we're not going to get much better than 0.5% so if it's comparable would just be a matter of taste.

catch’s picture

Assigned: catch » Unassigned

Team effort at this point.

sun’s picture

Combination of a few suggestions.

catch’s picture

FileSize
250.67 KB

0.02% slower than Damien's but that's margin of error now.

sun’s picture

Most probably, the more modules with updates are installed the more likely the implode() approach will win. ;) Because we invoke preg_match() for all hook_update_N-alike function names being defined. Contrary to that, 1x implode() and 1x preg_match_all is very, very cheap. :)

Anyway, awesome collaboration, gentlemen :)

jrchamp’s picture

Nice job all! I tend to agree that the preg_match_all will likely scale better.

Damien Tournoud’s picture

Very unlikely. I'm pretty certain that the preg_grep() / preg_match() approach scales better.

Edit: and it does use less memory.

catch’s picture

Should be easy enough to swap the same function out on a heavily loaded D6 site and compare the two, but I don't have that set up locally at the moment so others are welcome.

sun’s picture

Status: Needs review » Reviewed & tested by the community

Either way ;) Marking #18 as ready to go. :)

Dries’s picture

Looks good, but can we have some code comments?

sun’s picture

hah, you can't sleep, or why are you here, but not in IRC? ;)

At your service. (with some grammar + language help from dbabbage in IRC)

drewish’s picture

Status: Reviewed & tested by the community » Needs work

a couple of small nits to pick but sun loves dropping into other people's issues and tossing off a few and kicking it back to CNW so i think he'll understand ;)

+    // Filter all defined user functions by functions ending in numbers first
+    // (as module update functions do).

I think it'd be better to reference hook_update_N() rather than the more vague "module update functions".

+      // If this function is a module update function, add it to the list of
+      // module updates.

Same goes here. I guess both comments should be reworked to make it clear that first we'll make two passes first with the looser regex looking for functions ending with a numeric digit then reducing it further to those matching the hook_update_N() pattern.

Other than that it seems good to go.

catch’s picture

Status: Needs work » Needs review
FileSize
1.95 KB

Re-rolled with comments.

drewish’s picture

Status: Needs review » Reviewed & tested by the community

comments look good to me. bumping back to previous status.

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

deekayen’s picture

Status: Needs work » Reviewed & tested by the community

trying to get a new testing client working. it's not there yet.

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

catch’s picture

Status: Needs work » Reviewed & tested by the community
catch’s picture

FileSize
2.22 KB

Improved code comment to explain /why/ we use preg_grep() following discussion with webchick in irc.

webchick’s picture

Status: Reviewed & tested by the community » Needs review

Hm. I'm still confused by this code, which might mean the the docs need a bit of improvements, or it might mean that this still needs work.

+++ includes/install.inc	3 Aug 2009 21:28:18 -0000
@@ -92,24 +92,33 @@ function drupal_load_updates() {
+    foreach (preg_grep('/_\d+$/', $functions['user']) as $function) {

Curious. Why _\d+$ and not _update_\d+$? I understand why just "_update_" is a bad idea, but it seems odd that you're re-running another preg_match just below to check for something you could've caught the first time.

In fact, why not cut to the chase and just use $regexp to begin with?

I'm on crack. Are you, too?

Damien Tournoud’s picture

Status: Needs review » Needs work

In fact, we are changing the API here for no good reasons. All known modules (enabled or not) are updated in D6. Here we will only pick the enabled modules. I don't believe we need to specialize the inner regexp per module, let's just pick whatever is before the '_update_N' and take that as the module name, just as before.

drewish’s picture

dmaz, what do you mean by "updated in D6"?

jrchamp’s picture

@Drewish, I think he means that whenever a module is "known" to the Drupal installation that it runs the associated updates whether that module is enabled or not. The difference being that module_list() won't return the disabled modules whereas drupal_get_schema_versions() could in theory be used on a module that isn't enabled but whose functions are defined. I'm not sure how that works, but it seems like a slight change in functionality. Though it's probably better if disabled modules don't get included anyway given that a broken module which is disabled could then fatal error the admin.

@webchick, #13 and #15 relate to your solution. They wanted to make it as small an impact as possible. However, I agree that matching with the additional literal '_update_' in addition to the '\d+$' would likely be ideal in minimizing false positives while hopefully not slowing the matching performance.

@Damz, were you thinking something like this?

  $updates = &drupal_static(__FUNCTION__, NULL);
  if (!isset($updates)) {
    $updates = array();
    // Prepare regular expression to match all possible defined hook_update_N().
    $regexp = '/^(.+)_update_(\d+)$/';
    $functions = get_defined_functions();
    // Narrow this down to functions ending with an integer, since all
    // hook_update_N() functions end this way, and there are other
    // possible functions which match '_update_'. We use preg_grep() here
    // instead of foreaching through all defined functions, since the loop
    // through all PHP functions can take significant page execution time
    // and this function is called on every administrative page via
    // system_requirements().
    foreach (preg_grep('/_update_\d+$/', $functions['user']) as $function) {
      // If this function is a module update function, add it to the list of
      // module updates.
      if (preg_match($regexp, $function, $matches)) {
        list(, $module, $number) = $matches;
        $updates[$module][] = $number;
      }
    }
    // Ensure that updates are applied in numerical order.
    foreach ($updates as &$module_updates) {
      sort($module_updates, SORT_NUMERIC);
    }
  }

  return isset($updates[$module]) ? $updates[$module] : FALSE;
jrchamp’s picture

Status: Needs work » Needs review

Rolled a patch with the changes from #42 and a variable overwrite issue for $module.

- $regexp = '/^(' . implode('|', module_list()) . ')_update_(\d+)$/';
+ $regexp = '/^(.+)_update_(\d+)$/';

- foreach (preg_grep('/_\d+$/', $functions['user']) as $function) {
+ foreach (preg_grep('/_update_\d+$/', $functions['user']) as $function) {

- list(, $module, $number) = $matches;
- $updates[$module][] = $number;
+ list(, $module_name, $number) = $matches;
+ $updates[$module_name][] = $number;

Any objections?

jrchamp’s picture

Status: Needs review » Needs work

The last submitted patch failed testing.

jrchamp’s picture

Status: Needs work » Needs review
FileSize
2.2 KB

Not ideal, but this passes the tests. catch, does this still provide a benefit or does it defeat the purpose?

Status: Needs review » Needs work

The last submitted patch failed testing.

jrchamp’s picture

Status: Needs work » Needs review

The tests still pass for me. Kicking the test bot.

catch’s picture

This is still faster, but not as fast as previous versions:

HEAD about 30%, patch about 14%. Well worth committing but not as dramatic as previous versions (although possibly they were dramatic because they broke things...).

alexanderpas’s picture

+    foreach (preg_grep('/_update_\d+$/', $functions['user']) as $function) {
+      // If this function is a module update function, add it to the list of
+      // module updates.
+      if (preg_match($regexp, $function, $matches)) {
+        list(, $module_name, $number) = $matches;
+        $updates[$module_name][] = $number;
+      }
+    }

- is that if statement inside the foreach "unneeded", or do the the two regexp used not return exactly the same?
- isn't a strpos and and two substr faster than the preg_match and the list?

jrchamp’s picture

The preg_match matches are subsets of the string, whereas the preg_grep matches are a subset of the array. The preg_grep operates on an array of items and returns the matching items. The preg_match operates on a single item and returns the substrings of the items.

The problem with strpos is that we would need to do strrpos and two substr's. The list() is a language construct and not a function so it doesn't incur the function overhead. It is possible that strrpos and two substr's would be faster than a function call which uses a regex, but I'd want to see some numbers before I'd believe it. The nice thing about using the same regex multiple times is that it should already exist preprocessed within the regex engine. If you're considering running the numbers, you might also want to try:

  list($module_name, $number) = explode('_update_', $function);
jrchamp’s picture

catch, this (with the change I proposed in #51) saves us a preg_match for each matched function. Of course, this would conflict with modules whose name contains "_update". Currently, that list consists of http://drupal.org/project/jquery_update and http://drupal.org/project/jquery_form_update. Of those, the only current function which is applicable to Drupal 7 is jquery_update_update_7000. Perhaps mfer could weigh in on this? The whole point would be a performance and readability gain, so I'm not all that excited about doing an array_pop() and an implode().

CorniI’s picture

based on the patches above, I rolled a version which a) should maintain D6's behauvior and b) ist (profiled) faster than the patch by jrchamp on the admin index.
I query {system} for all modules which drupal has knowledge about. This is okay, because modules just uploaded, but never installed are not shown there. If a module wasn't ever installed, we don't have to update anything for it. That approach is slightly slower than the API change catch introduced.

catch’s picture

This seems like a decent approach. Cornil - could you post a screenshot from your profiler?

CorniI’s picture

FileSize
137.34 KB
141.76 KB

I missed the duration of the patch request on the screenshot, it's 590ms.
I don't know how to make screenshots showing the whole thing, so just the importan sections here, sorry.

CorniI’s picture

whitespace+80cols fix

chx’s picture

Status: Needs review » Needs work

use $regexp = '/^(?P<module>' . implode('|', $modules) . ')_update_(?P<version>\d+)$/'; and then $updates[$matches['module']][] = $matches['version']; way more pleasant to read isnt it?

CorniI’s picture

Status: Needs work » Needs review
FileSize
2.3 KB

it is, and removes the awkward list construct :)

chx’s picture

Status: Needs review » Reviewed & tested by the community

Good job! Benchmarks in #55.

jrchamp’s picture

I am concerned that the preg_grep does not contain '_update_'. Does it really hurt the performance to be a little more specific? Thank you for coming up with a better solution for this one.

CorniI’s picture

Yes, in that case the performance hit is measurable, because the regex needs to be applied to every array key string. The smaller, the better, and we filter out later the functions we need anyways.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Reviewed and committed to CVS HEAD. Thanks for the careful tweaking and performance testing!

catch’s picture

Version: 7.x-dev » 6.x-dev
Status: Fixed » Patch (to be ported)

Same code is in D6 too.

Damien Tournoud’s picture

Version: 6.x-dev » 7.x-dev
Status: Patch (to be ported) » Active

This patch is broken. Install a clean HEAD: the status report says "Database updates: out of date".

CorniI’s picture

Dries/webcick, can we get a rollback on this so I can work without time pressure on the issue?
thanks!

clemens.tolboom’s picture

andypost’s picture

subscribe :(

jrchamp’s picture

Status: Active » Needs review
FileSize
1.01 KB

This appears to resolve the installation issues, but is definitely a step backward in performance. The static isn't being reset after a new module is installed, and the system module behaves differently than all the others.

Dries’s picture

Sorry, I was on a plane yesterday. I'll wait for the test results on #68 and either go with #68 or rollback the previous patch.

Dries’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

I committed the patch in #68 as the tests came back green. Thanks @jrchamp.

Marking this 'code needs work' so we can brainstorm on better solutions, and so we can work on better tests. The latter seems to be a requirement given that none of us reviewers spotted the brokenness in the previous patch.

jrchamp’s picture

This patch resolves the installation issues, maintains the performance gains in the admin, but may have unexpected issues with modules added after installation. Also, this doesn't add any tests, something that Dries requested in #70 (which is why I'm not marking this needs review).

Need some feedback here, as this may not be the best solution. For reference, the reason that we don't use the installed modules list is that the 'system' "module" is not in the system table during installation before it calls this function.

catch’s picture

Since the last patch broke the tests at http://drupal.org/node/211182#comment-2073754 - and it looks like that already covers which updates to expect for which module, it'd make sense to consolidate the testing efforts between the two issues.

catch’s picture

Status: Needs work » Needs review

Yep, #211182: Updates run in unpredictable order has copious tests which we don't need to duplicate, seeing if this still applies.

Crell’s picture

Subscribing.

Issue tags: -Performance, -Needs tests

drifter requested that failed test be re-tested.

sun.core’s picture

Issue tags: +Performance, +Needs tests

#71: schema_versions-521838-71.patch queued for re-testing.

Gábor Hojtsy’s picture

@jrchamp: can you qualify those "unexpected issues" which "might" appear with the patch. That's not too clear.

jrchamp’s picture

#71 attempts to achieve the performance benefits of not re-caching the module updates unless we know that the list of modules is changing. The two locations where the list is refreshed as identified by the patch are required for a successful installation. However, my concern about unexpected issues are for situations where modules are loaded after the first time that this function executes. Any location which loads a module after the first time this function is run will need to clear the static for this function.

Rather, as the situation that would cause a problem for CVS HEAD and #71 is when a module is freshly loaded, it would probably be worthwhile to alter this function to set entries in the array which are "empty" even for modules which have no updates. As such, this function would have performance similar to #71 (assuming the module_list is already cached or will be cached during normal execution), but without the need to reset the static in multiple places within the codebase.

I have attached the patch, which assumes that module_list provides the list of "loaded modules" as the documentation implies. I did not get a chance to benchmark this, but did perform a couple fresh installations to verify basic functionality.

catch’s picture

@jrchamp - it might be worth looking at using system_list() directly here instead of module_list() (that's where the data is got from, and also cached now) - would that help at all? Doesn't have the same bootstrap / fixed_list / normal craziness of module_list()

Gábor Hojtsy’s picture

How does this improve performance? What I'm seeing is that by checking the return value a bit earlier, we save calling max(FALSE) and comparing the resulting 0 to another number. This looks like little code reorganization for barely noticeable performance benefits at best. Am I missing something?

jrchamp’s picture

The max(FALSE) check is really a bugfix that I noticed during testing. It prints an error to the screen when it is doing updates saying that the parameters to max() are incorrect.

The one small change is the following: if we assume that the modules loaded into memory (thus, the functions that would show up in the get_defined_functions() call) are returned by the module_list() / system_list() function call, then we can say "don't recache the updates array when you're checking to see if updates exist for a module which doesn't define any updates". The switch from isset() to empty() is to take into account the situation where the module either: !isset() || (is_array() && count() == 0)

Example:

If there was a module named 'mymodule' which had no updates, the updates array might look like this:

$updates = array(
'system' => array(7000, 7001, 7002),
'mymodule' => array(),
);

When we call drupal_get_schema_versions for 'mymodule', it will used the cached version of the updates array, rather than refreshing it simply because 'mymodule' didn't define a list of updates.

drupal_get_schema_versions('mymodule');

Honestly, I don't think that module_list() or system_list() are the functions that I want. I'm pretty sure that I want to be able to ask the static within drupal_load what modules it has already loaded, because it looks like system_list() does a query against the database to see what modules it thinks are loaded rather than giving me the list of modules whose module_update_number functions have already been loaded. Do either of you know where I can get that data? I realize that if module_load_all has already been run, then module_list will at least match the items in memory, but I wasn't sure if I could rely on it.

@catch: If system_list is what I want, you're saying that I should call: system_list('module_enabled') correct?

jrchamp’s picture

@Gábor Hojtsy - Does #81 make sense?

catch’s picture

@jrchamp: system_list('module_enabled') ought to work yeah.

jrchamp’s picture

@catch: After comparing http://api.drupal.org/api/function/module_list/7 and http://api.drupal.org/api/function/system_list/7, it looks like module_list is going to be faster. After all, it's the only place in core that called system_list('module_enabled'), and doesn't have the drupal_static / cache_get calls.

Quick summary for others just getting here: where #68 (HEAD) plays it safe and only remembers which modules define update functions, #78 attempts to "remember" all modules which have been loaded so that modules which do not define update functions will not force a cache refresh on drupal_get_schema_versions(). module_list() gets called many times during a normal page load, so its output is going to be cached already. #78 is only really worthwhile if drupal_get_schema_versions() is refreshing its cache unnecessarily when using #68 (HEAD).

catch’s picture

@jrchamp: are we sure that this function is only ever called after a full bootstrap, if so that sounds fine, if not system_list() guarantees that boostrap modules aren't returned but yeah there'd be no other reason to use it.

jrchamp’s picture

@catch: That's the problem. Because module_list relies on system_list, which pulls its data from the database, it only works after all modules have been loaded. Really, what I would want is for a function like the following to be called whenever a module is loaded into memory (10-20 calls of a very simple function):

function loaded_module_list($module = NULL) {
static $modules = array();
if (isset($module)) {
$modules[$module] = $module;
}
return $modules;
}

The nice thing is that we won't even have to have a reset parameter, because the active process can't "unlearn" the code it has loaded.

Does this seem valuable? Other suggestions?

grendzy’s picture

I tested #78 with all core modules enabled. There was no measurable performance difference on /admin, using `ab -C`. Is there something else I should be testing?

catch’s picture

Title: drupal_get_schema_versions() takes 30% of page execution time on /admin » Clean up drupal_get_schema_versions()
Priority: Critical » Normal

afaik there is no measurable performance improvement with the latest patch, the 30% was knocked down to 1 or 2%, this is just clean up now (and maybe saves a few cycles in the process, but nothing you could measure with ab). Because of that I'm downgrading from critical.

grendzy’s picture

Status: Needs review » Reviewed & tested by the community

catch, thanks for clarifying. #78 seems like a sensible cleanup. It does result in fewer loops through get_defined_functions().

grendzy’s picture

Issue tags: -Needs tests

as pointed out above, hook_update_dependencies() added tests so we should be covered.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

It looks like Gábor's comments were addressed. The patch looks fairly harmless (which probably means it will break everything ;)).

Committed #78 to HEAD.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

David_Rothstein’s picture

seanburlington’s picture

Version: 7.x-dev » 6.x-dev
Status: Closed (fixed) » Needs review
FileSize
1.24 KB

Hi,
I've been looking into why the admin page on my clients site is so slow

This function seems to be the culprit - as far as I can see no patch has yet been provided for Drupal 6 - so here's mine

It's a very quick piece of code with the aim of making minimal changes

What it does it to cache an array of functions with the string 'update' in them

This is a much smaller list that all functions

So iterating over this list for every module is less painful.

I make no claim to this being anything like a perfect solution - but it performs bearably well on a site with 200 modules !

seanburlington’s picture

bah! last patch was too quick and didn't work!

This once still has a speed up - but also works

iterate over 600 functions with _update_ in then instead of 7000 total user functions - for each module

drewish’s picture

There's some obvious code standards issues (tabs rather than spaces) and I wonder if it makes sense to just move that functionality into drupal_get_schema_versions() rather than having the _drupal_get_update_functions() helper function.

jrchamp’s picture

@seanburlington: Try this patch out. It is much faster on my system.

Status: Needs review » Needs work

The last submitted patch, drupal6_schema_versions-521838-97.patch, failed testing.

jrchamp’s picture

Status: Needs work » Needs review
FileSize
1.06 KB

I must have missed the Git memo. That might explain the other problems I was having. The newly diffed patch attached to this comment should work.

Status: Needs review » Needs work

The last submitted patch, drupal6_schema_versions-521838-99.patch, failed testing.

kenorb’s picture

+1

jrchamp’s picture

Status: Needs work » Needs review
Issue tags: -Performance

Status: Needs review » Needs work
Issue tags: +Performance

The last submitted patch, drupal6_schema_versions-521838-99.patch, failed testing.

jrchamp’s picture

Status: Needs work » Needs review
FileSize
1.06 KB

It looks like the test bot wasn't applying patches back then.

mikeytown2’s picture

Category: bug » task
FileSize
2.22 KB

Did some benchmarks on our large site

D6 Core: 2112ms
Patch #104: 159ms
D7 backport (this patch): 10.7ms

Added link to this patch to the D6 Performance wiki as well.

BrockBoland’s picture

Needs issue summary (partial in #84)

softescu’s picture

We have two different patches for D6 and D7. Patch #105 for D6 approach is different than patch #71 for D7 (summary at #78). I am going to recheck now to see if the patches still applies to D6 core and D7 core.

mikeytown2’s picture

@softescu
#91 committed #78 which is based off of #71. #105 is a backport from whats in D7.

Status: Needs review » Closed (outdated)

Automatically closed because Drupal 6 is no longer supported. If the issue verifiably applies to later versions, please reopen with details and update the version.