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.
Comments
Comment #3
mglamanComment #5
ericchew commentedRebased to fix merge issue.
Comment #6
frob@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.
Comment #7
mglamanI'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.
Comment #8
frobIt 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.
Comment #9
mglamanPage 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.
Comment #10
frobAll 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.
Comment #13
sim_1Great, thank you for this patch and review! merged and setting the issue to fixed.