Splitting this off as a sub-issue of #238950: Meta: update.module RAM consumption so that each self-contained improvement can be in its own issue, patch and status, instead of trying to fold everything into a monster patch...

Inside _update_process_info_list() (called by update_get_projects()) we generate an array of projects on the site with various info about each one that's important for computing the update status. In there, we're currently stashing the entire contents of the .info file, Just In Case(tm). ;) We really only need a handful of the .info attributes. Stay tuned for a patch with some performance numbers...

CommentFileSizeAuthor
#14 669554-14.update_filter_project_info_no_hidden.patch576 bytesdww
#13 669554-13.update_filter_project_info.d6.patch1.62 KBdww
#1 669554-1.update_filter_project_info.patch1.62 KBdww
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dww’s picture

dww
we/he/they
CreditAttribution: dww commented
Status: Active » Needs review
FileSize
669554-1.update_filter_project_info.patch1.62 KB

On a simple D7 test site with core and 3 contribs, the size of the serialized array we save in {cache_update} in the 'update_project_projects' cid went from 16782 bytes to 3097 (the biggest win is *not* saving the 'files' array for each .info file). Tests all still pass, running things manually all works, even with cvs_deploy enabled, etc.

Crell’s picture

Crell CreditAttribution: Crell commented

Eeep. There are a few contribs that currently rely on being able to add their own keys to module info files. The features module comes to mind as one. I'm developing another for D7 right now as well. If we start whitelisting keys in there we're going to break a fair bit of existing contrib functionality.

Status: Needs review » Needs work

The last submitted patch, 669554-1.update_filter_project_info.patch, failed testing.

dww’s picture

dww
we/he/they
CreditAttribution: dww commented
Status: Needs work » Needs review

@Crell: this is *just* for the *copy* of that data that's cached by update manager to compute update status. Everyone else is free to do whatever they want.

@testbot: WTF? How on Earth did this patch cause DBLogTestCase to fail? :( Requesting retest. Hurray for deterministic tests! :/

Re-test of 669554-1.update_filter_project_info.patch from comment #1 was requested by dww.

ksenzee’s picture

ksenzee
she/her
Location Seattle area
CreditAttribution: ksenzee commented

@Crell: Sure, but update.module doesn't need those keys, right?

And bumping back to needs review in case testbot is on crack.

Crell’s picture

Crell CreditAttribution: Crell commented

Hm, OK, update module I can't see a reason why it needs anything but name and version. But, er, why is it caching it separately anyway when it can request it from the system table? There's even a utility function for it.

dww’s picture

dww
we/he/they
CreditAttribution: dww commented

Crell: because it needs this data along with a bunch of other stuff about the project that's computed. And it seems better to just stash what we need all in one place than to have to do separate queries to recreate it all the time whenever we're trying to compute the status (which is every page load in /admin/*, for example).

Test bot was indeed on crack. #1 is RTBC by my tastes... Anyone have a reason why not? ;)

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented
Status: Needs review » Reviewed & tested by the community

Nope.

Crell’s picture

Crell CreditAttribution: Crell commented

OK, I misunderstood what we were doing here. No objection from me. Carry on.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Fixed

This seems like a sensible improvement, with nice results (~80% reduction?). Committed to HEAD.

I had asked in IRC whether we needed to care about dependencies[] for sub-themes, and dww pointed out that we use 'base theme' property for that, and that's already cached elsewhere.

I also asked whether we need to backport this to D6, but since the files[] array was the biggest issue, and we don't have that in D6, we're probably okay.

dww’s picture

dww
we/he/they
CreditAttribution: dww commented
Version: 7.x-dev » 6.x-dev
Status: Fixed » Reviewed & tested by the community

Well, actually, patch #1 applies (with minor fuzz) to DRUPAL-6, and it won't hurt anything. It will allow us to avoid caching stuff like 'description', 'dependencies', and other info that we don't care about, all of which can add up, especially since we carry this data around. Might as well backport to D6...

dww’s picture

dww
we/he/they
CreditAttribution: dww commented
Status: Reviewed & tested by the community » Needs review
FileSize
669554-13.update_filter_project_info.d6.patch1.62 KB

Actually, minor reroll to a) remove fuzz/offsets and b) remove 'hidden', since that's a new concept in D7.

dww’s picture

dww
we/he/they
CreditAttribution: dww commented
FileSize
669554-14.update_filter_project_info_no_hidden.patch576 bytes

Oh, and now that I think about it, we don't need to cache 'hidden' in D7, either, since we already ignored modules that are 'hidden' by the time we get here. So, here's a trivial patch for HEAD to remove that from the whitelist...

dww’s picture

dww
we/he/they
CreditAttribution: dww commented
Version: 6.x-dev » 7.x-dev

whoops, #14 is for D7 -- changing version for the bot...

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented
Status: Needs review » Reviewed & tested by the community
webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Version: 7.x-dev » 6.x-dev
Status: Reviewed & tested by the community » Needs review

Committed to HEAD. Back to 6.x.

dww’s picture

dww
we/he/they
CreditAttribution: dww commented
Status: Needs review » Reviewed & tested by the community

#13 is RTBC for D6. It's identical to what's in HEAD now except it applies cleanly to that branch. ;)

mikejonesok’s picture

mikejonesok CreditAttribution: mikejonesok commented

Thanks for the patch. It helps.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Reviewed & tested by the community » Fixed

Thanks, committed to Drupal 6 too.

dww’s picture

dww
we/he/they
CreditAttribution: dww commented

Yay, thanks!

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented

Grm, this made Drupal 6.16 incompatible with PHP 4, see #732096: Fatal error: Call to undefined function: array_intersect_key() in url/modules/update/update.compare.inc on line 695.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Fixed » Needs work
webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Needs work » Fixed

Oops. I see that other patch is RTBC.

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented
Status: Fixed » Reviewed & tested by the community

? :)

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented
Status: Reviewed & tested by the community » Fixed

Err...wow. I got lost between the two issues.

Status: Fixed » Closed (fixed)

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