When looking at Devel's query log, you see that the cache_set() query that caches the entire schema contains all 'description' keys for tables and all fields in tables.

The schema cache is loaded on every single request, so it's needlessly larger than it has to be, and unserialize() has lots of additional work to do.

We only ever need the descriptions when installing a schema. Other stuff (like Schema module) can easily cache work around the missing descriptions if it needs to, but that shouldn't slow down regular performance of Drupal core.

CommentFileSizeAuthor
#11 drupal.schema-cache-description.11.patch4.21 KBsun
#5 drupal.schema-cache-description.4.patch4.21 KBsun
drupal.schema-cache-description.0.patch4.17 KBsun

Comments

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

Note that #910190: Entire schema cache needlessly loaded on all pages is closely related to this issue. It avoids us from loading the entire schema cache on every page. However, regardless of that, I think that this patch would nevertheless be good to do.

moshe weitzman’s picture

moshe weitzman commented
Status: Needs review » Reviewed & tested by the community

Agreed. Code looks good. Bot is happy.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

Although my local box is totally not suitable for doing benchmarks, I've mass-applied the following range of current performance patches and am getting a nice boost:

#910190: Entire schema cache needlessly loaded on all pages
#910156: Remove 'description' keys from schema cache
#606966: 'load arguments' of parent path are not inherited
#910124: Clean up menu_contextual_links()

BEFORE patches:
Executed 36 queries in 25.18 ms.
Page execution time was 481.48 ms.
Memory used at: devel_boot()=2.5 MB, devel_shutdown()=16.23 MB, PHP peak=16.75 MB.

AFTER patches:
Executed 35 queries in 15.99 ms.
Page execution time was 471.18 ms.
Memory used at: devel_boot()=1.95 MB, devel_shutdown()=15.85 MB, PHP peak=16.5 MB.

Test page: admin/config/development/devel; Garland; enabled Navigation menu block containing 1 taxonomy term link and 1 node link.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Needs work 
+ * @param $clean
+ *   (optional) Whether to additionally remove 'description' keys of all tables
+ *   and fields (TRUE) to improve performance of serialize() and unserialize().
+ *   Defaults to FALSE.

Why is that parameter called $clean when it is a flag to indicate whether or not to load descriptions? Shouldn't it be $load_descriptions?

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs work » Needs review
StatusFileSize
new drupal.schema-cache-description.4.patch4.21 KB

Renamed $clean to $remove_descriptions as requested.

Status: Needs review » Needs work

The last submitted patch, drupal.schema-cache-description.4.patch, failed testing.

moshe weitzman’s picture

moshe weitzman commented
Status: Needs work » Reviewed & tested by the community

nice one, bot.

dries’s picture

dries commented
Status: Reviewed & tested by the community » Needs review

Any particular reason why we want to make this configurable? Is there a scenario where we never load the description?

At a minimum, we should consider not loading the descriptions by default. Better to lead with best practices, IMO.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

@Dries: This is what already happens in the patch. We still need to keep the descriptions when preparing schema for adding new tables or fields. By default, the descriptions are kept. Only the schema cache doesn't need them, so it passes FALSE. That, or I didn't understand your comment. ;)

dries’s picture

dries commented 
+++ includes/common.inc	16 Sep 2010 01:49:15 -0000
@@ -6075,20 +6075,30 @@ function drupal_get_schema_unprocessed($
+function _drupal_schema_initialize(&$schema, $module, $remove_descriptions = FALSE) {

I was suggesting that $remove_descriptions defaults to TRUE.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
StatusFileSize
new drupal.schema-cache-description.11.patch4.21 KB

Alright then. Flipped FALSE to TRUE.

moshe weitzman’s picture

moshe weitzman commented
Status: Needs review » Reviewed & tested by the community

Yeah, that’s better.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

#11: drupal.schema-cache-description.11.patch queued for re-testing.

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

Status: Fixed » Closed (fixed)
Issue tags: -Performance

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

Issue tags: +Performance

Restoring issue tags, see #2125755: System messages removed all issue tags during D7 upgrade.