I was doing a performance audit on a website and noticed that panels_load_displays() was being called for a bunch of dids that were not relevant to the pages in question (see attached screenshot). As I traversed the code, I noticed that panels_mini_load_all() was getting loaded on every page load because it was called in panels_mini_block_list_alter().
This line inside panels_mini_load_all() is where every single did gets loaded: http://cgit.drupalcode.org/panels/tree/panels_mini/panels_mini.module#n347
$displays = panels_load_displays(array_keys($dids));
But I'm wondering if loading panels_mini_load_all() inside of panels_mini_block_list_alter() makes sense to begin with:
http://cgit.drupalcode.org/panels/tree/panels_mini/panels_mini.module#n152
function panels_mini_block_list_alter(&$blocks) {
if (module_exists('page_manager')) {
$current_page = page_manager_get_current_page();
}
// Load add at once to save time.
panels_mini_load_all();
foreach ($blocks as $key => $block) {
if ($block->module != 'panels_mini') {
// This block was added by a contrib module, leave it in the list.
continue;
}
$panel_mini = panels_mini_load($block->delta);
I'd think that $panel_mini = panels_mini_load($block->delta); being loaded only when necessary, makes more sense from a performance perspective than preloading all mini panels on every page load, but seeing that this code was intentionally added to "save time", I figured I'd leave it up for discussion instead of just creating a patch for now.
| Comment | File | Size | Author |
|---|---|---|---|
| #4 | panels-mini-panels-block-list-alter-2826395-4.patch | 2.86 KB | WidgetsBurritos |
| Screen Shot 2016-11-10 at 10.34.02 AM.png | 477.41 KB | WidgetsBurritos |
Comments
Comment #2
WidgetsBurritos commentedActually I just realized I misread the code. Just removing
panels_mini_load_all()isn't sufficient to solve the problem because all that subsequent code will still get loaded for every mini panel. So havingpanels_mini_load_all()is technically an optimization, but if all this is trying to do is determine context, then perhaps that can be evaluated differently than preloading every single mini panel? Or at least cache the context requirements or something?Comment #3
WidgetsBurritos commentedI'm proposing we do something like I've done in this patch, where we cache the
hook_block_list_alter()changes. Thoughts?Comment #4
WidgetsBurritos commentedI've updated this patch a bit more to cache per block instead of caching all blocks cumulatively (as this could negatively affect non-panels blocks as well).
While I'm not convinced it solves 100% of the scenarios (mainly due to the unsets), I definitely think it's a step in the right direction and would appreciate further feedback.
Comment #5
WidgetsBurritos commentedComment #6
joelpittetThis looks good, I wonder if the caching layer is needed?
Comment #7
joelpittetThanks @WidgetsBurritos, I've committed this to the latest dev branch as we've been using it for some time now. Subtle tweaks on reroll and commit.
Comment #10
joelpittetComment #11
sgdev commentedA note for anyone using Mini Panels IPE, this patch breaks the module. The
panels_mini_load_all()function needs to run when Mini Panels IPE is in edit mode, and the module was leveraging this hook alter to ensure all panes were available.A new patch has been created for Mini Panels IPE that accomplishes this. It limits the call to when a user is in Mini Panels IPE mode, rather than loading on all pages. It is available here for review: https://www.drupal.org/project/panels_mini_ipe/issues/3331793