Skip to content

Tests: Reduce memory use in ComponentTests #18527

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

Merged
merged 2 commits into from
Apr 30, 2025
Merged

Conversation

majocha
Copy link
Contributor

@majocha majocha commented Apr 30, 2025

ComponentTests allocate a lot, and the Linux CI instance can barely take it:
image

A lot of fsi sessions are created to run script tests, that's one reason.
We can cache and reuse them, on practical basis, i.e. it's not always possible and some tests must be excluded from it, but most are fine. To exclude any test that is not cooperating, just run it with singleTestBuildAndRunIsolated.

I also added metrics telemetry export in tests. OTEL export is now generally enabled by setting FSHARP_OTEL_EXPORT env var.

Before, after:
image

I need this to unblock #18499.

Copy link
Contributor

github-actions bot commented Apr 30, 2025

✅ No release notes required

@majocha majocha changed the title Experiment, reduce memory use in tests Tests: Reduce memory use in ComponentTests Apr 30, 2025
@majocha majocha marked this pull request as ready for review April 30, 2025 09:00
@majocha majocha requested a review from a team as a code owner April 30, 2025 09:00
@majocha
Copy link
Contributor Author

majocha commented Apr 30, 2025

After:
No memory warning.

Good times:
image

@majocha
Copy link
Contributor Author

majocha commented Apr 30, 2025

Underlying issue is possibly #15669.

@majocha
Copy link
Contributor Author

majocha commented Apr 30, 2025

😮‍💨 this is ready.

@edgarfgp
Copy link
Contributor

You are a Legend Sir.

Copy link
Member

@T-Gro T-Gro left a comment

Choose a reason for hiding this comment

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

Thanks for getting to the bottom of it, this memory hungriness was introduced by me.

I did when migrating "fat" tests which did full fsc.exe compilation and then ran a dotnet program.
Using a shared FSI session for tests that care about execution results saved sometimes 500ms per test just by avoiding the file IO and process overhead - at the cost of keeping stuff in that shared session and increasing memory.

Manually choosing between shared and isolated sounds good.

If we really want to dogfood and automate this "shared vs isolated" decision, we could consider using the new caching+eviction mechanisms to balance amortized startup time overhead vs. memory :)) (in practise it would be hard to predict upfront which tests will consume a lot, the level of variation between test file sizes is quite high)

@majocha
Copy link
Contributor Author

majocha commented Apr 30, 2025

Yes, getting the tests in-proc (and in memory as opposed to on disk) is generally good. This here is just a workaround and the actual issues are

  1. the memory is not reclaimed when one would reasonably expect it to be.
  2. what exactly is so costly memory-wise when creating a new fsi session?

@github-project-automation github-project-automation bot moved this from New to In Progress in F# Compiler and Tooling Apr 30, 2025
@T-Gro T-Gro merged commit cf6b53e into dotnet:main Apr 30, 2025
33 checks passed
@github-project-automation github-project-automation bot moved this from In Progress to Done in F# Compiler and Tooling Apr 30, 2025
@T-Gro
Copy link
Member

T-Gro commented Apr 30, 2025

Yes, getting the tests in-proc (and in memory as opposed to on disk) is generally good. This here is just a workaround and the actual issues are

  1. the memory is not reclaimed when one would reasonably expect it to be.
  2. what exactly is so costly memory-wise when creating a new fsi session?

One suspect could be loading of all the necessary implicit framework imports and their related objects (Types, Methods,..)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants