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 Static Asset Generation Performance #12922

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

adamchal
Copy link

@adamchal adamchal commented Jan 7, 2025

Changes

Improve static asset generation performance by removing the await in the for loop. This change will allow the p-queue concurrency to limit the number of parallel tasks. Previously, the await in the for loop would cause all asset generation tasks to run serially.

Before:

for (const [originalPath, transforms] of staticImageList) {
	await generateImagesForPath(originalPath, transforms, assetsCreationPipeline, queue);
}

After:

for (const [originalPath, transforms] of staticImageList) {
	generateImagesForPath(originalPath, transforms, assetsCreationPipeline, queue);
}

Fixes #12845

Testing

No additional tests required.

Docs

Static asset generation performance should increase by os.cpus().length in most Astro projects with assets.

Copy link

changeset-bot bot commented Jan 7, 2025

🦋 Changeset detected

Latest commit: bd9728b

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 github-actions bot added the pkg: astro Related to the core `astro` package (scope) label Jan 7, 2025
Copy link

codspeed-hq bot commented Jan 7, 2025

CodSpeed Performance Report

Merging #12922 will not alter performance

Comparing adamchal:asset-generation-performance (bd9728b) with main (8b9d530)

Summary

✅ 6 untouched benchmarks

@adamchal
Copy link
Author

adamchal commented Jan 7, 2025

@ascorbic I’m skeptical that this change would affect any tests, but 5 CI tests are failing. A quick investigation reveals that some fixture assets are ENOENT. I’m looking for historical success with these tests but figured I’d check quickly with you first before digging deeper into the tests.

As you mentioned, because we await queue.onIdle(); the transformed asset should be finalized before any test would run. My fear is that this assumption is wrong, but that would likely be a dark tunnel of race conditions that just happens to work if we generate images more slowly (one-by-one).

@ascorbic
Copy link
Contributor

ascorbic commented Jan 8, 2025

Yes, this does make me concerned that it's not waiting correctly. One way to check could be to push all of the generateImagesForPath responses into an array and await Promise.all at the end. If that fixes it then it probably means that the idle wait isn't catching everything

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: astro Related to the core `astro` package (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Image processing parallelity
2 participants