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

perf: use must-revalidate cache-control header as common and only create header routes for routes with different cache-control #38820

Merged

Conversation

pieh
Copy link
Contributor

@pieh pieh commented Jan 23, 2024

Description

before on left, with the change in the pr on right for sample reproduction with 10000 dummy pages (this is zoomed in on execution of just this line

const { redirects, headers, lambdasThatUseCaching, fileMovingPromise } =
processRoutesManifest(routesManifest, headerRoutes)
):
image

Note that in above cpu profile biggest drain is actually way content of _headers file is produced and not necessarily amount of them (tho they both combined cause the problem). String concatenations for each route (

acc += buildHeaderString(curr.path, curr.headers)
) and then headers concatenation in
acc += ` ${curr.key}: ${curr.value}\n`
is causing allocating a lot of new strings and subsequently create a lot of work for garbage collector (and garbage collection is where majority of time was actually spent). This change just lowers amount of times this need to happen, but possibly that part could be improved (i.e. instead of doing acc += 'string-to-append' we could push entries to array and finally to .join('') on that array to produce final string, but this change alone seems to just mostly avoid hitting issues in the first place

In practice (first reported time is just the build, and second is build + time spent in adapter handling):
before:

info Done building in 25.2874965 sec
✨  Done in 140.97s.

and after

info Done building in 25.716898292 sec
✨  Done in 33.33s.

this also result in much smaller public/_headers file as we no longer create entries for each html and page-data files - see https://www.diffchecker.com/VWVHsM5R/

Documentation

Tests

Updated unit tests + https://github.com/gatsbyjs/gatsby/blob/master/e2e-tests/adapters/cypress/e2e/headers.cy.ts still passing

Related Issues

…ate header routes for routes with different cache-control
@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Jan 23, 2024
@pieh pieh added topic: performance Related to runtime & build performance topic: adapters Related to Gatsby Adapters and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels Jan 23, 2024
@pieh pieh marked this pull request as ready for review January 24, 2024 06:32
Copy link

@mraerino mraerino left a comment

Choose a reason for hiding this comment

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

:gotta-parrot-fast: 🚀

@pieh pieh merged commit fb77fe5 into master Jan 25, 2024
34 checks passed
@pieh pieh deleted the perf/use-must-revalidate-cache-control-as-default-in-header-rules branch January 25, 2024 14:38
pieh added a commit that referenced this pull request Jan 25, 2024
…ate header routes for routes with different cache-control (#38820)

* perf: use must-revalidate cache-control header as common and only create header routes for routes with different cache-control

* test: update headerRoutes unit tests

(cherry picked from commit fb77fe5)
pieh added a commit that referenced this pull request Jan 25, 2024
…ate header routes for routes with different cache-control (#38820) (#38824)

* perf: use must-revalidate cache-control header as common and only create header routes for routes with different cache-control

* test: update headerRoutes unit tests

(cherry picked from commit fb77fe5)

Co-authored-by: Michal Piechowiak <misiek.piechowiak@gmail.com>
This was referenced May 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: adapters Related to Gatsby Adapters topic: performance Related to runtime & build performance
Projects
Status: Published
Development

Successfully merging this pull request may close these issues.

3 participants