Problem/Motivation
These are the components of the administration menu subtree rendering flow
Subtrees toolbar menu item
/**
* Implements hook_menu().
*/
function toolbar_menu() {
$items['toolbar/subtrees/%'] = array(
'page callback' => 'toolbar_subtrees_jsonp',
'page arguments' => array(2),
'access callback' => '_toolbar_subtrees_access',
'access arguments' => array(2),
'type' => MENU_CALLBACK,
);
return $items;
}
Inject an inlined JS call to the subtree item retrieval menu item when the page renders.
// To conserve bandwidth, we only include the top-level links in the HTML.
// The subtrees are included in a JSONP script, cached by the browser. Here we
// add that JSONP script. We add it as an external script, because it's a
// Drupal path, not a file available via a stream wrapper.
// @see toolbar_subtrees_jsonp()
$menu['toolbar_administration']['#attached']['js'][url('toolbar/subtrees/' . _toolbar_get_subtree_hash())] = array('type' => 'external');
Retrieve the subtrees through a client-side AJAX call and return as JSON
/**
* Page callback: Returns the rendered subtree of each top-level toolbar link.
*
* @see toolbar_menu().
*/
function toolbar_subtrees_jsonp($hash) {
_toolbar_initialize_page_cache();
$subtrees = toolbar_get_rendered_subtrees();
$response = new JsonResponse($subtrees);
$response->setCallback('Drupal.toolbar.setSubtrees');
return $response;
}
The JS callback for the AJAX call is defined by toolbar.js under the Drupal object so it is available on the page to be invoked.
/**
* Set subtrees.
*
* JSONP callback.
* @see toolbar_subtrees_jsonp().
*/
Drupal.toolbar.setSubtrees = function(subtrees) {
Drupal.toolbar.subtrees = subtrees;
};
If, at the time that the attach method of Drupal.behaviors.toolbar
is invoked, the AJAX call to get the subtrees has returned and populated Drupal.toolbar.subtrees
, then render the HTML to the DOM.
// Add subtrees.
// @todo Optimize this to delay adding each subtree to the DOM until it is
// needed; however, take into account screen readers for determining
// when the DOM elements are needed.
if (Drupal.toolbar.subtrees) {
for (var id in Drupal.toolbar.subtrees) {
$('#toolbar-link-' + id).after(Drupal.toolbar.subtrees[id]);
}
}
The code flow above was always meant to be temporary improvement to simply hard-coding the submenu items into the HTML on the initial page request. But the flow can be further refined so that sub-menus items are retrieved on a needs basis, when the menu tree parent for the sub items is toggled open, rather than simply on page load.
Proposed resolution
(description of the proposed solution, the rationale behind it, and workarounds for people who cannot use the patch)
Remaining tasks
Propose the changes in a patch.
User interface changes
Loading menu sub items may incur a spinner as the AJAX request is processed.
API changes
None.
Related tasks
#1605290: Enable entity render caching with cache tag support
#2044583: Add EntityChangedInterface to allow entities with "changed" field to be properly cached
#2005644: Use client-side cache tags & caching to eliminate 1 HTTP requests/page for in-place editing metadata, introduce drupalSettings.user.permissionsHash
Comment | File | Size | Author |
---|---|---|---|
#45 | toolbar-menu-perf-1927174-45.patch | 32.16 KB | Wim Leers |
#45 | interdiff.txt | 913 bytes | Wim Leers |
#42 | toolbar-menu-perf-1927174-42.patch | 31.84 KB | jessebeach |
#42 | interdiff.txt | 5.44 KB | jessebeach |
#40 | interdiff.txt | 8.11 KB | jessebeach |
Comments
Comment #1
nod_The whole subtree in toolbar is a real hit on frontend performance. We can't ship with the current behavior, it's too slow on mobile.
Comment #2
catchIsn't this #1805054: Cache localized, access filtered, URL resolved, and rendered menu trees? If possible it'd be great to apply the same pattern to menu blocks as well - those are currently expensive to render and very inefficiently cached.
Comment #3
nod_The script loading the subtree needs the defer attribute at least. It currently takes 400ms to render the sub menus on desktop.
Simplifying the JS processing would go a long way to make that faster as well but I'd be happy with a setting to turn off expanding menus for now (that would be even better for performance. I never use it and I'm still paying the price for it).
Comment #4
jcisio CreditAttribution: jcisio commentedI haven't benchmark, but isn't the last part the bottle neck?
Could we do that in a fragment instead of changing 1000 times directly in the document?
Comment #5
nod_It's code in toolbar.menu.js that is dead slow, the piece of code you're showing takes like 3ms.
Check out the other file, there is
$menu.find('li > a').wrap('<div class="box">');
and that pretty much takes 200ms by itself.Comment #6
nod_at least.
Comment #7
tstoecklerThat sound's like a good interim step. If we don't make it to fix the actual performance problem, at least we can make the site render a bit faster.
Comment #8
Wim Leers1) I think the patch in #6 should be in a separate issue, to keep this issue "the big issue to solve".
2) Shouldn't we ensure that that happens *after*
toolbar.js
has been executed? I.e. use dependencies?Comment #9
webchickTagging.
Comment #10
Gábor HojtsyI tested two sites with #6 applied and not on the same machine. I did not notice any difference with Chrome's network stats neither on onload or DOMContentLoaded. Not sure where/how I could measure the effectiveness of it.
Comment #11
Gábor HojtsyI looked at the menu tree rendering quite a bit. Some problems I encountered / things I noted:
P1. The code does a great deal of processing to figure out a hash for the menu tree + language combo and then includes that hash in the URL to load the menu tree, BUT then it ignores that hash and loads/renders the menu anyway (the hash counting is only an issue on cold caches, but it makes the menu generation run twice for such pages).
P2. There is no clearing of this hash cache directly. But does not really matter since the hash is not used anyway.
P3. The whole subtree for all menu items is indeed added to the page. The effect of this is:
Representative values with menu subtree: 131 requests ❘ 71.6 KB transferred ❘ 2.40 s (onload: 1.30 s, DOMContentLoaded: 1.09 s)
Without menu subtree: 132 requests ❘ 91.6 KB transferred ❘ 2.78 s (onload: 1.64 s, DOMContentLoaded: 1.58 s)
So one less request, 20k less data loaded, 0.5s less to get to DOMContentLoaded. without the full menu. The default display of the (high level) menu is the same of course without the subtrees.
P4. The full menu is loaded even if you never go out of the horizontal mode of the menu, even though there is no way to see that menu. Maybe it still makes sense to load the subtree for accessibility still in this case though?
---------
Suggestions:
S1. I think we can get rid of the hash counting since we don't use it anyway. Attached patch for that.
S2. We should look at loading the subtree only later on a user action. I think the user action would be
- clicking a (top) menu item if in vertical mode
- a (top) menu item getting into focus regardless of horizontal or vertical mode (for accessibility and keyboard navigation in general)
This should help make the initial load happen that much faster as demonstrated above, while still making the full menu available to browse as needed. It also would not make all the menu load in vertical mode for sighted users (if we can separate the keyboard focus action from mouse click initiated focus).
I tried to decipher how the JS works and I think I mostly understand it. Some things changed since the OP and it is now using backbone models to encapsulate some of the logic and $.Deferred(). I think I would know what to *remove* to not make the subtree load, but then wiring it up on the click handler may be too challenging for me. I think:
- we'd need a global state about whether the subtree was loaded or not
- on click of the items (and considering above conditions), load the subtree and delay the rest of the click handling until loaded
- I did not find focus / accessibility handling, but that would need to be wired up to this too
Attached patch only to speed this up for cold caches by not generating a hash for nothing :) It does not include @nod_'s patch because based on the goals of this issue, the subtree will not be loaded (and therefore no need to defer) on page load. It will need further user action.
Comment #12
Gábor HojtsyBTW I'm not sure per item subtrees are worth a try at first. Either way, the onclick load of subtrees need to be implemented, so if loading the huge whole subtree is too costly/slow for that case, we can look at slicing and dicing the subtree then.
Also we don't cache toolbar_get_rendered_subtrees() at all, so much of the loading time is that not being cached which as catch pointed out above is in #1805054: Cache localized, access filtered, URL resolved, and rendered menu trees. But half of the time saved seems to be on the frontend based on the above numbers if I interpret them right (comparing the subtree-less and subtree-inclusive numbers).
Comment #13
Wim LeersGreat detective work, Gábor!
Notes for client-side caching of the menu tree:
Comment #14
Gábor HojtsyI posted the numbers backwards in #11 as the num of requests clearly shows. Instead of:
It should be:
Representative values without menu subtree: 131 requests ❘ 71.6 KB transferred ❘ 2.40 s (onload: 1.30 s, DOMContentLoaded: 1.09 s)
*With menu subtree*: 132 requests ❘ 91.6 KB transferred ❘ 2.78 s (onload: 1.64 s, DOMContentLoaded: 1.58 s)
Shows the difference much better :)
Comment #15
Gábor Hojtsy@wim: yeah, #1805054: Cache localized, access filtered, URL resolved, and rendered menu trees implements / suggests server side caching for menu trees; at least we can tell on the server side when menus change; however it is true that even short time client caching of the menu tree may be a massive performance improvement. I think this should come after we eliminate the tree unless really needed, which regardless of caching will be a *massive* improvement in the frontend as well based on the above numbers :)
Comment #16
Wim LeersYes, sorry for being off-topic. Stupid me :/ Wim--
Comment #17
Gábor HojtsyHeh, no client side caching would help too here I think. So many ways to improve. I think loading the tree later is the root of almost all of those. The way we load it now that is not even possible to client side cache because we call back events from that load, so it is not cacheable as data. Once we refactor that to load it as data separate, it will be client cache-able. (It is actually possible to improve server side caching of it in the meantime, if that helps. I'll see if working on that helps in a significant way).
Comment #18
Wim LeersExactly! :)
Comment #18.0
jessebeach CreditAttribution: jessebeach commentedadded related tasks
Comment #19
Gábor HojtsyNote that #2077279: Only load the admin menu subtrees if the toolbar tray is oriented vertically; only request subtrees when local cache is stale totally invalidates #11 above as it starts to use the hash key for local caching.
Comment #20
jessebeach CreditAttribution: jessebeach commentedWe've gone through several iterations on a focused solution to this critical in #2077279: Only load the admin menu subtrees if the toolbar tray is oriented vertically; only request subtrees when local cache is stale. Although I do not want to forgo a general solution to menu caching, the Toolbar case is sufficiently unique that a targeted solution makes sense in order to achieve a performance improvement.
So, I'm moving the patch from #2077279 to this queue and closing that issue. Given a general solution in the future, we might remove this code from Toolbar and favor it, but for now, we have a way to improve page composition and rendering time for authenticated users.
Comment #21
Gábor HojtsyI think this patch is great. Further reviews would be great of course. :)
Comment #22
jessebeach CreditAttribution: jessebeach commentednod_ made the following comment in chat.
Rendering the submenu items (when they're drawn from localStorage), even without the AJAX request, still imposes processing time on the page rendering. This patch delays rendering, from localStorage or the AJAX request, until the admin menu is positioned vertically within the toolbar structure.
I corrected some minor style guide issues and comment wording the Wim Leers pointed out in chat.
I also removed the following function which it turns out, isn't ever invoked:
Comment #24
Wim LeersIt does not make sense to me that #22 failed testing; the patch only touches Toolbar module's code, but the failed test is for the
admin/people
route's content. Retesting.Comment #25
Wim Leers#22: toolbar-menu-perf-1927174-22.patch queued for re-testing.
Comment #26
Wim Leerss/position/orientation/
Fixed in attached reroll.
This is unnecessarily complex/vague. Why not just use
model.changed.orientation === 'vertical'
?Fixed in attached reroll.
drupalSettings.toolbar.subtreesPath
is overkill, we haveDrupal.url()
, so we can generate the desired URL on the client-side, we only need to pass the hash itself. This also allows for a slight simplification in the test coverage, much clearer documentation where this JS setting is being added, less user-specific bytes to be passed, and the localStorage cache to be used somewhat more efficiently. Fixed: now there isdrupalSettings.toolbar.subtreesHash
._toolbar_get_subtree_hash()
is a misnomer, the hash is for *multiple* subtrees, as the existing function documentation already indicates, and as is indicated everywhere else in the code. So: renamed it to_toolbar_get_subtrees_hash()
. This is a private API function, so fine to change.That's it. I cleaned up some comments, renamed one function, made one if-test more explicit and moved the generation of the "subtrees URL" from the server side to the client-side, which happens to have as a side effect that less data needs to be served and cached.
The test coverage is extensive, but it's good to err on the side of too much rather than too little in a crucial feature like this one.
If jessebeach agrees with these changes, I'll RTBC.
I stepped through the code, did manual testing, and reviewed it until I grokked every bit of the code. It's going to make front-end performance on mobile devices *much* better. It's going to make each page-with-toolbar load faster. Faster than current D8 HEAD of course, but also faster than D7, because less data needs to be sent down the wire, less stuff needs to be generated, at a cost of only a tiny cache entry per toolbar user.
Great work! :)
Comment #27
jessebeach CreditAttribution: jessebeach commentedI like the changes in #26.
And if I could spell the word assert (not asert!), the interdiff would be much much smaller.
I retested everything manually and all behaviors remain unchanged.
Comment #28
jessebeach CreditAttribution: jessebeach commentedI'm assigning this to catch. I'd like him to weigh in before we commit. He's been involved with the performance issues for Toolbar and his input has been very informative.
Comment #29
Wim LeersI'm marking RTBC in any case. This is ready for committer review. If catch likes what he sees and agrees with me, then he can just commit it :)
Comment #30
alexpott#2074037: Add drupalPostUrl() — drupalPost()/drupalPostAjax() are for forms only, D8 JS performs non-form HTTP requests has changed drupalPost to drupalPostForm
Comment #31
Wim Leerss/drupalPost()/drupalPostForm()/
Comment #32
jessebeach CreditAttribution: jessebeach commentedWim is taking the approach to other patches we're working on and he noted over email that changing a translation will also require a subtrees menu cache clear.
So we need to listen to
hook_language_update
as well.Demoting to needs work and adding a case for language and tests as well.
Comment #33
catchOverall this looks fine. It'd be nice if we could lose the AJAX request when the toolbar is in vertical orientation as well - smaller screen sizes are likely to be those that also struggle rendering and/or more likely to be on 3g, but I don't see a way to do that when the theming requires knowledge of whether there's child menu items or not.
Is hook_language_update() actually fired when translations are updated? Or just when the language itself gets updated?
Comment #34
jessebeach CreditAttribution: jessebeach commentedThis will most likely be fired once and then rarely again (only after a cache clear). It's only sent for authenticated, admin-ish users as well. Right now we fire the AJAX request on every admin page load.
No, the language entity does not change :/
Drupal\locale\Form\TranslateEditForm
clears its own caches if strings are updated:But it does not provide any type of notification or hook that this happened. Maybe we can introduce one?
Comment #35
jessebeach CreditAttribution: jessebeach commentedWait, can't we just tag the toolbar caches with 'locale' as well? Isn't that what these tags are for?
Comment #36
jessebeach CreditAttribution: jessebeach commentedNo, that wouldn't work because the tag deletions happen within the context of a bin. Locale uses the default 'bootstrap' bin. Toolbar uses its custom 'toolbar' bin. We could switch Toolbar to the bootstrap bin.
Comment #37
Wim Leers#33: I'm very confused by your comment.
That implies embedding the entirety of the toolbar menu in the HTML. Implications:
localStorage
is present on every browser we support (and more: IE8 too).All the more reason to use client-side caching: only the first page load will be slower! Afterwards, the subtrees are cached. Zero bytes on the network for the admin menu tree, besides the few for the
subtreesHash
.… maybe you mean that ideally, we'd only do this AJAX request if the user actually wants to open one of the subtrees?
Comment #38
catch@jessebeach: Tag deletions don't happen within the context of a bin, they're global. #918538: Decouple cache tags from cache bins cleans that up a bit but it's the case at the moment too. So if the locale tag gets cleared when a translation is added, that's exactly what should get used, and that's why cache tags are great - no hook implementation to worry about. That reminds me the hook_user_*() implementations could go once #1605290: Enable entity render caching with cache tag support is in since that tag will be cleared anyway then.
@WimLeers - Yes I meant ideally do the request when opening one of the trees, apparently I typed the whole comment without actually mentioning that.... I know we only fire the request when it's not in localStorage, but the subtrees have to be loaded into the DOM on every page regardless of whether the subtree is actually clicked no?
Comment #39
jessebeach CreditAttribution: jessebeach commentedThis is doable. I would render a disclosure toggle on top-level items with child menus. We'd hold off fetching/rendering child menus until the disclosure is toggled.
Comment #40
jessebeach CreditAttribution: jessebeach commentedI've added support for clearing the Toolbar user cache through the
locale
cache tag. A test has been added to ensure that the subtrees hash is cleared appropriately. I've provided two versions of the patch, both with the same test. The fail version lacks the locale cache tag and should fail.I spent a bit of time experimenting with how we could defer the rendering of the subtree items further. Deferring their rendering until a user clicks on the toggle of a top level menu item turns out to be really messy. It requires going into the menu rendering pipeline and adding additional data to the menu link DOM elements.
I opted for a strategy that's a little less aggressive, but it gives us some boost. The submenu items are now only rendered when the Menu tab in the Toolbar is the active tab -- either on page load or when the user clicks the tab. This way, if you never click the tab, then the subtrees will never be inserted into the DOM and the rendering hit never applied. I hope this can split the difference. Optimizing on a per-item or per-tree basis will get messy and really, that should be a generic optimization, not just something in the Toolbar module.
Comment #41
Wim LeersloadSubtrees()
used to *always* load the subtrees (unless they're already being loaded).Now, they contain the conditionals that used to be in the calling code. Why can't they be in the calling code?
Test coverage looks solid :)
Why do we need this? Is there any reason why we wouldn't want the loading of the admin menu subtrees to be deferred?
It seems this is only used to determine that this is the *menu* tab? If so, then why not just add a class? Because right now, it seems like you could set the value of this attribute to
false
, which would cause the subtrees loading logic to *always* fire, even if the menu tab isn't active. But that's not how it works."if when"
Comment #42
jessebeach CreditAttribution: jessebeach commentedloadSubtrees()
is invoked currently invoked from two contexts.loadSubtrees()
has three conditions that must be met in order to get and render the subtrees. The invocation of this method might have conditions as well, as if the case with the ToolbarVisualView. Each condition is unique in the examples above.I need some kind of marker on the admin menu tab to identify it. You're right that the name of the attribute suggests a Boolean. I've updated it. I didn't want to add a class because I'm trying to avoid mixing styling elements with an addition that is purely behavioral in this case. I've changed the name of the attribute to:
And added a ton more comments to explain what this attribute does.
fixed.
Comment #44
Wim Leers#42: toolbar-menu-perf-1927174-42.patch queued for re-testing.
Comment #45
Wim LeersThe added documentation in #42 will help people reading the code significantly. Thanks.
I manually tested again and indeed the subtrees are now only loaded when the admin menu is opened and in vertical orientation. Hence also the request to fill the client-side cache (if that cache is still cold) is only made in that situation.
Test coverage to ensure the hash changes whenever a translation is changed is solid.
Hence: RTBC.
Patch no longer applied in one hunk due to #2053489: Standardize on \Drupal throughout core. Straight reroll. Plus one tiny doc fix (s/data marker/data attribute/).
Comment #46
webchickBack to catch.
Comment #47
catchOK this looks good now. It'd be good to keep an eye on this when it's possible to load individual subtrees, but this means less HTTP requests and a much smaller DOM for the majority of requests now.
Committed/pushed to 8.x, thanks!
Comment #48
Wim LeersHurray! :)
Comment #49.0
(not verified) CreditAttribution: commentedadded 2005644