feat: Rework image generation to improve performance #8821
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
🦋 Changeset detectedLatest commit: ecdc634 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
github-actions
bot
added
the
pkg: astro
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
!preview concurrent-assets |
|
|
We need to be able to detect when a file is empty as with multiple writers/readers you can get into a situation in which the generation crashes with a file generated and not written. This in turn causes the next build to fail with the |
bluwy
approved these changes
Oct 13, 2023
ematipico
approved these changes
Oct 13, 2023
|
Do we want to consider a way to set the cpu count manually? In some instances one might want to reduce the parallelisation if the backend doesn't support it for remote images, or something... |
Princesseuh
commented
Oct 15, 2023
Comment on lines
+21
to
+24
| argsIgnorePattern: '^_', | ||
| varsIgnorePattern: '^_', | ||
| caughtErrorsIgnorePattern: '^_', | ||
| ignoreRestSiblings: true, |
There was a problem hiding this comment.
This is unrelated to the PR, but it was driving me nuts
|
!preview concurrent-assets |
|
|
@bluwy @ematipico I kinda rewrote it all from scratch 🙈. I updated the PR description to reflect what was done and would love to see a re-review from you both! (I made |
Princesseuh
changed the title
Princesseuh
commented
Oct 15, 2023
| env: AssetEnv, | ||
| queue: PQueue | ||
| ) { | ||
| const originalImageData = await loadImage(originalFilePath, env); |
There was a problem hiding this comment.
In theory, the fact that we load the image at the beginning even when all the transforms might be cached already is slower, especially for fully cached builds where there's a lot of different images with a small amount of variants. However, in practice I found that this didn't change much, probably that the rest of the improvements counteract this, or maybe hard drives are just fast enough now.
If we want to get it of this, what we could probably do is store a list of cached hashes and check first if all the transforms requested are already cached before loading the image, shouldn't be too hard.
There was a problem hiding this comment.
I think we should apply your suggestions, probably in a follow-up PR. An I/O operation is usually more expensive than a memory operation, and even though Node.js is really great in I/O operations, I think we should try to avoid them if we can.
There was a problem hiding this comment.
I definitely agree, but I think it's not as bad as it might seems because the file is already written in the same process, so it's presumably already in some sort of hot cache.
There was a problem hiding this comment.
Maybe we also make originalImageData a function to lazily load it, and those who call in parallel would share the same cache? Vite has this utility (which could be generalized) to simplify a bit.
But also I think we can do it in a follow-up too if there's not a big perf impact currently.
Princesseuh
commented
Oct 15, 2023
Comment on lines
+215
to
+217
| for (const [originalPath, transforms] of staticImageList) { | ||
| await generateImagesForPath(originalPath, transforms, assetsCreationEnvironment, queue); | ||
| } |
There was a problem hiding this comment.
A question someone might have: Would make this part concurrent help?
No (at least as far as I can tell), it doesn't really matter if the queue is full of variants from one image vs variants of multiple images.
|
!preview concurrent-assets |
|
15 tasks
ematipico
reviewed
Oct 16, 2023
| env: AssetEnv, | ||
| queue: PQueue | ||
| ) { | ||
| const originalImageData = await loadImage(originalFilePath, env); |
There was a problem hiding this comment.
I think we should apply your suggestions, probably in a follow-up PR. An I/O operation is usually more expensive than a memory operation, and even though Node.js is really great in I/O operations, I think we should try to avoid them if we can.
We have tests with remote images, both cached and uncached ones! We also have tests for errors specific to remote images etc. |
|
@Princesseuh not sure where the delays are, but the cached builds are definitely slower than before... the un-cached ones are incredibly fast in comparison, but the cached builds aren't that much faster than uncached now. |
It's a mix of multiple things
For local images on a fast hard drive, there's probably no big differences between this PR and main, but remote images suffer a bit more, I'll fix it |
It's somewhat baffling to me though, adding concurrency doesn't only not improve the performance (for what is basically the same process as the uncached images, just without the fetch, which is basically what has always been done and not much different bar the actual copying to what local images do), but slows it down by almost an order of magnitude... there must be something tiny somewhere that causes this.
I have no doubts about that ahah |
1 task
natemoo-re
approved these changes
Oct 23, 2023
There was a problem hiding this comment.
As discussed during today's standup, I'm totally comfortable with the tradeoff being made here! Approved.
Uncached build are an order-of-magnitude, while cached builds seem to be slightly slower in some cases. Hopefully that can be improved in a follow-up, but this is still worth it.
github-actions
bot
added
the
pr: docs
sarah11918
reviewed
Oct 25, 2023
Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>
Princesseuh
dismissed
GitHub Actions’s stale review
It's merge day, once again
Merged
natemoo-re
pushed a commit
that referenced
this pull request
Nov 22, 2023
* feat: implement concurrency for asset generation * add changeset * fix: count * feat: rework image generation to reuse image buffer for transforms of the same image * fix: assetsPrefix nonsense * feat: add back the counter * refactor: cleanup my TS nonsense * nit: reuse type * nit: apply suggestions * nit: macOS micro optimization * Update .changeset/good-mirrors-bake.md Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca> --------- Co-authored-by: Matteo Manfredi <matteo@manfredi.io> Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes
Change how image generation is done to improve performance:
On my website, that has 222 local images, here's the result:
Before
After
Not exactly scientific, but there's definitely an improvement in there.
Testing
Tests should still pass!
Docs
N/A.