-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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
feat(gatsby-plugin-sharp): cache base64 if possible #9059
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few comments!
@@ -401,7 +410,7 @@ function queueImageResizing({ file, args = {}, reporter }) { | |||
} | |||
} | |||
|
|||
async function notMemoizedbase64({ file, args = {}, reporter }) { | |||
async function base64({ file, args = {}, reporter, cache }) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One note on the cache here. We changed (in Gatsby 2.1.5 or so) how plugins get a cache instance. What will happen is that if a plugin, e.g. gatsby-transformer-remark, passes its cache instance to this function, these nodes will be cached at .cache/caches/gatsby-transformer-remark
which is probably ok but worth noting!
@pieh any ideas for how we'd improve this? Is it worth improving?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless we allow importing cache manager from gatsby, I don't think there is anything that can be done here so gatsby-plugin-sharp
use it's own cache instead of gatsby-transformer-remark
or gatsby-transformer-sharp
caches
7f847ee
to
abd6449
Compare
Thanks for jumping on this! BTW, did you benchmark the difference in build times with this change? |
I didn't run any benchmarks, but running this on a gatsby project with quite a lot of images didn't show any visible performance difference. I am interested in running a proper benchmark actually i am just not sure how i would do it. if anyone has an idea i am open for that! |
Where this PR helps is on the second run as then all the base64 images don't need to recalculated. Without this PR, generating base64s is the same each run and with this, it'd be nearly free the second time. So a test could be add 20 images to a site and query for the base64 of each and test the 1st and 2nd run times with and without this PR. |
I ll try and come up with something tonight but i think this wont change much: in graphiql i run twice this query: query IndexQuery {
allFile(
filter: {extension: { in: ["jpg"]}}
) {
edges {
node {
childImageSharp {
fluid(maxWidth:650, quality: 100) {
base64
}
}
}
}
}
} and while it takes some time for the first run (144 pictures), it is then almost instant on both cases, even when i add more fields in the params (unless there is cache in the graphiql queries). |
I hope i understood what benchmark you were looking for. AFAIU there is no substantial change. With Memoizewithout cache folder
second run
building
With cachewithout cache folder
second run
building
|
I've been doing more targeted benchmarks with this - this will only affect first rerun of query that has image processing with base64. For test case I've created page with 20 and 200 images and here are results with initial query run (this is using
So there definitely is some saving. When using |
re-add memoized version (if cache not available)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙏 Thanks @oorestisime!
* 'master' of github.com:gatsbyjs/gatsby: (63 commits) Update how-to-contribute.md to mention the style guide when writing blogs/tutorials (gatsbyjs#9742) Added Tylermcginnis website (gatsbyjs#9619) Fix grammar and punctuation (gatsbyjs#9498) Fix typo of plugin authoring (gatsbyjs#9737) Authentication tutorial - small fixes (gatsbyjs#9738) chore: move run-sift (gatsbyjs#9549) docs: fix minor typo (gatsbyjs#9730) chore(release): Publish fix(gatsby-plugin-page-creator): ensure that __tests__ directory is actually ignored (gatsbyjs#9720) fix: revert admin redirect (gatsbyjs#9728) fix: adjust page order to make nested matchPaths work (gatsbyjs#9719) feat(gatsby-plugin-sharp): cache base64 if possible (gatsbyjs#9059) chore(release): Publish fix(gatsby-plugin-offline): Serve the offline shell for short URLs + use no-cors for external resources (gatsbyjs#9679) chore(release): Publish fix: ensure babel-preset-gatsby can be used with unit tests (gatsbyjs#9629) feat(www): Filter posts by date (gatsbyjs#9400) fix(blog): azure blog post url date (gatsbyjs#9718) feat(blog): Add post on publishing to Azure (gatsbyjs#8868) Emphasize importance of promise return on source-plugin docs (gatsbyjs#9650) ...
Closes: gatsbyjs#6999 <!-- Q. Which branch should I use for my pull request? A. Use `master` branch (probably). Q. Which branch if my change is a bug fix for Gatsby v1? A. In this case, you should use the `v1` branch Q. Which branch if I'm still not sure? A. Use `master` branch. Ask in the PR if you're not sure and a Gatsby maintainer will be happy to help :) Note: We will only accept bug fixes for Gatsby v1. New features should be added to Gatsby v2. Learn more about contributing: https://www.gatsbyjs.org/docs/how-to-contribute/ -->
Closes: #6999