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

[mono][interp] Fix some leaks during compilation #97143

Merged
merged 2 commits into from
Jan 18, 2024

Conversation

BrzVlad
Copy link
Member

@BrzVlad BrzVlad commented Jan 18, 2024

Should fix out of memory on System.Numerics.Tensors.Tests. These seem to be sporadically failing, maybe since #94555

@ghost
Copy link

ghost commented Jan 18, 2024

Tagging subscribers to this area: @BrzVlad, @kotlarmilos
See info in area-owners.md if you want to be subscribed.

Issue Details

Should fix out of memory on System.Numerics.Tensors.Tests. These seem to be sporadically failing, maybe since #94555

Author: BrzVlad
Assignees: -
Labels:

area-Codegen-Interpreter-mono

Milestone: -

@BrzVlad
Copy link
Member Author

BrzVlad commented Jan 18, 2024

#97124

EDIT:
#96923, this android failure doesn't seem to be interpreter

@srxqds
Copy link
Contributor

srxqds commented Jan 18, 2024

#97124

EDIT: #96923, this android failure doesn't seem to be interpreter

so dotnet8.0 have this problem?

this line also have memory leak?

MonoMethodHeader *mheader = interp_method_get_header (target_method, error);

The target method and, implicitly, its signature are already inflated. This was allocating a new signature every time the method was called which was leaked. On some of the bigger tests suites, that heavily use generics, this can reduce the mem usage in the order of GBs.
The header local types are not used anywhere so we can just free it.
@BrzVlad
Copy link
Member Author

BrzVlad commented Jan 18, 2024

@srxqds The header leak is rather negligible, this header leak fix will likely not have a big impact. We still keep some data around more than needed, but it should be negligible. The method signature leak is more significant, it exists on .net8, but it is unlikely to be problematic in a typical application, since it is bounded by the number of methods that are compiled which is limited. It was showing more on our test suites, since they can be very heavy, and in tests suites you have thousands of tests that are compiled and run only once, so it was starting to show up there.

@vargaz
Copy link
Contributor

vargaz commented Jan 18, 2024

The browser-wasm linux Release LibraryTests_EAT failure is unrealted.

@vargaz vargaz merged commit 21ecb72 into dotnet:main Jan 18, 2024
108 of 111 checks passed
tmds pushed a commit to tmds/runtime that referenced this pull request Jan 23, 2024
* [mono][interp] Stop trying to inflate signature

The target method and, implicitly, its signature are already inflated. This was allocating a new signature every time the method was called which was leaked. On some of the bigger tests suites, that heavily use generics, this can reduce the mem usage in the order of GBs.

* [mono][interp] Free mheader in case of inline failure

The header local types are not used anywhere so we can just free it.
@github-actions github-actions bot locked and limited conversation to collaborators Feb 18, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants