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. |
hi-ogawa
changed the title
BenchmarkResult.samples array BenchmarkResult.samples array to reduce memory usage
There was a problem hiding this comment.
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.
Is this not breaking change? Though benchmarking is experimental feature 🤔
There was a problem hiding this comment.
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.
@sheremet-va have we previously introduced breaking changes for experimental features in minor releases, or also on patch releases?
Co-authored-by: Ari Perkkiö <ari.perkkio@gmail.com>
There was a problem hiding this comment.
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.
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
|
|
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.samplesby default on test runner side. I addedbenchmark.includeSamplesoption 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.yamlunless you introduce a new test example.Tests
pnpm test:ci.Documentation
pnpm run docscommand.Changesets
feat:,fix:,perf:,docs:, orchore:.