Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

0.6.0 longer build time #1323

Closed
rmoff opened this issue Aug 21, 2023 · 6 comments · Fixed by #1358
Closed

0.6.0 longer build time #1323

rmoff opened this issue Aug 21, 2023 · 6 comments · Fixed by #1358

Comments

@rmoff
Copy link
Contributor

rmoff commented Aug 21, 2023

Describe the bug

With 0.6.0 the build time doubles.

To Reproduce

  • 0.5.0

    bundle exec jekyll build
    Calling `DidYouMean::SPELL_CHECKERS.merge!(error_name => spell_checker)' has been deprecated. Please call `DidYouMean.correct_error(error_name, spell_checker)' instead.
    Configuration file: /Users/rmoff/git/lakeFS/docs/_config.yml
    To use retry middleware with Faraday v2.0+, install `faraday-retry` gem
                Source: /Users/rmoff/git/lakeFS/docs
           Destination: /Users/rmoff/git/lakeFS/docs/_site
     Incremental build: disabled. Enable with --incremental
          Generating...
          Remote Theme: Using theme pmarsceill/just-the-docs
       GitHub Metadata: No GitHub API authentication could be found. Some fields may be missing or have incorrect data.
                        done in 9.807 seconds.
     Auto-regeneration: disabled. Use --watch to enable.
    
  • 0.6.0

    bundle exec jekyll build
    Calling `DidYouMean::SPELL_CHECKERS.merge!(error_name => spell_checker)' has been deprecated. Please call `DidYouMean.correct_error(error_name, spell_checker)' instead.
    Configuration file: /Users/rmoff/git/lakeFS/docs/_config.yml
    To use retry middleware with Faraday v2.0+, install `faraday-retry` gem
                Source: /Users/rmoff/git/lakeFS/docs
           Destination: /Users/rmoff/git/lakeFS/docs/_site
     Incremental build: disabled. Enable with --incremental
          Generating...
          Remote Theme: Using theme pmarsceill/just-the-docs
       GitHub Metadata: No GitHub API authentication could be found. Some fields may be missing or have incorrect data.
                        done in 18.2 seconds.
     Auto-regeneration: disabled. Use --watch to enable.
    

Expected behavior

Build time should be the same (or quicker).

build --profile

  • 0.5.0

    Filename                                                                          | Count |    Bytes |  Time
    ----------------------------------------------------------------------------------+-------+----------+------
    _layouts/default.html                                                             |    96 | 4580.08K | 7.269
    _includes/nav.html                                                                |    96 | 1494.90K | 6.097
    jekyll-remote-theme-20230821-84887-6k4ybi/_includes/head.html                     |    96 |  357.78K | 0.653
    
  • 0.6.0

    Filename                                                                          | Count |    Bytes |   Time
    ----------------------------------------------------------------------------------+-------+----------+-------
    _layouts/default.html                                                             |    96 | 4692.78K | 14.810
    jekyll-remote-theme-20230821-84592-eyzcxk/_includes/head.html                     |    96 |  466.27K |  8.814
    jekyll-remote-theme-20230821-84592-eyzcxk/_includes/head_nav.html                 |    96 |  108.39K |  8.305
    _includes/nav.html                                                                |    96 | 1494.90K |  5.482
    jekyll-remote-theme-20230821-84592-eyzcxk/_includes/css/activation.scss.liquid    |    96 |  124.06K |  0.889
    

Ruby version

$ ruby --version
ruby 3.2.2

Further diagnostics

Please let me know if there is any other useful information that I can provide.

@mattxwang
Copy link
Member

Thanks for submitting an issue @rmoff. Hm, this is definitely concerning. @pdmosses would you be able to take a look at this? If not, I can try to dig in slightly later this week.

@pdmosses
Copy link
Contributor

@rmoff Thanks – the reported issue is very helpful!

Please let me know if there is any other useful information that I can provide.

To locate the source of the increased build time for the lakeFS site, I've cloned https://github.com/rmoff/lakeFS and I'm using the master branch. I'm assuming that your profiling is based on the same branch.

Profiling it locally with v0.5.0 and 0.6.0, the overall build times correspond to what you reported: increasing from about 8.5 seconds to about 18.5 seconds! (As the site uses the github-pages gem, switching to Jekyll 4 is non-trivial, so lets ignore that possibility.)

Initially, I suspected your customised versions of _layouts/default.html and _includes/nav.html to be the cause of the issue, as they are shadowing files used by the theme.1 In v0.6.0, nav.html is page-independent, and and its inclusion is cached.

Simply replacing:

{% include nav.html pages=site.html_pages %}

by

{% include_cached nav.html pages=site.html_pages %}

in your version of _layouts/default.html reduced the v0.6.0 build time from 18.5 to 11 seconds.

However, that is still an unexpected increase. And almost all the 11 seconds appeared to be for rendering the theme file _includes/head_nav.html, which is included (uncached) by head.html in v0.6.0.

Further profiling revealed that the remaining build time is almost entirely due to the two uses of Jekyll's scssify filter in head_nav.html: when both are removed,2 the total build time is only about 4.5 seconds:

Filename                                                                           | Count |    Bytes |  Time
-----------------------------------------------------------------------------------+-------+----------+------
_layouts/default.html                                                              |    94 | 4145.37K | 1.017
jekyll-remote-theme-20230821-78876-1pz0siz/_includes/head.html                     |    94 |  321.10K | 0.448
jekyll-remote-theme-20230821-78876-1pz0siz/_includes/vendor/anchor_headings.html   |    94 | 1085.33K | 0.260
assets/js/zzzz-search-data.json                                                    |     1 |  511.66K | 0.169
_includes/nav.html                                                                 |     1 |   14.01K | 0.083
_includes/footer.html                                                              |    94 |  486.80K | 0.043
assets/js/search-data.json                                                         |     1 |  484.30K | 0.038
_includes/head_nav.html                                                            |    94 |    0.09K | 0.032

The CSS generated by head_nav.html references the SCSS variables $feedback-color and $nav-list-expander-right. I'd incorrectly assumed that scssify would take only a short time when used on the SCSS files provided by the theme. I'll need to find a (significantly) more efficient way of accessing the values of the required variables.

In the meantime, assuming that visitors to the site always have JS enabled, there's a simple work-around for the current inefficiency of _includes/head_nav.html: shadow that file by an empty file. I expect that to significantly reduce your build times. However, upgrading securely to v0.6.0 will involve replaying your customisation of the other theme files.

Footnotes

  1. It is evident from the paths shown in the profiles that your profiling used local versions of those files.

  2. Removing only one use of scssify gives hardly any reduction.

@rmoff
Copy link
Contributor Author

rmoff commented Aug 21, 2023

Thanks @pdmosses, this is an amazing amount of detail and analysis :)

Apologies for not specifying; the repo in question is https://github.com/treeverse/lakeFS/ (the rmoff version is a fork).

pdmosses added a commit to pdmosses/just-the-docs that referenced this issue Aug 25, 2023
Fix just-the-docs#1323

- Remove `_includes/head_nav.html`.
- Generate page-independent SCSS in `assets/css/just-the-docs-head-nav.css`.
- Link to `/assets/css/just-the-docs-head-nav.css` in `head.html`.
- Disable the above stylesheet in `assets/js/just-the-docs.js`.
- Generate page-dependent CSS in `_includes/css/activation.scss.liquid` and include in `head.html`.
@pdmosses
Copy link
Contributor

I'd incorrectly assumed that scssify would take only a short time when used on the SCSS files provided by the theme. I'll need to find a (significantly) more efficient way of accessing the values of the required variables.

I think I've found one, reducing the total build time for the lakeFS site to just 4 seconds (including page-dependent CSS for use when JS is disabled). It's available at https://github.com/pdmosses/just-the-docs/tree/fix-head-nav. However, it needs further testing before I submit a PR.

@bittlingmayer
Copy link

We noticed that the sidebar was taking most of the time for our site with a lot of pages.

So we just cut the build time by about 7x by caching the sidebar:
#1340

@pdmosses
Copy link
Contributor

pdmosses commented Sep 2, 2023

So we just cut the build time by about 7x by caching the sidebar:
#1340

Unfortunately, simply caching the sidebar affects the navigation. Switching to Jekyll 4 should provide a similar speedup without affecting the navigation.

pdmosses added a commit that referenced this issue Oct 1, 2023
* Remove "passive" toggle

PR #1244 introduced the "passive" toggle, but just-the-docs.js subsequently disabled the only styling that used it, so it became redundant.
This removes it.

* Reduce build time for page-dependent CSS

Fix #1323

- Remove `_includes/head_nav.html`.
- Generate page-independent SCSS in `assets/css/just-the-docs-head-nav.css`.
- Link to `/assets/css/just-the-docs-head-nav.css` in `head.html`.
- Disable the above stylesheet in `assets/js/just-the-docs.js`.
- Generate page-dependent CSS in `_includes/css/activation.scss.liquid` and include in `head.html`.

* No override svg rotate

* Disable both stylesheets safely

* Move the site nav to a new include

- Fix the complete site nav
- Move the site nav to `_includes/site_nav.html`
- Cache the site nav
- Uncache `nav.html`

* Move nav and site_nav to _includes/components

* Replace id prefix

* Update breadcrumbs.html

Replace several filters by a single loop through all the pages,
but breaking as soon as possible.

Profiling indicates that this saves up to 50% of the breadcrumbs build time for the filters.

* Update just-the-docs-head-nav.css

Adjust the number of lines to keep

* Update head.html

Remove superflous type.

* Update activation.scss.liquid

Remove a superfluous closing brace.
Adjust layout.

* Use `scssify` to remove nesting

Preliminary profiling indicates that using `scssify` on the small number of nested CSS rules produced by `activation.scss.liquid` is quick enough.

* Update head.scss

Manual attempt at prettier (pending installation in Atom).

* Avoid generation of nested CSS

Local profiling indicated that using `scssify` on each page takes about 1% of the build time.

- Update `_includes/css/activation.scss.liquid` to generate non-nested CSS.
- Remove use of `scssify` from `_includes/head.html`.

* Ignore false positives from validator

Ignores: `:1.810-1.823: error: CSS: Parse Error.` and `:1.811-1.824: error: CSS: Parse Error.`; had to shift things around since the local config overrides the CI flag.

* Inline `_sass/head.css`

* Update CHANGELOG.md

---------

Co-authored-by: Matthew Wang <matt@matthewwang.me>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants