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

Build: use partialCached #1951

Merged
merged 3 commits into from
Apr 3, 2025
Merged

Conversation

dotnetCarpenter
Copy link
Contributor

Changes

  • footer.html
  • monitor.html

Context

This gives a 11% (9 seconds) speedup, when building the site, on my system.

@@ -165,7 +165,7 @@ <h1 data-pagefind-meta="title">About{{ if (isset .Params "subtitle") }} - {{ .Pa
{{ end }}
</div>
</div>
{{ partial "footer.html" . }}
{{ partialCached "footer.html" . }}
Copy link
Member

Choose a reason for hiding this comment

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

I believe that this would break the /search page, as there is a line in footer.html that is contingent on the current page section.

Copy link
Member

Choose a reason for hiding this comment

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

@dotnetCarpenter could you have a look whether the search page renders correctly?

Copy link
Member

Choose a reason for hiding this comment

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

@dotnetCarpenter I had a look, and it does not look good:

ca70cdd917e0fef93cec35d593cd9e12db7a5ab7

In my hands, you need this to fix it:

Suggested change
{{ partialCached "footer.html" . }}
{{ if eq (.Scratch.Get "section") "search" }}
{{ partial "footer.html" . }}
{{ else }}
{{ partialCached "footer.html" . }}
{{ end }}

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dscho I'll take a look :)

@dscho
Copy link
Member

dscho commented Feb 19, 2025

That is a nice improvement! If we can make it work even with the search page (think: https://git-scm.com/search/results?search=site&language=en), I am all for it!

@dotnetCarpenter
Copy link
Contributor Author

@dscho it seems that varying by "section" works. That is, create a new cached version for each section. But now I'm not seeing any speed improvement in build time. I think, without any measurable performance boost in build time, this PR only complicates matters and should be closed.

I'm extremely new to hugo, so there might be something worth salvaging here but I don't think so.

@dotnetCarpenter
Copy link
Contributor Author

All tests pass with #1955 on Kali linux with:

bun script/serve-public.js
npx playwright install firefox
npx playwright test --project=firefox

@dscho
Copy link
Member

dscho commented Feb 25, 2025

npx playwright test --project=firefox

I believe that this tests https://git-scm.com/ by default. To test the local site, you need to set PLAYWRIGHT_TEST_URL=http://localhost:5000/ before running Playwright (and then you do not need to start the server manually).

@dscho
Copy link
Member

dscho commented Feb 25, 2025

@dscho it seems that varying by "section" works. That is, create a new cached version for each section. But now I'm not seeing any speed improvement in build time.

Right. A much better idea might be to move this line from footer.html into the baseof.html layout, just before the line that includes the footer in the part that is relevant to the search page:

{{ if eq (.Scratch.Get "section") "search" }}<script src="{{ relURL "pagefind/pagefind-ui.js" }}"></script>{{ end }}

Since this is the only conditional code in footer.html (and there is no conditional code in monitor.html), this should salvage the improved build time again.

For what it's worth, I believe that I did things quite sub-optimally with Hugo; It feels as if the proper way would not end up with a huge baseof.html layout including lots of conditionals...

@dotnetCarpenter
Copy link
Contributor Author

dotnetCarpenter commented Feb 25, 2025

I ran the test with PLAYWRIGHT_TEST_URL=http://localhost:5000/ and got an error that looks like the expectation is slightly off. It's missing a trailing /.

Error: Timed out 5000ms waiting for expect(locator).toHaveAttribute(expected)

Locator: locator('#search-results').getByRole('link').first()
Expected pattern: /\/docs\/git-add\/fr(\.html)?$/
Received string:  "/docs/git-add/fr/"
Call log:
  - expect.toHaveAttribute with timeout 5000ms
  - waiting for locator('#search-results').getByRole('link').first()
    5 × locator resolved to <a href="/docs/git-add/fr/">git-add</a>
      - unexpected value "/docs/git-add/fr/"


  138 |   await searchBox.fill('add')
  139 |   await searchBox.press('Shift')
> 140 |   await expect(searchResults.getByRole("link").nth(0)).toHaveAttribute('href', /\/docs\/git-add\/fr(\.html)?$/)
      |                                                        ^
  141 |
  142 |   // pressing the Enter key should navigate to the full search results page
  143 |   await searchBox.press('Enter')
    at /home/dotnet/projects/opensource/git-scm.com/tests/git-scm.spec.js:140:56

Perhaps /\/docs\/git-add\/fr(\.html|\/)?$/ would be better, if .html is a possible solution. What are the correct possibilities?

103-1-failure

EDIT

Hmm for some reason there are two versions of each result. One with a trailing / and one without. I get the same result with PR #1955.

@dscho
Copy link
Member

dscho commented Feb 27, 2025

Expected pattern: //docs/git-add/fr(.html)?$/
Received string: "/docs/git-add/fr/"

Hmm. The received string seems bogus: If you call curl https://git-scm.com/docs/git-add/fr/ you will see that it wants to redirect to https://git-scm.com/docs/git-add/fr:

<html lang="en">
 <head>
  <meta charset="utf-8">
  <title>Redirecting&hellip;</title>
  <link rel="canonical" href="https://git-scm.com/docs/git-add/fr">
  <meta http-equiv="refresh" content="0; url=https://git-scm.com/docs/git-add/fr">
  <meta name="robots" content="noindex">
 </head>
 <body>
  <script>window.location.replace(document.querySelector("link[rel='canonical']").href + window.location.search + window.location.hash)</script>
  <h1>Redirecting&hellip;</h1>
  <a href="https://git-scm.com/docs/git-add/fr">Click here if you are not redirected.</a>
 </body>
</html>

So why does your Playwright instance not follow that redirect?

@dscho
Copy link
Member

dscho commented Feb 27, 2025

So why does your Playwright instance not follow that redirect?

Oh, did you perchance turn off "ugly" URLs?

@dscho
Copy link
Member

dscho commented Mar 2, 2025

@dscho it seems that varying by "section" works. That is, create a new cached version for each section. But now I'm not seeing any speed improvement in build time.

Right. A much better idea might be to move this line from footer.html into the baseof.html layout, just before the line that includes the footer in the part that is relevant to the search page:

{{ if eq (.Scratch.Get "section") "search" }}<script src="{{ relURL "pagefind/pagefind-ui.js" }}"></script>{{ end }}

Since this is the only conditional code in footer.html (and there is no conditional code in monitor.html), this should salvage the improved build time again.

For what it's worth, I believe that I did things quite sub-optimally with Hugo; It feels as if the proper way would not end up with a huge baseof.html layout including lots of conditionals...

How about rebasing on top of gh-pages (which has the additional Playwright test of the search results page) and playing with this idea?

@dscho
Copy link
Member

dscho commented Mar 14, 2025

@dotnetCarpenter gentle ping?

@dscho
Copy link
Member

dscho commented Apr 2, 2025

@dotnetCarpenter another gentle ping?

@dotnetCarpenter
Copy link
Contributor Author

Here are the results after rebasing with gh-pages:

$ hugo
---
languageCode: en
title: Git
disablePathToLower: true
relativeURLs: true
uglyURLs: true
disableKinds:
- RSS
- taxonomy
- term
markup:
  goldmark:
    renderer:
      unsafe: true
module:
  mounts:
  - source: content
    target: content
  - source: static
    target: static
  - source: data
    target: data
  - source: external/book/data/book
    target: data/book
  - source: external/book/content/book
    target: content/book
  - source: external/book/static/book
    target: static/book
  - source: external/docs/data
    target: data
  - source: external/docs/content
    target: content
params:
  hugo_version: 0.139.4
  pagefind_version: 1.1.1
  latest_version: 2.49.0
  latest_relnote_url: https://raw.github.com/git/git/master/Documentation/RelNotes/2.49.0.adoc
  latest_release_date: '2025-03-14'
  macos_installer:
    url: https://sourceforge.net/projects/git-osx-installer/files/git-2.33.0-intel-universal-mavericks.dmg/download?use_mirror=autoselect
    version: 2.33.0
    release_date: '2021-08-30'
    filename: git-2.33.0-intel-universal-mavericks.dmg
  windows_installer:
    portable32:
      filename: PortableGit-2.48.1-32-bit.7z.exe
      release_date: '2025-02-13'
      version: 2.48.1
      url: https://github.com/git-for-windows/git/releases/download/v2.48.1.windows.1/PortableGit-2.48.1-32-bit.7z.exe
    portable64:
      filename: PortableGit-2.49.0-64-bit.7z.exe
      release_date: '2025-03-17'
      version: 2.49.0
      url: https://github.com/git-for-windows/git/releases/download/v2.49.0.windows.1/PortableGit-2.49.0-64-bit.7z.exe
    installer32:
      filename: Git-2.48.1-32-bit.exe
      release_date: '2025-02-13'
      version: 2.48.1
      url: https://github.com/git-for-windows/git/releases/download/v2.48.1.windows.1/Git-2.48.1-32-bit.exe
    installer64:
      filename: Git-2.49.0-64-bit.exe
      release_date: '2025-03-17'
      version: 2.49.0
      url: https://github.com/git-for-windows/git/releases/download/v2.49.0.windows.1/Git-2.49.0-64-bit.exe



$ npx -y pagefind --site public
Running Pagefind v1.3.0 (Extended)
Running from: "/home/dotnet/projects/opensource/git-scm.com"
Source:       "public"
Output:       "public/pagefind"

[Walking source directory]
Found 95222 files matching **/*.{html}

[Parsing files]
Found a data-pagefind-body element on the site.
↳ Ignoring pages without this tag.

[Reading languages]
Discovered 36 languages: fr, pt-pt, tr, is, zh-tw, pt-br, it, fa, gr, nl, be, zh_hans-cn, sv, ko, uz, pl, zh, bg, en, ms, cs, pt_br, mk, de, sl, az, ro, ru, uk, sr, ja, es, id, zh_hant, tl, vi

[Building search indexes]
Total: 
  Indexed 36 languages
  Indexed 4709 pages
  Indexed 411895 words
  Indexed 36 filters
  Indexed 0 sorts


$ node script/serve-public.js
Now listening on: http://localhost:5000/
Could not read /home/dotnet/projects/opensource/git-scm.com/public/does-not.exist


$ PLAYWRIGHT_TEST_URL=http://localhost:5000/ npx playwright test --project=firefox
Running 10 tests using 2 workers
  1 skipped
  9 passed (14.2s)

@dscho
Copy link
Member

dscho commented Apr 3, 2025

Here are the results after rebasing with gh-pages:

Are you sure that you fetched gh-pages from this repository? I still see merge conflicts...

@dotnetCarpenter
Copy link
Contributor Author

🤔 not sure why there are conflicts after rebasing. In my mind that should remove the conflicts...

@dotnetCarpenter
Copy link
Contributor Author

$ git fetch --all
$ git rebase upstream/gh-pages
$ git remote -v
dscho	[email protected]:dscho/git-scm.com.git (fetch)
dscho	[email protected]:dscho/git-scm.com.git (push)
origin	[email protected]:dotnetCarpenter/git-scm.com.git (fetch)
origin	[email protected]:dotnetCarpenter/git-scm.com.git (push)
upstream	[email protected]:git/git-scm.com.git (fetch)
upstream	[email protected]:git/git-scm.com.git (push)

But I can try to do a pull in my gh-pages branch and re-build.

@dscho
Copy link
Member

dscho commented Apr 3, 2025

@dotnetCarpenter when I direct my browser to https://github.com/dotnetCarpenter/git-scm.com/tree/hugo-build-time, I see this:

image

155 commits behind gh-pages...

Does git rev-parse HEAD say 3c1dacb8530585faf81fde20283917803e13c74e? And git rev-parse upstream/gh-pages, does it say aa794fdd762c8e6144569e634ef730639f953cf4? If so, I do not understand how git rebase upstream/gh-pages fails to do that job. Maybe try git rebase aa794fdd762c8e6144569e634ef730639f953cf4?

@dotnetCarpenter
Copy link
Contributor Author

@dotnetCarpenter when I direct my browser to https://github.com/dotnetCarpenter/git-scm.com/tree/hugo-build-time, I see this:

image

155 commits behind gh-pages...

Does git rev-parse HEAD say 3c1dacb8530585faf81fde20283917803e13c74e? And git rev-parse upstream/gh-pages, does it say aa794fdd762c8e6144569e634ef730639f953cf4? If so, I do not understand how git rebase upstream/gh-pages fails to do that job. Maybe try git rebase aa794fdd762c8e6144569e634ef730639f953cf4?

I did it the manuel way and checked out gh-pages, then pull, then rebase again :)

@dscho
Copy link
Member

dscho commented Apr 3, 2025

I did it the manuel way and checked out gh-pages, then pull, then rebase again :)

Oh, you probably have a local branch called upstream/gh-pages, i.e. refs/heads/upstream/gh-pages...

@dotnetCarpenter
Copy link
Contributor Author

After proper rebase the test suite fails but it seems unrelated?

PLAYWRIGHT_TEST_URL='http://localhost:5000/' npx playwright test --project=firefox Running 11 tests using 2 workers 1) [firefox] › tests/git-scm.spec.js:294:1 › dark mode ───────────────────────────────────────────
Error: expect(page).toHaveScreenshot(expected)

  25948 pixels (ratio 0.09 of all image pixels) are different.

Expected: /home/dotnet/projects/opensource/git-scm.com/tests/git-scm.spec.js-snapshots/light-mode-firefox-linux.png
Received: /home/dotnet/projects/opensource/git-scm.com/test-results/git-scm-dark-mode-firefox/light-mode-actual.png
    Diff: /home/dotnet/projects/opensource/git-scm.com/test-results/git-scm-dark-mode-firefox/light-mode-diff.png

Call log:
  - expect.toHaveScreenshot(light-mode.png) with timeout 5000ms
    - verifying given screenshot expectation
  - taking page screenshot
    - disabled all CSS animations
  - waiting for fonts to load...
  - fonts loaded
  - 25948 pixels (ratio 0.09 of all image pixels) are different.
  - waiting 100ms before taking screenshot
  - taking page screenshot
    - disabled all CSS animations
  - waiting for fonts to load...
  - fonts loaded
  - captured a stable screenshot
  - 25948 pixels (ratio 0.09 of all image pixels) are different.


  302 |
  303 |   const o = { maxDiffPixels: 30 };
> 304 |   await expect(page).toHaveScreenshot({ name: 'light-mode.png', ...o });
      |                      ^
  305 |   await darkModeButton.click();
  306 |   await expect(page).toHaveScreenshot({ name: 'dark-mode.png', ...o });
  307 |
    at /home/dotnet/projects/opensource/git-scm.com/tests/git-scm.spec.js:304:22

attachment #1: light-mode-expected.png (image/png) ─────────────────────────────────────────────
tests/git-scm.spec.js-snapshots/light-mode-firefox-linux.png
────────────────────────────────────────────────────────────────────────────────────────────────

attachment #2: light-mode-actual.png (image/png) ───────────────────────────────────────────────
test-results/git-scm-dark-mode-firefox/light-mode-actual.png
────────────────────────────────────────────────────────────────────────────────────────────────

attachment #3: light-mode-diff.png (image/png) ─────────────────────────────────────────────────
test-results/git-scm-dark-mode-firefox/light-mode-diff.png
────────────────────────────────────────────────────────────────────────────────────────────────

attachment #4: screenshot (image/png) ──────────────────────────────────────────────────────────
test-results/git-scm-dark-mode-firefox/294-1-failure.png
────────────────────────────────────────────────────────────────────────────────────────────────

1 failed
[firefox] › tests/git-scm.spec.js:294:1 › dark mode ────────────────────────────────────────────
1 skipped
9 passed (16.4s)

@dscho
Copy link
Member

dscho commented Apr 3, 2025

25948 pixels (ratio 0.09 of all image pixels) are different.

Hmm. This could be a version-dependent problem, or maybe a difference in operating system. Or maybe an issue related to anti-aliasing. Color differences are finicky. Could I ask you to replace maxDiffPixels: 30 with maxDiffPixelRatio: 0.5 and then try again? Or maybe threshold: 0.3 would be a more appropriate fix for this?

+ footer.html
+ monitor.html

This gives a 11% (9 seconds) speedup, when building the site,
on my system.
Create a new cached footer for each section set in baseof.html.
This makes sure that pagefind/pagefind-ui.js is included if the
section is "search".
@dscho dscho force-pushed the hugo-build-time branch from 7be0ce3 to 7ea3705 Compare April 3, 2025 11:55
@dscho
Copy link
Member

dscho commented Apr 3, 2025

25948 pixels (ratio 0.09 of all image pixels) are different.

Hmm. This could be a version-dependent problem, or maybe a difference in operating system. Or maybe an issue related to anti-aliasing. Color differences are finicky. Could I ask you to replace maxDiffPixels: 30 with maxDiffPixelRatio: 0.5 and then try again? Or maybe threshold: 0.3 would be a more appropriate fix for this?

@dotnetCarpenter Also, could you paste the images that are presented side-by-side in Playwright's report (it opens a web server to serve them after failed tests, and you can then click on the link it shows and then expand the failed test to see the differences in various modes)?

@dscho dscho merged commit 048bec5 into git:gh-pages Apr 3, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants