[mono] fix two call get_default_jit_mm make hotreload work in non default alc #84247

srxqds · 2023-04-03T12:23:14Z

hi, we found if the code is not in default alc, it's not invalidate_transformed when metadata update

clear two places // FIXME: Enumerate all memory managers

@vargaz @lambdageek @lateralusX can you take time to reivew it?

the interp_jit_info_foreach will be called by ep-rt-mono.c

…XIT_GC_UNSAFE; mono_runtime_delegate_invoke wrapper with MONO_ENTER_GC_UNSAFE/MONO_EXIT_GC_UNSAFE;

lambdageek

Thanks for the PR! This is definitly something we need to address, otherwise hot reload has no effect on methods in non-default ALCs.

The current PR introduces a lock order dependency between two low-level locks that I think will be fragile. It would be better if we could iterate over a copy of the ALCs list, while also having some way to prevent the individual ALCs from being collected until the iteration is done.

lambdageek · 2023-04-03T19:20:30Z

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

+ InterpJitInfoFuncUserData func_data;
+ func_data.func = func;
+ func_data.user_data = user_data;
+ mono_alc_foreach (interp_invalidate_transformed_func, &func_data);


typo? should be interp_jit_info_foreach_func

lambdageek · 2023-04-03T19:21:32Z

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

- jit_mm_lock (jit_mm);
- mono_internal_hash_table_apply (&jit_mm->interp_code_hash, invalidate_transform, NULL);
- jit_mm_unlock (jit_mm);
+ mono_alc_foreach (interp_invalidate_transformed_func, NULL);


This will call interp_invalidate_transformed_func while holding the alc_list_lock

lambdageek · 2023-04-03T19:23:13Z

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

+ MonoAssemblyLoadContext* alc = (MonoAssemblyLoadContext*)data;
+ MonoJitMemoryManager *jit_mm = (MonoJitMemoryManager*)(alc->memory_manager->runtime_info);
+
+ jit_mm_lock (jit_mm);


When interp_invalidate_transformed_func is called, it will take the jit memory manager lock.

So we're implicitly introducing a lock order: alcs_list_lock > MonoMemoryManager:lock

lambdageek · 2023-04-03T19:29:13Z

src/mono/mono/metadata/assembly-load-context.c

+ alcs_lock ();
+ g_slist_foreach (alcs, func, user_data);
+ alcs_unlock ();


I don't like that this creates a lock ordering dependency between the jit memory manager locks and the alcs lock. It's not particularly obvious that those locks are related, so if another piece of code takes locks in a different order, we could get a classic deadlock.

What if mono_alc_foreach made a copy of the alcs list while holding the lock, and then called g_slist_foreach on the copy?

(And also we would need to protect each ALC, if it is collectible, from being collected while we're iterating)

srxqds · 2023-04-03T20:30:25Z

@lambdageek hi, I have adjusted according to your comments, the only thing not did

while also having some way to prevent the individual ALCs from being collected until the iteration is done.

I found the alc is not cleanup now and other function like mono_alc_get_all_loaded_assemblies also not did this point.
I leave FIXME in mono_alc_get_all () function.

srxqds · 2023-04-06T01:16:16Z

hi, can you review it again? @lambdageek

srxqds · 2023-04-10T11:01:23Z

comment so you don't forget

srxqds · 2023-04-21T00:45:54Z

hi, can this got merged or anything to adjust?

vargaz · 2023-04-22T04:57:18Z

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

+ for (guint i = 0; i < alcs->len; ++i) {
+ alc = (MonoAssemblyLoadContext*)g_ptr_array_index (alcs, i);
+ MonoJitMemoryManager *jit_mm = (MonoJitMemoryManager*)(alc->memory_manager->runtime_info);
+


This could be:

MonoJitMemoryManager *jit_mm = jit_mm_for_mm (alc->memory_manager);

vargaz · 2023-04-22T05:00:32Z

Why do we need to iterate over all ALCs ?

srxqds · 2023-04-22T05:04:33Z

Why do we need to iterate over all ALCs ?

because the invalidate_transformed method should process all ALCs to make hotreload on non default ALC

srxqds added 12 commits November 24, 2021 09:57

mono_runtime_delegate_invoke wrapper with MONO_ENTER_GC_UNSAFE/MONO_E…

ceac159

…XIT_GC_UNSAFE; mono_runtime_delegate_invoke wrapper with MONO_ENTER_GC_UNSAFE/MONO_EXIT_GC_UNSAFE;

Update object.c

a56b0cf

Merge branch 'dotnet:main' into main

3de69c4

Merge branch 'dotnet:main' into main

5fcf23a

Merge branch 'dotnet:main' into main

0e0414d

Merge branch 'dotnet:main' into main

5239c87

Merge branch 'dotnet:main' into main

c6b78f6

Merge branch 'dotnet:main' into main

8b52e80

add mono_alc_foreach

60007d9

implmented mono_alc_foreach

a841d5f

Update loader-internals.h

b5725db

fix get_default_jit_mm()

47ecbf2

srxqds requested review from BrzVlad, vargaz, kotlarmilos, lambdageek and thaystg as code owners April 3, 2023 12:23

ghost added the community-contribution Indicates that the PR has been added by a community member label Apr 3, 2023

dotnet-issue-labeler bot added the area-VM-meta-mono label Apr 3, 2023

srxqds changed the title ~~[mono] fix two call get_default_jit_mm~~ [mono] fix two call get_default_jit_mm make hotreload work in non default alc Apr 3, 2023

fix compiler error

22e6366

srxqds mentioned this pull request Apr 3, 2023

fill the mono_alc_get_ambient with right behavior #62099

Closed

srxqds added 2 commits April 3, 2023 20:56

fix type cast

ca0f1f7

fix compile error

e703a9c

This was referenced Apr 3, 2023

Tracking issue for CI build timeouts #76454

Closed

WasmTestOnBrowser-System.* test failures in CI #83655

Closed

Wasm debugger test timing out #83847

Closed

[wasm] interpreter timeouts when WebSocket closes unexpectedly #84101

Closed

lambdageek added area-EnC-mono Hot Reload for WebAssembly, iOS/Android, etc and removed area-VM-meta-mono labels Apr 3, 2023

lambdageek reviewed Apr 3, 2023

View reviewed changes

srxqds added 3 commits April 4, 2023 04:19

change mono_alc_foreach to mono_alc_get_all

42df66e

Update assembly-load-context.c

b8e3d67

Update interp.c

60fd50a

fix

0d63ccf

build-analysis bot mentioned this pull request Apr 4, 2023

CI System.Text.RegularExpressions.Tests failure #84323

Closed

vargaz reviewed Apr 22, 2023

View reviewed changes

lambdageek approved these changes Apr 23, 2023

View reviewed changes

vargaz merged commit 0693424 into dotnet:main Apr 23, 2023

ghost locked as resolved and limited conversation to collaborators May 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[mono] fix two call get_default_jit_mm make hotreload work in non default alc #84247

[mono] fix two call get_default_jit_mm make hotreload work in non default alc #84247

srxqds commented Apr 3, 2023 •

edited

Loading

lambdageek left a comment

lambdageek Apr 3, 2023

lambdageek Apr 3, 2023

lambdageek Apr 3, 2023

lambdageek Apr 3, 2023

srxqds commented Apr 3, 2023

srxqds commented Apr 6, 2023

srxqds commented Apr 10, 2023

srxqds commented Apr 21, 2023

vargaz Apr 22, 2023

vargaz commented Apr 22, 2023

srxqds commented Apr 22, 2023 •

edited

Loading

[mono] fix two call get_default_jit_mm make hotreload work in non default alc #84247

[mono] fix two call get_default_jit_mm make hotreload work in non default alc #84247

Conversation

srxqds commented Apr 3, 2023 • edited Loading

lambdageek left a comment

Choose a reason for hiding this comment

lambdageek Apr 3, 2023

Choose a reason for hiding this comment

lambdageek Apr 3, 2023

Choose a reason for hiding this comment

lambdageek Apr 3, 2023

Choose a reason for hiding this comment

lambdageek Apr 3, 2023

Choose a reason for hiding this comment

srxqds commented Apr 3, 2023

srxqds commented Apr 6, 2023

srxqds commented Apr 10, 2023

srxqds commented Apr 21, 2023

vargaz Apr 22, 2023

Choose a reason for hiding this comment

vargaz commented Apr 22, 2023

srxqds commented Apr 22, 2023 • edited Loading

srxqds commented Apr 3, 2023 •

edited

Loading

srxqds commented Apr 22, 2023 •

edited

Loading