Another spin-off from #238950: Meta: update.module RAM consumption, although this one is sadly a bit of a monster patch. But, I knew we were going to need something like this eventually...
Currently, the heart of update status is a GIGANTIC function, called update_calculate_project_data(). It iterates over every project and computes the status for each one. It takes the array of data about projects from update_get_projects() and the array of available release data from update_get_available() and friends. Sadly, it assumes we're going to save all of both, and is very careless about where it saves the data it computes. In almost all code paths, it appends the available release data for each project into the project data array for the update status. :( This is hugely costly to RAM, especially as the # of releases for a project grows.
Furthermore, being one monster function that iterates over all projects makes it a lot harder to write tests for update manager. :( We basically have to write them as end-to-end functional tests, instead of unit testing this logic directly.
So, I have a patch that does the following:
- Splits out the per-project logic into a separate function, update_calculate_project_update_status().
- update_calculate_project_data() is now reduced to the wrapper that deals with caching and iterating over projects.
- update_calculate_project_update_status() is careful to only save things we care about into the $project_data array, and we do *not* stash all of $available in there anymore.
So, although the patch is pretty huge, it's mostly due to the fact that a bunch of nested logic had to be indented differently as it moved into the per-project helper function (that can now finally be unit tested, yay!). A lot of variables also changed from stuff like $projects[$project] to $project_data and $available[$project] to $available...
I'll post it in a second, along with some preliminary performance numbers. Just want to watch all the existing functional tests pass locally first...
| Comment | File | Size | Author |
|---|---|---|---|
| #7 | 669626-7.update_calculate_project_update_status.patch | 33.43 KB | albert volkman |
| #1 | 669626-1.update_calculate_project_update_status.patch | 34.39 KB | dww |
Comments
Comment #1
dwwFor testing the gains, I found a couple of D7 projects with more than one release:
http://drupal.org/project/basic
http://drupal.org/project/fast_gallery
Downloaded a release of each to my site, and selected the checkbox to check status for disabled modules and themes. Then, I printed the size of the serialized array we save in {cache_update} for 'update_project_data':
Before: 45176 bytes
After: 30283 bytes
The gains only grow (a lot) the more projects you add, and the more releases each project has. So, this is one of those performance patches that's only going to be more and more important throughout the life of D7.
Comment #2
dave reidGood start for helping reduce the RAM usage. Looks good and tested well.
Comment #3
webchickI looked this over and didn't see anything out of the ordinary. It does indeed seem to be mostly just moving code around.
I like this patch for the following reasons:
1. It reduces memory consumption. Duh. :P
2. But it does so fairly dramatically, it seems. ~30% for a handful of projects with a handful of releases. Most active sites have many more modules with many more releases. I'd be curious what the figures look like on a typical D6 site.
3. It paves the way for more testable code.
dww said that the existing tests helped him catch bugs with this patch, so I'm going to assume that they're fairly solid, and if not, we'll commit test fixes along with bug fixes as a result of this patch. ;)
Committed to HEAD.
Comment #4
webchickOops. I meant this. I think.
Comment #5
dwwYay, thanks! Yes, it'd be nice to backport this to D6, too. However: a) I'll hopefully be on vacation for most of the next week and b) I'd rather not spend the time on the backport if it's doomed not to be committed. So, I'm going to unassign myself for now and email Gabor about this to get his feedback on the viability of committing a backported patch before I write it.
Comment #6
gábor hojtsyHm, looks like a useful improvement only changing the internal APIs used by update module, as far as I see from a quick glance. Seems to be a good win for D6.
Comment #7
albert volkman commentedD6 backport, 2.5 years later :)