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

Improve pagination performance #8602

Merged
merged 3 commits into from
Jun 8, 2021
Merged

Improve pagination performance #8602

merged 3 commits into from
Jun 8, 2021

Conversation

jmooring
Copy link
Member

@jmooring jmooring commented Jun 1, 2021

These calls are equivalent:

{{ template "_internal/pagination.html" . }}
{{ template "_internal/pagination.html" (dict "page" .) }}
{{ template "_internal/pagination.html" (dict "page" . "format" "default") }}

To use an alternate format:

{{ template "_internal/pagination.html" (dict "page" . "format" "terse") }}

Fixes #8599

@jmooring
Copy link
Member Author

jmooring commented Jun 2, 2021

Test site...

Instructions

Clone this branch of the repository and build the site.

git clone --single-branch -b hugo-github-issue-8602 https://github.com/jmooring/hugo-testing hugo-github-issue-8602
cd hugo-github-issue-8602
npm install
hugo
npx serve

Notes:

  • We use the npm serve package (npx serve) to serve the published site because hugo server can be a bit sluggish with larger sites.
  • Running hugo while running an IDE with an integrated file watcher can produce misleading performance metrics for larger sites. Close the IDE, then run hugo from a native terminal.
  • Finally, also in the interest of obtaining more accurate performance metrics, remove the public and resources directories before each run.

Site Structure

There are 10 content types (sections):

Section Title Content Pages Section Pages Pagers Aliases
10 Pages 10 1 0 1
20 Pages 20 1 1 1
30 Pages 30 1 2 1
40 Pages 40 1 3 1
50 Pages 50 1 4 1
60 Pages 60 1 5 1
70 Pages 70 1 6 1
80 Pages 80 1 7 1
90 Pages 90 1 8 1
10,000 Pages 10,000 1 999 1

Visit each section to see an example of the pagination format as it varies with the number of pagers.

Expected Counts

  • Pages: 10,461 (content pages + section pages + home page)
  • Paginator pages (pagers): 1035
  • Aliases: 10

Format

There are two display formats: default and terse.

The "default" format:

  • Always displays a link to the First pager
  • Always displays a link to the Previous pager
  • Always displays a link to the Next pager
  • Always displays a link to the Last pager
  • Displays links to a maximum of 5 pagers, 2 on either side of the link to the current pager

The "terse" format:

  • Does not display a link to the First pager from the first pager
  • Does not display a link to the Previous pager from the first pager
  • Does not display a link to the Next pager from the last pager
  • Does not display a link to the Last pager from the last pager
  • Displays links to a maximum of 3 pagers, 1 on either side of the link to the current pager

To use the "default" format, these are equivalent:

{{ template "_internal/pagination.html" . }}
{{ template "_internal/pagination.html" (dict "page" .) }}
{{ template "_internal/pagination.html" (dict "page" . "format" "default") }}

To use the "terse" format:

{{ template "_internal/pagination.html" (dict "page" . "format" "terse") }}

This approach will allow us to add alternate formats in the future without changing the appearance of existing sites.

I based the formats on a review of pagination formats from Google, Bing, and Amazon. See https://gist.github.com/jmooring/94e6e0bb0568ae5cdbae3372eb450111.

Style

While I have applied minimal styling to this test site, the Pull Request does not contain any CSS. Site creators continue to be responsible for styling.

Validation

The output is valid HTML5 per https://validator.w3.org/.

Scores from Google Lighthouse:

  • Performance: 100
  • Accessibility: 100
  • Best Practices: 100
  • SE0: 100

Performance

Build times are the average of 3 runs.

ID Pagination Template Format Build Time (seconds)
1 None NA 1.8
2 Old NA 22.4
3 New default 2.1
4 New terse 2.1

@jmooring jmooring marked this pull request as ready for review June 2, 2021 05:11
These calls are equivalent:

{{ template "_internal/pagination.html" . }}
{{ template "_internal/pagination.html" (dict "page" .) }}
{{ template "_internal/pagination.html" (dict "page" . "format" "default") }}

To use an alternate format:

{{ template "_internal/pagination.html" (dict "page" . "format" "terse") }}

Fixes #8599
Copy link
Member

@bep bep left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, this looks good now, but with the added complexity we need a minimal test for the variants.

Could you add the variants and some very simple assertions to TestEmbeddedTemplates in hugolib package?

Note that I'm mostly worried about someone making a "cometic change" months from now, introducing a nil-pointer or something.

@jmooring
Copy link
Member Author

jmooring commented Jun 5, 2021

Added minimal test for variants; assertions for first pager only.

@bep
Copy link
Member

bep commented Jun 6, 2021

Hey, I have thought about this while sleeping this weekend, and the internals looks very good -- but I'm not sure adding a dict config option is worth the complexity for toggling just one option (and the option is basically meaning "choose another template"). So, this:

{{ template "_internal/pagination.html" . }}
{{ template "_internal/pagination.html" (dict "page" .) }}
{{ template "_internal/pagination.html" (dict "page" . "format" "default") }}
{{ template "_internal/pagination.html" (dict "page" . "format" "terse") }}

Would be covered by:

{{ template "_internal/pagination.html" . }}
{{ template "_internal/pagination-terse.html" . }}

Which I think is easier to explain to people. What do you think?

@jmooring
Copy link
Member Author

jmooring commented Jun 6, 2021

I started out with something like this:

{{ $options := dict
  "page" .
  "slots" 9
  "showFirst" true
  "showFirstOnFirstPage" false
  "showPrevious" true
  "showPreviousOnFirstPage" false
  "showNext" true
  "showNextOnLastPage" false
  "showLast" true
  "showLastOnLastPage" false
}}
{{ template "_internal/pagination.html" $options }}

Or to call it with sensible defaults:

{{ template "_internal/pagination.html" . }}

But I was concerned that you would say, "This is too complicated." So I simplified, but perhaps I shouldn't have. I like the above better than what I finished with, and I would be happy to revert to this approach, including test cases.

@bep
Copy link
Member

bep commented Jun 7, 2021

But I was concerned that you would say, "This is too complicated."

I would have said that. This needs to be maintained and explained to people. With that set of options, the current setup would make sense -- but do people really want/need that?

@varunchopra
Copy link

I apologise if this sounds too stupid/ignorant -

  1. Does this change reduce memory usage? I believe the core ask with "slow" builds has been that slow becomes impossible due to RAM usage as Hugo doesn't support incremental builds.
  2. Can you explain how this impacts templates that are in use right now?

Also, if it helps, your comment here #8602 (comment) certainly makes much more sense than the OP.

@github-actions
Copy link

github-actions bot commented Jun 9, 2022

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 9, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve pagination performance
3 participants