Improve build time of navigation panel #956
Conversation
The possibility of using the `title` as a default value for `nav_order` requires page grouping (because of the lack of support in Jekyll-Liquid for sorting by the value of an expression). It appears that extracting a sorted list of pages from a sorted list of groups is quadratic in the number of groups: it can take 35 seconds for 130 single-value groups. This update to nav.html circumvents this source of inefficiency by generating the navigation directly from the sorted page groups.
The possibility of using the `title` as a default value for `nav_order` requires page grouping (because of the lack of support in Jekyll-Liquid for sorting by the value of an expression). It appears that extracting a sorted list of pages from a sorted list of groups is quadratic in the number of groups: it can take 35 seconds for 130 single-value groups. This update to nav.html circumvents this source of inefficiency by generating the navigation directly from the sorted page groups. Building the same site with 130 pages now takes only about 6 seconds with Jekyll v4.2.2. In the previous commit, distinguishing between numbers and strings didn't work. This commit has been partially tested, locally.
There was a problem hiding this comment.
Overall I think this is good to merge. I've left a nit and a note for later, but I think neither should block this being merged.
I am slightly wary of this possibly introducing breaking changes to existing sites, though I appreciate the thoroughness with the tests repo. I think my plan is:
- merge this
- cut a new rc (not v0.4.0)
- let @captn3m0 and @nathancarter test our RC to confirm that it still resolves their problem (somewhat trivial)
- field any new bug reports we receive from the new nav
- publish a v0.4.0 by the end of this month 😊
Improve alignment. Co-authored-by: Matt Wang <matt@matthewwang.me>
The current navigation tests check only for regression regarding previous bug fixes, so I don't think they should be regarded as "thorough". Some of the changes made in v0.4.0.rc1 are bug fixes, which are deliberately not backwards compatible with v0.3.3. However, this PR is intended to generate exactly the same nav panels as v0.4.0.rc1. After locally updating the just-the-docs-tests: diff _site-0.4.0.rc1-3.8.7/index.html _site-fix-nav-performance-3.8.7/index.html
28a29
> <a class="skip-to-main" href="#main-content">Skip to main content</a>
178c179
< <br>Version: v0.4.0.rc1
---
> <br>Version: fix-nav-performance
just-the-docs-tests: That confirms that the nav links on the home page of I think it's safe to conclude that the nav build optimisation in this PR doesn't change the hierarchy displayed in the nav panels. The nav panels on all the other pages are (hopefully) the same as on the home page, apart from unfolding and font details. But also the breadcrumbs and the auto-TOCs might differ. A rigorous check would diff each pair of html files, which would require some scripting. |
|
Great, thanks @pdmosses - that explanation helps clarify things for me. Time to merge + release rc2! |
|
Changelog:
|
Summary:Summary of build times with different versions:
|
|
@captn3m0 thanks for your follow-up with the summary and the profiling results! Presumably you're using Jekyll v4.2.2. As there are breaking changes from Jekyll v3 (some obvious, some non-obvious), I hope we'll soon be able to drop support for Jekyll v3. Perhaps that's easier now that GH Actions is the default build on GH Pages? I wrote in a comment:
I might try doing that (in a fresh PR). I suspect that rather few sites use a mixture of numbers and strings. |
|
I wrote:
@mattxwang, in fact I could have used If it's possible to revert the commit that added the aria labels to the PR branch, that would allow me to regenerate the the previous test site (so as to diff it with BTW, I was expecting my PR branch to be merged into |
Understood. In the future, would it be possible to rebase to the last commit before this one was merged in, and then compare? I believe the history is linear, so you'd still be able to isolate the difference introduced in one commit.
Go for it! Feel free to force-push to this branch, etc. - especially since we've already merged it. Let me know if you need help with that!
Oh, no - not a mistake on your end! I've enabled a setting for this repo that requires branches to be up-to-date with |
I can try – so long as I don't need to resolve conflicts in the process…
Ah, I've just recalled that
Not now that I know about it! 😄 |
- See just-the-docs/just-the-docs#956 - This is still 7-8 seconds slower than our fast version, but this lets us stay closer to upstream
- See just-the-docs/just-the-docs#956 - This is still 7-8 seconds slower than our fast version, but this lets us stay closer to upstream
- See just-the-docs/just-the-docs#956 - This is still 7-8 seconds slower than our fast version, but this lets us stay closer to upstream
- See just-the-docs/just-the-docs#956 - This is still 7-8 seconds slower than our fast version, but this lets us stay closer to upstream
- See just-the-docs/just-the-docs#956 - This is still 7-8 seconds slower than our fast version, but this lets us stay closer to upstream
- See just-the-docs/just-the-docs#956 - This is still 7-8 seconds slower than our fast version, but this lets us stay closer to upstream
- See just-the-docs/just-the-docs#956 - This is still 7-8 seconds slower than our fast version, but this lets us stay closer to upstream
- See just-the-docs/just-the-docs#956 - This is still 7-8 seconds slower than our fast version, but this lets us stay closer to upstream
- See just-the-docs/just-the-docs#956 - This is still 7-8 seconds slower than our fast version, but this lets us stay closer to upstream
- See just-the-docs/just-the-docs#956 - This is still 7-8 seconds slower than our fast version, but this lets us stay closer to upstream
- See just-the-docs/just-the-docs#956 - This is still 7-8 seconds slower than our fast version, but this lets us stay closer to upstream
- See just-the-docs/just-the-docs#956 - This is still 7-8 seconds slower than our fast version, but this lets us stay closer to upstream
Fix #863.
The current Liquid code to generate the navigation panel involves the inefficient extraction of a list of pages from a list of page groups (identified by @captn3m0 in his original explanation of the performance issue).
The optimisation implemented by this PR generates navigation links directly from the list of page groups, thereby avoiding the extraction of a list of pages from it. The Liquid code is now a bit tedious, but I don't see a simpler solution.
The need for grouping pages arises because Jekyll doesn't provide a filter to sort a list of pages on the value of an arbitrary expression.
Using Jekyll v4.2.2 (macOS 12.5, M2 MacBook Air, 16 GB memory), building https://github.com/endoflife-date/endoflife.date using https://github.com/pdmosses/just-the-docs/blob/fix-nav-performance/_includes/nav.html produced the following profile extract:
@nathancarter has tried adding the new
nav.htmlto a site with over 300 pages, and reported that it improved the build time of more than 3 minutes to about 30 seconds.Further optimisation of navigation might be possible (e.g., using Jekyll include caching), but the current optimisation should be sufficient for v0.4.0.
To test that this PR does not appear to affect the navigation panel generated by v0.3.3:
_config.ymlandGemfileto use this PR branch.bundle update.(Many of the differences reported in the GitHub visualisation of the changes are due to shifting much of the code 2 spaces to the left, in connection with moving the first
ulelement to be close to its first item.)