[mono] Actually free dynamic methods, even if we have profiler attached #119749
Conversation
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.
BrzVlad
requested review from
steveisok,
thaystg and
vitek-karas
as code owners
There was a problem hiding this comment.
Pull Request Overview
This PR removes the profiler check that prevented freeing dynamic methods when a profiler was attached in Mono. The change eliminates a performance issue where dynamic methods would never be freed in environments where profilers are always installed (like macOS) even when no actual profiling is occurring.
Key changes:
- Removes the early return condition that blocked method freeing when profilers were present
- Allows dynamic methods to be properly garbage collected regardless of profiler state
github-actions
bot
added
the
needs-area-label
BrzVlad
added
area-Codegen-meta-mono
and removed
needs-area-label
|
Did you look at EventPipe profiler usage as well, it keeps some methods around, but believe they are not dynamic methods and AFAIK never acquired through profiler API's. It does iterate all methods through the JIT and Interpreter maps during rundown to get all metadata so we can symbolicate stackwalks. Question is if we will lose information about dynamic methods or maybe they never ended up in the maps to begin with. If its only dynamic method wrappers that can be freed they might not end up in stackwalks to begin with. |
|
I haven't looked into the EventPipe profiler. I was hoping you could chime in here. Everything should be good as long as the profiler calls into the runtime to obtain any relevant information and it doesn't locally hold method specific information that can be freed for dynamic methods (like MonoMethod, InterpMethod, MonoJitInfo) after the event is processed. |
|
I will take a look tomorrow, but one thing that might be problematic is if these methods end up in stack traces and then there is no methods left in our JIT/Interpreter tables so no way for clients collecting stack traces to resolve address -> method name. |
|
Are the stacktrace methods collected at a later stage ? not in realtime ? |
Yes, EventPipe will only collect function addresses in callstacks included in events, then when the session ends, it will run something called rundown that includes metadata for all functions. Rundown will run when closing an EventPipe session, so a session can start/stop at any time during runtime lifetime, collect some events (that includes callstacks) and then get needed metadata in rundown to symbolicate the information included in a specific session. This is different compared to old mono log profiler that needs to run from start and capture all method load events to have the metadata needed to symbolicate stack traces. If we include methods addresses in callstacks that have been removed once we close the session and execute rundown, then tools won't be able to symbolicate that address. |
|
So how does CoreCLR handles this ? We could be more specific by disabling freeing if EventPipe is running or something along these lines ? |
|
Haven't looked into CoreCLR details, if retain some method info even for "freed" methods or if it ends up with addresses in callstacks that can't be symbolicated. For desktop builds, EventPipe is always available, but for mobile platforms you need to include it in the build by enable diagnostic support. If this is mainly to preserve memory on mobile platforms, we should be able to check if the EventPipe component is available or not. |
|
Alternative we could potentially react to the method_free profiler callback and collect needed info inside EventPipe to make sure we can emit the needed event data, question is how much additional data we would need to allocate to hold the data, its mainly the address and enough information so we can generate the method name when emitting the rundown events. If we won't be able to generate the name later, we could do it in method_free, but it will then take up some memory in EventPipe to keep the full name of the method. |
|
I'm thinking that even if the component is present, we could check if certain profiler callbacks are actually attached or not ? And if they are not we could free these dead methods. But if the component is not enabled by default on mobile/wasm release builds, we could simplify but just checking it it is included then ? |
mono_component_event_pipe()->component.available() will give info if event pipe is included in the app or not. |
|
What about debugger? It looks like it passes ids to MonoMethod pointers back and forth between debugger and debugee. |
|
This check was originally added only for the profiler. There seem to be traces around the code of freeing debug related stuff for freed methods, so this shouldn't have been a problem in the past. @thaystg Should we be ok with freeing dynamic methods as is ? Or do we also need to disable it if debugger is present ? |
|
Debugger always creates a profiler using mono_profiler_create to track some profiler events, see mono_debugger_agent_init_internal, so I believe mono_profiler_installed will be true when debugger have been initialized. |
BrzVlad
force-pushed
the
fix-dyn-free
branch
from
6172851 to
c489062
Compare
|
/ba-g wasm-build-tests |
|
/backport to release/10.0 |
|
Started backporting to release/10.0: https://github.com/dotnet/runtime/actions/runs/17914925579 |
…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
…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
…und dynamic methods (#120600) * [mono][interp] Fix various leaks, primarily around dynamic methods (#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 * [mono][interp] Fix attempt to free tiering data when tiering is disabled (#119294) * [mono] Actually free dynamic methods, even if we have profiler attached (#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 * [mono][interp] Fix race when registering patch point data items (#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). * [mono] Enable dynfree of methods only if env var is passed If the MONO_ENABLE_DYNMETHOD_FREE is not passed, the behavior is not changed at all in order to mitigate potential risk. The old behavior was that if we have a profiler installed we don't free dynamic methods, which was always the case on maui.
2 participants
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
MonoMethodinstances 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.