The navigation top bar is rendered directly in navigation_page_top() / the top_bar element, rather than via a #lazy_builder.
There are at least a couple of advantages to rendering it within a lazy builder:
1. Any cache tags or cache contexts from local task access checks won't reach the dynamic page cache (because they'll be isolated to the placeholder), this will mean higher cache hit rates for the dynamic page cache.
2. I found this via #3496369: Multiple load path aliases without the preload cache which relies on Fibers + placeholders to create a list of path aliases to load, which are then loaded all at once when returning to the first Fiber/placeholder. When the local tasks are in a placeholder (which currently happens either with the old toolbar, or for users without access to it), the links from those get picked up, which allows that to work better.
| Comment | File | Size | Author |
|---|
Show commandsStart 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:
changes
Comments
Comment #3
catchComment #4
catchDoesn't remove any database queries on its own. We'll probably need navigation module enabled in the Umami performance tests to see any of that, but it does make the render caching more efficient by moving the cache get to the CachedStrategy placeholder strategy which multiple render cache gets.
Comment #6
catchJust realised this probably doesn't make more impact on performance tests because:
1. The navigation performance test doesn't have any entities to load or path aliases etc.
2. Umami tests still use the toolbar and not navigation module.
Comment #7
berdirI guess one obvious reason that \Drupal\Tests\navigation\FunctionalJavascript\PerformanceTest doesn't see much of an impact is that it doesn't do anything in this scenario. We just look at the frontpage without any nodes. Top bar really only does anything at all when viewing an entity, where it builds for example the local tasks.
Might want to do some manual tests or extend the automated tests, but it seems more useful to switch umami (and standard) asap to navigation and then we can drop this custom test anyway.
Comment #8
catchWe need the last stable blocker from #3421969: [PLAN] New Navigation and Top Bar to replace Toolbar Roadmap: Path to Stable for that, but that one has been at review for some time now, so hopefully soon.
Comment #9
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #10
catchRebased.
Comment #11
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #12
catchBack to a green pipeline.
Comment #13
berdirI was wondering if we should wait until we can more easily check this with an actual entity, but I think that's not necessary. This works, it reduces cache lookups and we'll see the impact on performance tests between toolbar and navigation as we switch over standard and umami to navigation and that should be sufficient.
Comment #14
alexpottCommitted and pushed f82dea9e6e4 to 11.x and e29a6002942 to 11.3.x. Thanks!
Backported to 11.3.x as an allowable change during alpha.