Problem/Motivation

zurb_foundation_theme_suggestions_alter checks the visible blocks. However, the data is not statically cached. It causes a huge performance hit.

function zurb_foundation_theme_suggestions_alter(&$suggestions, $variables, $hook) {
  if ($hook == 'menu' || $hook == 'block') {
    // For menu blocks, check to see if the menu block is placed in an off-canvas
    // region, and if so use our custom template.
    $all_blocks = \Drupal::service('block.repository')->getVisibleBlocksPerRegion();

It ends up calling getVisibleBlocksPerRegion 41 times. And that method does not statically cache results. We could fix core, but easier here!

Here is a timeline view, each of the green blocks in that row is a call to fetch the visible blocks.

Proposed resolution

Statically cache the visible block results to reduce the number of times all blocks are loaded.

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

mglaman created an issue. See original summary.

mglaman’s picture

Issue summary: View changes
StatusFileSize
new39.47 KB

ericchew made their first commit to this issue’s fork.

ericchew’s picture

Status: Active » Needs review

Rebased to fix merge issue.

frob’s picture

@mglaman can you post a profile with your fix implemented. If I am reading this right then your fix should only remove 5 calls to getVisibleBlocksPerRegion.

mglaman’s picture

I'll try to find it. But I'm not sure of "only" means it is isn't significant, it definitely had a large impact on performance.

frob’s picture

It might or it might not. Should depend on the number of blocks set in the block configuration. Also this cache seems redundant with the page cache.

mglaman’s picture

Page cache doesn't apply for authenticated users, and most Drupal Commerce sites cannot use page cache. The called method does not implement any caching layers as explained in https://www.drupal.org/project/drupal/issues/2479459. So callers must prevent multiple invocations in one request.

frob’s picture

All that makes sense and I don't see any harm in adding it, but it feels like a band-aid over a bullet wound to put this in the a theme. A better solution would be to extend and override the service in a module while we wait for core to close that 7 year old issue.

Okay, now that I have gotten the comedy out of the way, I see no harm in this. I have reviewed it but we should add a comment about removing the extra cache layer after #2479459: BlockRepository::getVisibleBlocksPerRegion() does an uncached entity query on every request closes.

sim_1 made their first commit to this issue’s fork.

  • sim_1 committed 2a84a88 on 8.x-6.x authored by mglaman
    Issue #3249710 by mglaman, frob, ericchew: hook_theme_suggestions_alter...
sim_1’s picture

Status: Needs review » Fixed

Great, thank you for this patch and review! merged and setting the issue to fixed.

Status: Fixed » Closed (fixed)

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