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

Refactor benchmark script #6376

Merged
merged 37 commits into from
Mar 1, 2023
Merged

Refactor benchmark script #6376

merged 37 commits into from
Mar 1, 2023

Conversation

bluwy
Copy link
Member

@bluwy bluwy commented Feb 27, 2023

Changes

Update root pnpm benchmark script and CI !bench command. See https://github.com/bluwy/astro/pull/2 for examples, e.g. https://github.com/bluwy/astro/pull/2#issuecomment-1448419909

Also added a new /benchmark folder with READMEs within it that should hopefully help clarify how it works.

The Astro codebase is also tweaked a bit to collect timing/memory logs via process.env.ASTRO_TIMER_PATH, and if it exists collect and write the results to the path. This makes passing the results easier between child processes. It's only used for benchmarking.


Locally, you can run pnpm benchmark, pnpm benchmark memory, pnpm benchmark server-stress, etc

In PR comments, you can comment !bench, !bench memory, !bench server-stress, etc

Testing

Manual testing

Docs

n/a. This doesn't touch on user documentation

@changeset-bot
Copy link

changeset-bot bot commented Feb 27, 2023

⚠️ No Changeset found

Latest commit: 9a943d3

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@github-actions github-actions bot added pkg: astro Related to the core `astro` package (scope) 🚨 action Modifies GitHub Actions labels Feb 27, 2023
@bluwy

This comment was marked as outdated.

@bluwy
Copy link
Member Author

bluwy commented Feb 27, 2023

Looks like I need to fork this to properly test the workflow. Now it's using the main workflow.

@github-actions

This comment was marked as outdated.

@natemoo-re
Copy link
Member

Looks like I need to fork this to properly test the workflow. Now it's using the main workflow.

Yep that's correct. I use a personal fork for testing actions/CI.

@bluwy bluwy marked this pull request as ready for review February 28, 2023 16:01
@bluwy bluwy requested a review from a team as a code owner February 28, 2023 16:01
@bluwy bluwy removed the request for review from a team February 28, 2023 16:02
Copy link
Contributor

@matthewp matthewp left a comment

Choose a reason for hiding this comment

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

Looks great! Excited for this.

benchmark/bench/server-stress.js Show resolved Hide resolved
benchmark/ci-helper.js Show resolved Hide resolved
*/
export class AstroTimer {
private enabled: boolean;
private ongoingTimers: Map<string, OngoingStat> = new Map();
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we could use WeakMap? It doesn't seem we iterate them

Copy link
Member Author

Choose a reason for hiding this comment

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

Weak maps only allows objects as keys, we also need to refetch the OngoingStat when calling the end() method, so we need it to persists indefinitely as there isn't a reference to pass around.

Copy link
Member

@sarah11918 sarah11918 left a comment

Choose a reason for hiding this comment

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

Just a heads up that Docs usually removes the review request to Astro Docs Maintainers ourselves! (We get pinged, so we do monitor these requests.)

We are automatically pinged whenever a README file is included in a PR because this is content that someone might... read! And even if internal, we do care about its quality.

benchmark/bench/README.md Outdated Show resolved Hide resolved
benchmark/make-project/README.md Outdated Show resolved Hide resolved
bluwy and others added 2 commits March 1, 2023 16:10
Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>
@bluwy
Copy link
Member Author

bluwy commented Mar 1, 2023

Going to merge this to unblock the memory optimizations soon. Happy to improve further if there's more changes to add. Thanks for the reviews!

@bluwy bluwy added this pull request to the merge queue Mar 1, 2023
Merged via the queue into main with commit f493794 Mar 1, 2023
@bluwy bluwy deleted the astro-bench branch March 1, 2023 08:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🚨 action Modifies GitHub Actions pkg: astro Related to the core `astro` package (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants