-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
fix(benchmark): clear BenchmarkResult.samples
array to reduce memory usage
#6541
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for vitest-dev ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
BenchmarkResult.samples
array BenchmarkResult.samples
array to reduce memory usage
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.
Would be great to add test case for this in https://github.com/vitest-tests/ and include it in Ecosystem CI. We already have some similar cases there. This one could be benchmark-large
.
For example the coverage-large
is there to verify that we don't run into similar OOM crash when coverage results of large project are collected. We used to store them in memory - #4603.
The reporters-large
is there to verify that Vitest doesn't get completely stuck when there's a lot of reporting to do: #4710.
@@ -24,6 +24,7 @@ export const benchmarkConfigDefaults: Required< | |||
exclude: defaultExclude, | |||
includeSource: [], | |||
reporters: ['default'], | |||
includeSamples: false, |
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.
Is this not breaking change? Though benchmarking is experimental feature 🤔
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.
Right, this is a breaking change. We could start from default true, but I'm hoping that people haven't really needed to have raw samples. For example, since my last PR #5398. benchmark outputJson
already omits raw samples.
Co-authored-by: Ari Perkkiö <ari.perkkio@gmail.com>
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.
Would be great to add test case for this in https://github.com/vitest-tests/ and include it in Ecosystem CI. We already have some similar cases there. This one could be
benchmark-large
.
Sounds good! I'll prepare one.
@@ -24,6 +24,7 @@ export const benchmarkConfigDefaults: Required< | |||
exclude: defaultExclude, | |||
includeSource: [], | |||
reporters: ['default'], | |||
includeSamples: false, |
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.
Right, this is a breaking change. We could start from default true, but I'm hoping that people haven't really needed to have raw samples. For example, since my last PR #5398. benchmark outputJson
already omits raw samples.
…o fix-benchmark-clear-samples
📝 Ran ecosystem CI: Open
|
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.
Looks good to me! I've verified that the vitest-tests/benchmark-large
crashes without this change.
Description
I think we should clear
BenchmarkResult.samples
by default on test runner side. I addedbenchmark.includeSamples
option which allows returning samples, but it's disabled by default.The fix is verified with this reproduction https://github.com/hi-ogawa/reproductions/tree/main/vitest-6539-bench-heap-oom
Plot of "used_heap_size"
Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
pnpm-lock.yaml
unless you introduce a new test example.Tests
pnpm test:ci
.Documentation
pnpm run docs
command.Changesets
feat:
,fix:
,perf:
,docs:
, orchore:
.