Caching results of system_region_list

I just ran into the same issue, and arrived at a similar patch; however, I was just thinking - would it be better to patch system_region_list() itself? That way we could cache for both REGIONS_ALL and REGIONS_VISIBLE while iterating over the list just once.

Something like this, perhaps.

Sounds like this could improve the performance of the the core function, but could this be over-engineering the solution since it adds more complexity with a new layer of caching? It also sounds like it doesn't address what could be an inefficiency in the context module - does that module need to make 100 calls to this function in the first place?

Well no, it doesn't *need* to in the sense that context could cache the result itself, as joelcollinsdc originally proposed. But putting the caching in the core function improves the performance regardless of the caller. It's called three times on a normal page view in vanilla core anyway.

I think the performance of that first call can be improved, too.

@joelcollinsdc - maybe you could open up a new issue for the patch to the context module. It seems to me patching context would definitely be helfpul, but might be worth looking into why all the calls are being repeated in the first place and addressing there.

@John Morahan - the original function returns an empty array if the theme_key isn't set, where as the patch doesn't handle that condition. I think something like else {return array();} at the end would do it. unless you meant to change this?

@technicalknockout the original function returns an empty array if $themes[$theme_key] isn't set (where $themes is the return value from list_themes()). The patch first checks if $list[$theme_key] is set (where $list is the static cache variable), then later it initializes $list[$theme_key] to a pair of empty arrays, checks if $themes[$theme_key] is set before trying to populate them, and finally returns one of them at the end. So the overall behaviour should be unchanged. Or have you observed otherwise?

yes, I see it initializes two empty arrays for keys REGIONS_ALL & REGIONS_VISIBLE, but there is nothing that restricts $theme_key from being passed in as something like $theme_key = 'nonsense'. In this case, the check <?php if (isset($themes[$theme_key])) ?> returns false and $list[$theme_key] remains unset. At the end the code still will attempt to reach into the uninitialized $list[$theme_key][$show]

Nope. $list[$theme_key] is initialized before isset($themes[$theme_key]) is checked:

+    $list[$theme_key] = array(REGIONS_ALL => array(), REGIONS_VISIBLE => array());
+    $themes = list_themes();
+    if (isset($themes[$theme_key])) {

It would indeed fail if $show was something other than REGIONS_ALL or REGIONS_VISIBLE, but as the API doc explicitly states "Possible values: REGIONS_ALL or REGIONS_VISIBLE", I don't think this case is worth worrying about.

Sure, it's documented, but it is not enforced and the current behavior is to return an empty array. Maybe it's unlikely, but this change could cause a disruption for existing sites if for some reason they weren't following the API documentation. Is there an advantage to changing how this edge condition is handled currently?

Fair point. However, if people are happy in principle with addressing this in core, it would have to go into D8 first anyway, in which case it hardly matters how consistently we handle misuse of the API (though worth keeping in mind for a backport, obviously).

Also I see drupal_flush_all_caches() calls drupal_static_reset() in D8, so the explicit resets should not be necessary there.

I'll reroll it later.

another point just thinking more about it (still in reference to D7, haven't looked at D8 at all on this yet) - this function does really little work. The work is done by list_themes and it caches its results internally already. Did you see a performance gain before & after the patch?

Yes, though obviously it's more significant when the context module is involved (see joelcollinsdc's 30ms improvement right at the top; I got even better results), there is a slight improvement even with just core (total inclusive time for that function according to xdebug is 0.997ms before, 0.435ms after).

d8 version, untested

I just bumped up against the 2000 t's issue in my own xhprofs, searched in the context queue, found this: https://drupal.org/node/1688330

Definitely the right way to fix this is a drupal static inside of the system_region_list function.

The current behavior of system_region_list is to respond with all regions if there is no second argument, or if the second argument is "REGIONS_ALL". Any other second argument passed to system_region_list is going to behave as if "REGIONS_VISIBLE" was passed. So, to address @technicalknockouts concern, you could simply return

if ($show == REGIONS_ALL) {
  return $list[$theme_key][REGIONS_ALL];
}  else {
  return $list[$theme_key][REGIONS_VISIBLE];
}

Ideally this wouldn't be an argument dependent function at all and ought to be factored into two separate functions that each cache their static results like system_visible_region_list($theme_key) and system_region_list($theme_key);

To keep a backwards compatible API you could still allow for system_region_list to forward to system_visible_region_list if anything other than REGIONS_ALL is set for $show.

There has been no activity here for 9 years.

Is this still a problem?

If you are experiencing this problem on a supported version of Drupal reopen the issue, by setting the status to 'Active', and comment on what still needs to be done.

Thanks!

Postponed as per #36

Since there's been no follow up going to close out. If still a valid bug in D11 please re-open

Thanks all