Skip to content

Commit 94fa6c0

Browse files
authored
[release/10.0] [mono][interp] Fix various leaks, primarily around dynamic methods (#120524)
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 - [x] Customer reported - [ ] Found internally 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 - [ ] Yes - [x] No ## 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.
1 parent c66dbae commit 94fa6c0

File tree

7 files changed

+89
-14
lines changed

7 files changed

+89
-14
lines changed

src/mono/mono/metadata/loader.c

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@
3636
#include <mono/metadata/loader-internals.h>
3737
#include <mono/metadata/class-init.h>
3838
#include <mono/metadata/class-internals.h>
39+
#include <mono/metadata/components.h>
3940
#include <mono/metadata/debug-helpers.h>
4041
#include <mono/metadata/reflection.h>
4142
#include <mono/metadata/profiler-private.h>
@@ -1371,8 +1372,11 @@ mono_free_method (MonoMethod *method)
13711372

13721373
MONO_PROFILER_RAISE (method_free, (method));
13731374

1374-
/* FIXME: This hack will go away when the profiler will support freeing methods */
1375-
if (G_UNLIKELY (mono_profiler_installed ()))
1375+
// EventPipe might require information about methods to be stored throughout
1376+
// entire app execution, so stack traces can be resolved at a later time.
1377+
// Same for debugger, we are being overly conservative
1378+
if (mono_component_event_pipe ()->component.available () ||
1379+
mono_component_debugger ()->component.available ())
13761380
return;
13771381

13781382
if (method->signature) {

src/mono/mono/mini/interp/interp-internals.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -148,6 +148,7 @@ struct InterpMethod {
148148
/* locals_size is equal to the offset of the param_area */
149149
guint32 locals_size;
150150
guint32 alloca_size;
151+
int n_data_items;
151152
int num_clauses; // clauses
152153
int transformed; // boolean
153154
unsigned int param_count;

src/mono/mono/mini/interp/interp.c

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3635,12 +3635,17 @@ interp_free_method (MonoMethod *method)
36353635

36363636
jit_mm_lock (jit_mm);
36373637

3638-
#if HOST_BROWSER
36393638
InterpMethod *imethod = (InterpMethod*)mono_internal_hash_table_lookup (&jit_mm->interp_code_hash, method);
3640-
mono_jiterp_free_method_data (method, imethod);
3639+
if (imethod) {
3640+
#if HOST_BROWSER
3641+
mono_jiterp_free_method_data (method, imethod);
36413642
#endif
36423643

3643-
mono_internal_hash_table_remove (&jit_mm->interp_code_hash, method);
3644+
mono_interp_clear_data_items_patch_sites (imethod->data_items, imethod->n_data_items);
3645+
3646+
mono_internal_hash_table_remove (&jit_mm->interp_code_hash, method);
3647+
}
3648+
36443649
jit_mm_unlock (jit_mm);
36453650

36463651
if (dmethod->mp) {

src/mono/mono/mini/interp/tiering.c

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -128,6 +128,58 @@ register_imethod_data_item (gpointer data, gpointer user_data)
128128
}
129129
}
130130

131+
void
132+
mono_interp_clear_data_items_patch_sites (gpointer *data_items, int n_data_items)
133+
{
134+
if (!enable_tiering)
135+
return;
136+
// data_items is part of the memory of a dynamic method that is being freed.
137+
// slots within this memory can be registered as patch sites for other imethods
138+
// We conservatively assume each slot could be an imethod slot, then look it up
139+
// in imethod to patch_sites hashtable. If we find it in the hashtable, we remove
140+
// the slot from the patch site list.
141+
mono_os_mutex_lock (&tiering_mutex);
142+
143+
for (int i = 0; i < n_data_items; i++) {
144+
GSList *sites;
145+
gpointer *slot = data_items + i;
146+
gpointer imethod_candidate = *slot;
147+
148+
if (dn_simdhash_ptr_ptr_try_get_value (patch_sites_table, imethod_candidate, (void **)&sites)) {
149+
GSList *prev = NULL;
150+
151+
// Remove slot from sites list
152+
if (sites->data == slot) {
153+
// If the slot is found in the first element we will also need to update the hash table since
154+
// the list head changes
155+
if (!sites->next) {
156+
g_slist_free_1 (sites);
157+
dn_simdhash_ptr_ptr_try_remove (patch_sites_table, imethod_candidate);
158+
} else {
159+
prev = sites;
160+
sites = sites->next;
161+
g_slist_free_1 (prev);
162+
dn_simdhash_ptr_ptr_try_replace_value (patch_sites_table, imethod_candidate, sites);
163+
}
164+
} else {
165+
prev = sites;
166+
sites = sites->next;
167+
while (sites != NULL) {
168+
if (sites->data == slot) {
169+
prev->next = sites->next;
170+
g_slist_free_1 (sites);
171+
// duplicates not allowed
172+
break;
173+
}
174+
prev = sites;
175+
sites = sites->next;
176+
}
177+
}
178+
}
179+
}
180+
mono_os_mutex_unlock (&tiering_mutex);
181+
}
182+
131183
void
132184
mono_interp_register_imethod_data_items (gpointer *data_items, GSList *indexes)
133185
{

src/mono/mono/mini/interp/tiering.h

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,9 @@ mono_interp_tiering_enabled (void);
1414
void
1515
mono_interp_register_imethod_data_items (gpointer *data_items, GSList *indexes);
1616

17+
void
18+
mono_interp_clear_data_items_patch_sites (gpointer *data_items, int n_data_items);
19+
1720
void
1821
mono_interp_register_imethod_patch_site (gpointer *imethod_ptr);
1922

src/mono/mono/mini/interp/transform.c

Lines changed: 18 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -3310,6 +3310,8 @@ interp_inline_newobj (TransformData *td, MonoMethod *target_method, MonoMethodSi
33103310
goto fail;
33113311
td->ip += 5;
33123312

3313+
td->headers_to_free = g_slist_prepend_mempool (td->mempool, td->headers_to_free, mheader);
3314+
33133315
push_var (td, dreg);
33143316
return TRUE;
33153317
fail:
@@ -3911,6 +3913,7 @@ interp_transform_call (TransformData *td, MonoMethod *method, MonoMethod *target
39113913
return_val_if_nok (error, FALSE);
39123914

39133915
if (interp_inline_method (td, target_method, mheader, error)) {
3916+
td->headers_to_free = g_slist_prepend_mempool (td->mempool, td->headers_to_free, mheader);
39143917
td->ip += 5;
39153918
goto done;
39163919
}
@@ -4655,7 +4658,7 @@ interp_method_compute_offsets (TransformData *td, InterpMethod *imethod, MonoMet
46554658
// 64 vars * 72 bytes = 4608 bytes. Many methods need less than this
46564659
int target_vars_capacity = num_locals + 64;
46574660

4658-
imethod->local_offsets = (guint32*)g_malloc (num_il_locals * sizeof(guint32));
4661+
imethod->local_offsets = (guint32*)imethod_alloc0 (td, num_il_locals * sizeof(guint32));
46594662
td->vars = (InterpVar*)g_malloc0 (target_vars_capacity * sizeof (InterpVar));
46604663
td->vars_size = num_locals;
46614664
td->vars_capacity = target_vars_capacity;
@@ -4753,7 +4756,7 @@ interp_method_compute_offsets (TransformData *td, InterpMethod *imethod, MonoMet
47534756
}
47544757
#endif
47554758

4756-
imethod->clause_data_offsets = (guint32*)g_malloc (header->num_clauses * sizeof (guint32));
4759+
imethod->clause_data_offsets = (guint32*)imethod_alloc0 (td, header->num_clauses * sizeof (guint32));
47574760
td->clause_vars = (int*)mono_mempool_alloc (td->mempool, sizeof (int) * header->num_clauses);
47584761
for (guint i = 0; i < header->num_clauses; i++) {
47594762
int var = interp_create_var (td, mono_get_object_type ());
@@ -9663,7 +9666,7 @@ get_native_offset (TransformData *td, int il_offset, gboolean precise)
96639666
return GPTRDIFF_TO_INT (td->new_code_end - td->new_code);
96649667
}
96659668

9666-
static void
9669+
static GSList*
96679670
generate (MonoMethod *method, MonoMethodHeader *header, InterpMethod *rtm, MonoGenericContext *generic_context, MonoError *error)
96689671
{
96699672
TransformData transform_data;
@@ -9856,12 +9859,10 @@ generate (MonoMethod *method, MonoMethodHeader *header, InterpMethod *rtm, MonoG
98569859
rtm->alloca_size = td->total_locals_size + td->max_stack_size;
98579860
g_assert ((rtm->alloca_size % MINT_STACK_ALIGNMENT) == 0);
98589861
rtm->locals_size = td->param_area_offset;
9859-
// FIXME: Can't allocate this using imethod_alloc0 as its registered with mono_interp_register_imethod_data_items ()
9860-
//rtm->data_items = (gpointer*)imethod_alloc0 (td, td->n_data_items * sizeof (td->data_items [0]));
9861-
rtm->data_items = (gpointer*)mono_mem_manager_alloc0 (td->mem_manager, td->n_data_items * sizeof (td->data_items [0]));
9862+
rtm->data_items = (gpointer*)imethod_alloc0 (td, td->n_data_items * sizeof (td->data_items [0]));
98629863
memcpy (rtm->data_items, td->data_items, td->n_data_items * sizeof (td->data_items [0]));
9864+
rtm->n_data_items = td->n_data_items;
98639865

9864-
mono_interp_register_imethod_data_items (rtm->data_items, td->imethod_items);
98659866
rtm->patchpoint_data = td->patchpoint_data;
98669867

98679868
if (td->ref_slots) {
@@ -9935,14 +9936,17 @@ generate (MonoMethod *method, MonoMethodHeader *header, InterpMethod *rtm, MonoG
99359936
g_ptr_array_free (td->seq_points, TRUE);
99369937
if (td->line_numbers)
99379938
g_array_free (td->line_numbers, TRUE);
9938-
g_slist_free (td->imethod_items);
9939+
for (GSList *l = td->headers_to_free; l; l = l->next)
9940+
mono_metadata_free_mh ((MonoMethodHeader *)l->data);
99399941
mono_mempool_destroy (td->mempool);
99409942
mono_interp_pgo_generate_end ();
99419943
if (td->retry_compilation) {
99429944
retry_compilation = TRUE;
99439945
retry_with_inlining = td->retry_with_inlining;
99449946
goto retry;
99459947
}
9948+
9949+
return td->imethod_items;
99469950
}
99479951

99489952
gboolean
@@ -10092,7 +10096,8 @@ mono_interp_transform_method (InterpMethod *imethod, ThreadContext *context, Mon
1009210096
memcpy (&tmp_imethod, imethod, sizeof (InterpMethod));
1009310097
imethod = &tmp_imethod;
1009410098

10095-
MONO_TIME_TRACK (mono_interp_stats.transform_time, generate (method, header, imethod, generic_context, error));
10099+
GSList *imethod_data_items;
10100+
MONO_TIME_TRACK (mono_interp_stats.transform_time, imethod_data_items = generate (method, header, imethod, generic_context, error));
1009610101

1009710102
mono_metadata_free_mh (header);
1009810103

@@ -10113,6 +10118,8 @@ mono_interp_transform_method (InterpMethod *imethod, ThreadContext *context, Mon
1011310118
mono_interp_stats.methods_transformed++;
1011410119
mono_atomic_fetch_add_i32 (&mono_jit_stats.methods_with_interp, 1);
1011510120

10121+
mono_interp_register_imethod_data_items (imethod->data_items, imethod_data_items);
10122+
1011610123
// FIXME Publishing of seq points seems to be racy with tiereing. We can have both tiered and untiered method
1011710124
// running at the same time. We could therefore get the optimized imethod seq points for the unoptimized method.
1011810125
gpointer seq_points = dn_simdhash_ght_get_value_or_default (jit_mm->seq_points, imethod->method);
@@ -10121,6 +10128,8 @@ mono_interp_transform_method (InterpMethod *imethod, ThreadContext *context, Mon
1012110128
}
1012210129
jit_mm_unlock (jit_mm);
1012310130

10131+
g_slist_free (imethod_data_items);
10132+
1012410133
if (mono_stats_method_desc && mono_method_desc_full_match (mono_stats_method_desc, imethod->method)) {
1012510134
g_printf ("Printing runtime stats at method: %s\n", mono_method_get_full_name (imethod->method));
1012610135
mono_runtime_print_stats ();

src/mono/mono/mini/interp/transform.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -316,6 +316,7 @@ typedef struct
316316
// FIXME: ptr_u32
317317
dn_simdhash_ptr_ptr_t *data_hash;
318318
GSList *imethod_items;
319+
GSList *headers_to_free;
319320
#ifdef ENABLE_EXPERIMENT_TIERED
320321
// FIXME: ptr_u32
321322
dn_simdhash_ptr_ptr_t *patchsite_hash;

0 commit comments

Comments
 (0)