[release/10.0] [mono][interp] Fix various leaks, primarily around dynamic methods #120524
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…otnet#119176) * [mono][interp] Fix leaking of compilation data For dynamic methods, imethod_alloc0 allocates from the dynamic method's mempool which is freed when the method is collected. We were previously allocating only from the global memory manager. * [mono][interp] Stop leaking data items Previously we were registering imethod locations from these data items in order for them to be patched once a method is tiered up. Because of this registering, the data item had to always be around. We now free the data item for dynamic methods and also we deregister the patch locations when freeing such a method. * [mono][interp] Free headers allocated for inlined methods We add them to a list for later freeing. This uses the same pattern as jit. * [mono][interp] Skip interp free for methods not yet compiled
…ed (dotnet#119749) * [mono] Actually free dynamic methods, even if we have profiler attached Ever since the early days of mono, freeing dynamic methods was disabled if a profiler was attached. The reason for this was probably that the profiler might store `MonoMethod` instances in its own data, leading to problems if we free those instances. Looking at the profilers nowadays it is not clear where patterns like this would happen. Profilers that do store methods (like aot, coverage), don't process wrapper methods, so we should be safe since dynamic methods have the MONO_WRAPPER_DYNAMIC_METHOD wrapper type. The problem is that, nowadays, we can always have a profiler attached even if no actual profiling happens. macios for example always calls mono_profiler_install. I belive actual callbacks can be added later as necessary. * Disable freeing dynamic methods only when eventpipe or debugger are enabled
…et#119990) The code of a compiled method can use data items that contain InterpMethod* pointers. Because at the moment when a method is compiled these referenced interp methods weren't yet tiered up, we will register these data item locations to the tiering backend so that, when they do get tiered up, the locations get updated with the new InterpMethod* reference. At this moment of compilation time, we could race with other compilers of the same InterpMethod so we could end up registering redundant data_items (only the data_items for the method winning the race will actually end up being used by the executing interpreter code). Normally this is harmless, we just patch some data that never gets used. The problems start when we free interpreter methods. When the interp method is freed, we need to clear the patch_sites_table of any locations pointing into this method's data items. We do a search starting from the data items of this method. Because we have no way to reference the wrongly registered data items, these entries will stay alive in the table. As the memory gets reused, these entries will point into other runtime code, and when the get patched, it will result in corruption of memory. We fix this issue by registering the patch sites only for the method winning the compilation race (the one that first sets the transformed flag).
BrzVlad
requested review from
kotlarmilos,
steveisok,
thaystg and
vitek-karas
as code owners
There was a problem hiding this comment.
Pull Request Overview
This PR backports multiple changes from main to fix memory leaks in the Mono interpreter, particularly around dynamic methods. The leaks were causing significant memory issues in MAUI iOS applications where dynamic method execution is handled by the interpreter.
- Adds proper cleanup of method headers and data items to prevent memory leaks
- Implements patch site clearing for tiering to handle dynamic method cleanup
- Changes memory allocation strategy for interpreter data to use proper allocators
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/mono/mono/mini/interp/transform.h | Adds headers_to_free field to track method headers for cleanup |
| src/mono/mono/mini/interp/transform.c | Updates memory allocation and adds header cleanup logic |
| src/mono/mono/mini/interp/tiering.h | Adds function declaration for clearing data item patch sites |
| src/mono/mono/mini/interp/tiering.c | Implements patch site cleanup for dynamic method freeing |
| src/mono/mono/mini/interp/interp.c | Adds data item patch site clearing during method cleanup |
| src/mono/mono/mini/interp/interp-internals.h | Adds n_data_items field to track data item count |
| src/mono/mono/metadata/loader.c | Updates method freeing conditions to check for EventPipe and debugger |
|
Tagging subscribers to this area: @BrzVlad, @kotlarmilos |
|
hope can merge into 9.0.11 |
BrzVlad
added
Servicing-approved
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This backports multiple changes that were added on main in order to fix various leaks around interpreter and dynamic methods.
#119176
#119294
#119749
#119990
Customer Impact
On maui-ios, dynamic method execution is done with the interpreter. Freeing of dynamic methods was disabled for a long time and a few leaks were present in the interpreter, preventing completely freeing all the data associated with an interpreter method. Interpreter is more heavily used on maui, compared to Xamarin, so this is more likely to become a problem. On a customer application, starting and closing a workflow 14 times resulted in leaking of around 60MB of memory.
Regression
Testing
Tested on a heavy customer application that it works correctly. This path is also commonly hit as part of our libraries tests and all these changes have been in main for multiple weeks now, without any issues. This was also validated by the customer on their app, by having the fix deployed for early testing.
Risk
Low-Moderate. Most of this change is quite trivial and it just adds freeing of some data or changing the memory allocator for certain interpreter compilation data, so that it gets freed. There is a more complex part of the change, that adds freeing for data used by tiering. While this part would have a moderate risk, it is isolated to the tiering machinery, and customers have an easy way to disable tiering, effectively mitigating the risk in my opinion.