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

Files: 
CommentFileSizeAuthor
#45 toolbar-menu-perf-1927174-45.patch32.16 KBWim Leers
PASSED: [[SimpleTest]]: [MySQL] 59,737 pass(es).
[ View ]
#45 interdiff.txt913 bytesWim Leers
#42 toolbar-menu-perf-1927174-42.patch31.84 KBjessebeach
PASSED: [[SimpleTest]]: [MySQL] 59,857 pass(es).
[ View ]
#42 interdiff.txt5.44 KBjessebeach
#40 interdiff.txt8.11 KBjessebeach
#40 toolbar-menu-perf-1927174-40-fail.patch30.46 KBjessebeach
FAILED: [[SimpleTest]]: [MySQL] 59,690 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#40 toolbar-menu-perf-1927174-40.patch30.47 KBjessebeach
PASSED: [[SimpleTest]]: [MySQL] 59,373 pass(es).
[ View ]
#31 toolbar-menu-perf-1927174-31.patch25.2 KBWim Leers
PASSED: [[SimpleTest]]: [MySQL] 59,662 pass(es).
[ View ]
#26 toolbar-menu-perf-1927174-26.patch25.18 KBWim Leers
PASSED: [[SimpleTest]]: [MySQL] 59,416 pass(es).
[ View ]
#26 interdiff.txt15.26 KBWim Leers
#22 toolbar-menu-perf-1927174-22.patch24.58 KBjessebeach
PASSED: [[SimpleTest]]: [MySQL] 59,337 pass(es).
[ View ]
#22 interdiff.txt4.26 KBjessebeach
#20 delay-menu-subtrees-2077279-35.patch23.83 KBjessebeach
PASSED: [[SimpleTest]]: [MySQL] 59,309 pass(es).
[ View ]
#11 toolbar-speedup-cold-cache.patch1.77 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [MySQL] 57,672 pass(es).
[ View ]
#6 core-js-toolbar-defer-1927174-6.patch907 bytesnod_
PASSED: [[SimpleTest]]: [MySQL] 55,017 pass(es).
[ View ]

Comments

Priority:Normal» Critical

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.

Isn'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.

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).

I haven't benchmark, but isn't the last part the bottle neck?

for (var id in Drupal.toolbar.subtrees) {
  $('#toolbar-link-' + id).after(Drupal.toolbar.subtrees[id]);
}

Could we do that in a fragment instead of changing 1000 times directly in the document?

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.

Status:Active» Needs review
StatusFileSize
new907 bytes
PASSED: [[SimpleTest]]: [MySQL] 55,017 pass(es).
[ View ]

at least.

Status:Needs review» Reviewed & tested by the community

That 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.

Status:Reviewed & tested by the community» Needs review

1) 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?

Issue tags:+Spark

Tagging.

I 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.

Issue tags:+sprint
StatusFileSize
new1.77 KB
PASSED: [[SimpleTest]]: [MySQL] 57,672 pass(es).
[ View ]

I 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.

BTW 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).

Great detective work, Gábor!

Notes for client-side caching of the menu tree:

I posted the numbers backwards in #11 as the num of requests clearly shows. Instead of:

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)

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 :)

@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 :)

Yes, sorry for being off-topic. Stupid me :/ Wim--

Heh, 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).

Exactly! :)

Issue summary:View changes

added related tasks

Status:Needs review» Active

Note 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.

Status:Active» Needs review
StatusFileSize
new23.83 KB
PASSED: [[SimpleTest]]: [MySQL] 59,309 pass(es).
[ View ]

We'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.

I think this patch is great. Further reviews would be great of course. :)

StatusFileSize
new4.26 KB
new24.58 KB
PASSED: [[SimpleTest]]: [MySQL] 59,337 pass(es).
[ View ]

nod_ made the following comment in chat.

// Trigger an initial attempt to load menu sub-items. This first attempt
// is made after the media query handlers have had an opportunity to
// process. The toolbar starts in the vertical position by default, unless
// the viewport is wide enough to accommodate a horizontal orientation.
// Thus we give the Toolbar a chance to determine if it should be set
// to horizontal before attempting to load menu subtrees.
Drupal.toolbar.views.toolbarVisualView.loadSubtrees();"

When you have the tree cached and are on a horizontal toolbar, the subtrees are still added to the toolbar items.

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.

/**
* Calls the endpoint URI that will return rendered subtrees with JSONP.
*/
loadSubtrees: function () {
  if(!this.model.get('areSubtreesLoaded')) {
    var endpoint = drupalSettings.toolbar.subtreesPath;
    var cachedEndpoint = localStorage.getItem('Drupal.toolbar.subtreesPath');
    var cachedSubtrees = JSON.parse(localStorage.getItem('Drupal.toolbar.subtrees'));
    var isVertical = this.model.get('orientation') === 'vertical';
    // If we have the subtrees in localStorage and the endpoint url --
    // including the hash of the subtrees -- has not changed, then use
    // the cached data.
    if (isVertical && endpoint === cachedEndpoint && cachedSubtrees) {
      Drupal.toolbar.setSubtrees.resolve(cachedSubtrees);
      this.model.set('areSubtreesLoaded', true);
    }
    // Only make the call to get the subtrees if the orientation of the
    // toolbar is vertical.
    else if (isVertical) {
      // Remove the cached menu information.
      localStorage.removeItem('Drupal.toolbar.subtreesPath');
      localStorage.removeItem('Drupal.toolbar.subtrees');
      // The response from the server will call the resolve method of the
      // Drupal.toolbar.setSubtrees Promise.
      $.ajax(endpoint);
      // Cached the endpoint to the subtrees locally.
      localStorage.setItem('Drupal.toolbar.subtreesPath', endpoint);
      this.model.set('areSubtreesLoaded', true);
    }
  }
}

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:

/**
* Checks whether an item is in the active trail.
*
* Useful when using a menu generated by menu_tree_all_data() which does
* not set the 'in_active_trail' flag on items.
*
* @return
*   TRUE when path is in the active trail, FALSE if not.
*
* @todo
*   Look at migrating to a menu system level function.
*/
function toolbar_in_active_trail($path) {
  $active_paths = &drupal_static(__FUNCTION__);
  // Gather active paths.
  if (!isset($active_paths)) {
    $active_paths = array();
    $trail = menu_get_active_trail();
    foreach ($trail as $item) {
      if (!empty($item['href'])) {
        $active_paths[] = $item['href'];
      }
    }
  }
  return in_array($path, $active_paths);
}

Status:Needs review» Needs work

The last submitted patch, toolbar-menu-perf-1927174-22.patch, failed testing.

It 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.

Status:Needs work» Needs review

#22: toolbar-menu-perf-1927174-22.patch queued for re-testing.

StatusFileSize
new15.26 KB
new25.18 KB
PASSED: [[SimpleTest]]: [MySQL] 59,416 pass(es).
[ View ]
  1. +++ b/core/modules/toolbar/js/toolbar.js
    @@ -88,6 +89,14 @@ Drupal.behaviors.toolbar = {
    +      // process. The toolbar starts in the vertical position by default, unless

    s/position/orientation/

    Fixed in attached reroll.

  2. +++ b/core/modules/toolbar/js/toolbar.js
    @@ -316,10 +328,16 @@ Drupal.toolbar = {
    +      // Load the subtrees if the toolbar tray is in a vertical orientation.
    +      // There is no other attribute in the model with the value of vertical
    +      // that could be confused with the orientation attribute.
    +      if (value && value === 'vertical') {

    This is unnecessarily complex/vague. Why not just use model.changed.orientation === 'vertical'?

    Fixed in attached reroll.

  3. drupalSettings.toolbar.subtreesPath is overkill, we have Drupal.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 is drupalSettings.toolbar.subtreesHash.
  4. _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.
  5. Lots of small mistakes and language errors in the new (extensive!) test coverage. Fixed.

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! :)

I 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.

Assigned:Unassigned» catch

I'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.

Status:Needs review» Reviewed & tested by the community

I'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 :)

Status:Reviewed & tested by the community» Needs work

+++ b/core/modules/toolbar/lib/Drupal/toolbar/Tests/ToolbarAdminMenuTest.php
@@ -0,0 +1,381 @@
+    $this->drupalPost('admin/modules', $edit, t('Save configuration'));
...
+    $this->drupalPost('admin/modules', $edit, t('Save configuration'));
...
+    $this->drupalPost("admin/structure/menu/item/" . $link['mlid'] . "/edit", $edit, t('Save'));
...
+    $this->drupalPost('admin/people/permissions', $edit, t('Save permissions'));
...
+    $this->drupalPost('user/' . $this->admin_user->id() . '/edit', array("roles[$rid]" => $rid), t('Save'));
...
+    $this->drupalPost('admin/config/regional/language/add', $edit, 'Add language');
...
+    $this->drupalPost('user/' . $admin_user_id . '/edit', array("roles[$rid]" => $rid), t('Save'));

#2074037: Add drupalPostUrl() — drupalPost()/drupalPostAjax() are for forms only, D8 JS performs non-form HTTP requests has changed drupalPost to drupalPostForm

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new25.2 KB
PASSED: [[SimpleTest]]: [MySQL] 59,662 pass(es).
[ View ]

s/drupalPost()/drupalPostForm()/

Status:Reviewed & tested by the community» Needs work

Wim 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.

Overall 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?

It'd be nice if we could lose the AJAX request when the toolbar is in vertical orientation as well

This 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.

Is hook_language_update() actually fired when translations are updated? Or just when the language itself gets updated?

No, the language entity does not change :/

Drupal\locale\Form\TranslateEditForm clears its own caches if strings are updated:

if ($updated) {
  // Clear cache and force refresh of JavaScript translations.
  _locale_refresh_translations(array($langcode), $updated);
  _locale_refresh_configuration(array($langcode), $updated);
}

But it does not provide any type of notification or hook that this happened. Maybe we can introduce one?

Wait, can't we just tag the toolbar caches with 'locale' as well? Isn't that what these tags are for?

No, 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.

#33: I'm very confused by your comment.

It'd be nice if we could lose the AJAX request when the toolbar is in vertical orientation as well

That implies embedding the entirety of the toolbar menu in the HTML. Implications:

  1. Send more bytes (the same bytes!) on every page load that has the toolbar.
  2. Slower serving of every page load that has the toolbar.
  3. No client-side caching, even though localStorage is present on every browser we support (and more: IE8 too).

smaller screen sizes are likely to be those that also struggle rendering and/or more likely to be on 3g

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?

@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?

I meant ideally do the request when opening one of the trees

This 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.

Status:Needs work» Needs review
StatusFileSize
new30.47 KB
PASSED: [[SimpleTest]]: [MySQL] 59,373 pass(es).
[ View ]
new30.46 KB
FAILED: [[SimpleTest]]: [MySQL] 59,690 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new8.11 KB

I'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.

Assigned:catch» jessebeach
Status:Needs review» Needs work
  1. +++ b/core/modules/toolbar/js/toolbar.js
    @@ -334,7 +334,7 @@ Drupal.toolbar = {
    -      if (this.model.changed.orientation === 'vertical') {
    +      if (this.model.changed.orientation === 'vertical' || this.model.changed.activeTab) {
    @@ -508,7 +508,13 @@ Drupal.toolbar = {
    -      if (!this.model.get('areSubtreesLoaded')) {
    +      var $activeTab = $(this.model.get('activeTab'));
    +      var orientation = this.model.get('orientation');
    +      // Only load and render the admin menu subtrees if:
    +      //   (1) They have not been loaded yet.
    +      //   (2) The active tab is the administration menu tab.
    +      //   (3) The orientation of the tray is vertical.
    +      if (!this.model.get('areSubtreesLoaded') && $activeTab.data('drupal-defer-subtrees') !== undefined && orientation === 'vertical') {

    loadSubtrees() 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?

  2. +++ b/core/modules/toolbar/lib/Drupal/toolbar/Tests/ToolbarAdminMenuTest.php
    @@ -346,6 +347,96 @@ function testNonCurrentUserAccountUpdates() {
    +  function testLocaleTranslationSubtreesHashCacheClear() {

    Test coverage looks solid :)

  3. +++ b/core/modules/toolbar/toolbar.module
    @@ -463,6 +463,9 @@ function toolbar_toolbar() {
    +          // A marker that indicates to the client to defer loading of the admin
    +          // menu subtrees until this tab is activated.
    +          'data-drupal-defer-subtrees' => '',

    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.

  4. +++ b/core/modules/toolbar/toolbar.module
    @@ -624,7 +627,9 @@ function _toolbar_get_subtrees_hash() {
    +    // subtrees rendering if when string translations are made.

    "if when"

Status:Needs work» Needs review
StatusFileSize
new5.44 KB
new31.84 KB
PASSED: [[SimpleTest]]: [MySQL] 59,857 pass(es).
[ View ]

loadSubtrees() 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?

loadSubtrees() is invoked currently invoked from two contexts.

/* Drupal.behaviors.toolbar.attach()
  *
  * This is an unqualified invocation of loadSubtrees().
  */
Drupal.toolbar.views.toolbarVisualView.loadSubtrees();
/**
  * Drupal.toolbar.ToolbarVisualView::render()
  *
  * This is a qualified invocation because render is called for many reasons.
  * The toolbar starts in a vertical position, so the first call to render could potentially
  * result in the subtrees being loaded, even though the toolbar might eventually
  * end up in a horizontal position after all the media queries are processed. So
  * the invocation here must be conditional to one of two actions (1) changing the
  * orientation or changing the active tab.
  */
if (this.model.changed.orientation === 'vertical' || this.model.changed.activeTab) {
  this.loadSubtrees();
}

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.

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.

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:

data-drupal-subtrees

And added a ton more comments to explain what this attribute does.

"if when"

fixed.

Status:Needs review» Needs work
Issue tags:-sprint, -Spark

The last submitted patch, toolbar-menu-perf-1927174-42.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+sprint, +Spark

#42: toolbar-menu-perf-1927174-42.patch queued for re-testing.

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new913 bytes
new32.16 KB
PASSED: [[SimpleTest]]: [MySQL] 59,737 pass(es).
[ View ]

The 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/).

Assigned:jessebeach» catch

Back to catch.

Status:Reviewed & tested by the community» Fixed

OK 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!

Assigned:catch» jessebeach
Issue tags:-sprint

Hurray! :)

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

Issue summary:View changes

added 2005644