-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
fix(benchmark): clear BenchmarkResult.samples
array to reduce memory usage
#6541
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.
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.
@sheremet-va have we previously introduced breaking changes for experimental features in minor releases, or also on patch releases?
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.
All the time
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.
When can we expect a new release? It'd be good to clarify the release cycle here in the docs. |
@aleclarson I'm not sure if we'll have a stable release cycle (we'll discuss about it), but we just integrated pkg pr new, so you can grab one from main if you want to test it early https://github.com/vitest-dev/vitest/runs/30935774010 |
##### [v2.1.2](https://github.com/vitest-dev/vitest/releases/tag/v2.1.2) ##### 🐞 Bug Fixes - Move `Vitest.setServer` to post `configureServer` hook to enable import analysis for workspace config loading - by [@hi-ogawa](https://github.com/hi-ogawa) in vitest-dev/vitest#6584 [<samp>(e7f35)</samp>](vitest-dev/vitest@e7f35214) - **benchmark**: - Clear `BenchmarkResult.samples` array to reduce memory usage - by [@hi-ogawa](https://github.com/hi-ogawa) and [@AriPerkkio](https://github.com/AriPerkkio) in vitest-dev/vitest#6541 [<samp>(a6407)</samp>](vitest-dev/vitest@a6407afc) - **browser**: - Fix dynamic import inside worker - by [@hi-ogawa](https://github.com/hi-ogawa) in vitest-dev/vitest#6569 [<samp>(ea2d4)</samp>](vitest-dev/vitest@ea2d429b) - Fix browser mock factory event race condition - by [@hi-ogawa](https://github.com/hi-ogawa) in vitest-dev/vitest#6530 [<samp>(f131f)</samp>](vitest-dev/vitest@f131f93b) - Serve ui assets as static - by [@hi-ogawa](https://github.com/hi-ogawa) in vitest-dev/vitest#6564 [<samp>(adcda)</samp>](vitest-dev/vitest@adcdaee8) - Update solidjs testing library lib - by [@CamilleTeruel](https://github.com/CamilleTeruel) in vitest-dev/vitest#6548 [<samp>(91442)</samp>](vitest-dev/vitest@91442dfc) - Use `data:` protocol on preview provider file upload - by [@userquin](https://github.com/userquin) in vitest-dev/vitest#6501 [<samp>(e9821)</samp>](vitest-dev/vitest@e9821f70) - Fix base for client script - by [@hi-ogawa](https://github.com/hi-ogawa) in vitest-dev/vitest#6510 [<samp>(f9528)</samp>](vitest-dev/vitest@f952874e) - Throw an error if "@vitest/browser/context" is imported outside of the browser mode - by [@sheremet-va](https://github.com/sheremet-va) in vitest-dev/vitest#6570 [<samp>(383f1)</samp>](vitest-dev/vitest@383f1791) - **coverage**: - Remove empty coverage folder on test failure too - by [@AriPerkkio](https://github.com/AriPerkkio) in vitest-dev/vitest#6547 [<samp>(1371c)</samp>](vitest-dev/vitest@1371ca6a) - Include `*.astro` by default - by [@AriPerkkio](https://github.com/AriPerkkio) in vitest-dev/vitest#6565 [<samp>(f8ff7)</samp>](vitest-dev/vitest@f8ff76a9) - `cleanOnRerun: false` to invalidate previous results - by [@AriPerkkio](https://github.com/AriPerkkio) in vitest-dev/vitest#6592 [<samp>(88bde)</samp>](vitest-dev/vitest@88bde99c) - **expect**: - Fix `toBeDefined` with `expect.poll` - by [@hi-ogawa](https://github.com/hi-ogawa) in vitest-dev/vitest#6562 [<samp>(f7da6)</samp>](vitest-dev/vitest@f7da6199) - **runner**: - Mark tests as skipped when `beforeAll` failed - by [@hi-ogawa](https://github.com/hi-ogawa) in vitest-dev/vitest#6524 [<samp>(fb797)</samp>](vitest-dev/vitest@fb79792d) - Support fixture parsing of lowered async syntax - by [@hi-ogawa](https://github.com/hi-ogawa) in vitest-dev/vitest#6531 [<samp>(b553c)</samp>](vitest-dev/vitest@b553c7d6) - Fix fixture parsing of lowered async syntax for non arrow functions - by [@hi-ogawa](https://github.com/hi-ogawa) in vitest-dev/vitest#6575 [<samp>(3de00)</samp>](vitest-dev/vitest@3de00ab6) - Guard test hook callback - by [@sheremet-va](https://github.com/sheremet-va) in vitest-dev/vitest#6604 [<samp>(14971)</samp>](vitest-dev/vitest@1497134e) - Run `onTestFinished` and `onTestFailed` during `retry` and `repeats` - by [@hi-ogawa](https://github.com/hi-ogawa) in vitest-dev/vitest#6609 [<samp>(c5e29)</samp>](vitest-dev/vitest@c5e29098) - **ui**: - List tests on ui when `--standalone` - by [@hi-ogawa](https://github.com/hi-ogawa) in vitest-dev/vitest#6577 [<samp>(d0bf8)</samp>](vitest-dev/vitest@d0bf89d3) - **vite-node**: - Fix esm false-detection inside comment - by [@hi-ogawa](https://github.com/hi-ogawa) in vitest-dev/vitest#6506 [<samp>(91f85)</samp>](vitest-dev/vitest@91f85997) - **vitest**: - Install dependencies with the same version when prompted - by [@sheremet-va](https://github.com/sheremet-va) in vitest-dev/vitest#6611 [<samp>(ed8b7)</samp>](vitest-dev/vitest@ed8b7c08) - Make env.SSR consistent between different pools - by [@sheremet-va](https://github.com/sheremet-va) in vitest-dev/vitest#6616 [<samp>(8a8d3)</samp>](vitest-dev/vitest@8a8d3f03) - Don't start a websocket server if api is disabled - by [@sheremet-va](https://github.com/sheremet-va) in vitest-dev/vitest#6617 [<samp>(82140)</samp>](vitest-dev/vitest@821400b8) - **workspace**: - Fix glob pattern detection - by [@hi-ogawa](https://github.com/hi-ogawa) in vitest-dev/vitest#6502 [<samp>(7727c)</samp>](vitest-dev/vitest@7727ca87) - Ignore DS_Store by default - by [@sheremet-va](https://github.com/sheremet-va) in vitest-dev/vitest#6571 [<samp>(d2a86)</samp>](vitest-dev/vitest@d2a86ff5) ##### [View changes on GitHub](vitest-dev/vitest@v2.1.1...v2.1.2)
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:
.