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] Add mono hot reload support for updating parameter name #85796

Merged
merged 15 commits into from
Jun 16, 2023

Conversation

fanyang-mono
Copy link
Member

Fixes #56626
Fixes #50978

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-EnC-mono Hot Reload for WebAssembly, iOS/Android, etc label May 4, 2023
@ghost ghost assigned fanyang-mono May 4, 2023
@thaystg
Copy link
Member

thaystg commented May 4, 2023

Ohh, I think we need to support it on debugger side also.
Do you want to work on it with my help or I should add it to my TODO list?

@fanyang-mono
Copy link
Member Author

Ohh, I think we need to support it on debugger side also. Do you want to work on it with my help or I should add it to my TODO list?

Oh yeah, I will work on it with you within this PR.

@fanyang-mono
Copy link
Member Author

The current version of MicrosoftDotNetHotReloadUtilsGeneratorBuildToolVersion doesn't support modifying parameter name. When running the debugger test for this, I hit an error saying error ENC0107: Renaming parameter requires restarting the application because it is not supported by the runtime.. I would like to get this PR merged first, then sort of the debugger test, as it might take a while to get a version supporting modifying parameter name.

Copy link
Member

@lambdageek lambdageek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had a couple of suggestions to make the no-hot-reload case quicker (if there's been no updates, don't bother doing the slow lookup of the current generation)

Please:

  1. Make an issue for the weak hash table
  2. Add a test to src/libraries/System.Runtime.Loader/tests/ApplyUpdateTests.cs to do some reflection stuff with parameters
  3. Add the UpdateParameters capability to hot_reload_get_capabilities

src/mono/mono/metadata/metadata.c Outdated Show resolved Hide resolved
src/mono/mono/metadata/reflection-cache.h Outdated Show resolved Hide resolved
src/mono/mono/metadata/reflection-cache.h Outdated Show resolved Hide resolved
src/mono/mono/metadata/reflection-cache.h Outdated Show resolved Hide resolved
src/mono/mono/metadata/reflection-cache.h Outdated Show resolved Hide resolved
src/mono/mono/metadata/reflection-cache.h Show resolved Hide resolved
@lambdageek
Copy link
Member

This will update hotreload-utils to support the UpdateParameters capability in testcases dotnet/hotreload-utils#264

@fanyang-mono fanyang-mono requested a review from radical as a code owner May 9, 2023 14:51
fanyang-mono and others added 6 commits May 9, 2023 10:59
Co-authored-by: Aleksey Kliger (λgeek) <akliger@gmail.com>
Co-authored-by: Aleksey Kliger (λgeek) <akliger@gmail.com>
Co-authored-by: Aleksey Kliger (λgeek) <akliger@gmail.com>
Co-authored-by: Aleksey Kliger (λgeek) <akliger@gmail.com>
Co-authored-by: Aleksey Kliger (λgeek) <akliger@gmail.com>
@fanyang-mono
Copy link
Member Author

Had a couple of suggestions to make the no-hot-reload case quicker (if there's been no updates, don't bother doing the slow lookup of the current generation)

Please:

  1. Make an issue for the weak hash table
  2. Add a test to src/libraries/System.Runtime.Loader/tests/ApplyUpdateTests.cs to do some reflection stuff with parameters
  3. Add the UpdateParameters capability to hot_reload_get_capabilities
  1. Created an issue: [Mono] Implement mono_weak_hash_table_lookup_extended #85982
  2. Created an issue: [Mono] Add a hot reload test for modifying parameter name #85984
  3. Added in this PR

Copy link
Member

@lambdageek lambdageek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The latest changes LGTM, except you need to switch all the "default->"no invalidate" and "no invalidate"->"default"

Default: after a hot reload ApplyChanges, if you ask for a reflection object for a type, event, property, field or method, you get a new different managed object.
No invalidate: after a hot reload ApplyChanges, if you ask for a reflection object for an assembly, module, method body, parameter, etc, you get the same one as before the change - ie the cached object is not invalidated.

src/mono/mono/metadata/reflection.c Outdated Show resolved Hide resolved
src/mono/mono/metadata/reflection.c Outdated Show resolved Hide resolved
src/mono/mono/metadata/reflection.c Outdated Show resolved Hide resolved
src/mono/mono/metadata/reflection.c Outdated Show resolved Hide resolved
src/mono/mono/metadata/sre.c Outdated Show resolved Hide resolved
src/mono/mono/metadata/sre.c Outdated Show resolved Hide resolved
src/mono/mono/metadata/reflection.c Outdated Show resolved Hide resolved
src/mono/mono/metadata/reflection.c Outdated Show resolved Hide resolved
@radical
Copy link
Member

radical commented Jun 7, 2023

Wasm/windows library tests:

[19:11:43] info: [2023-06-07T19:11:43.489Z] [PASS] System.Runtime.Loader.Tests.AssemblyLoadContextTest.GetLoadContextTest_ValidTrustedPlatformAssembly
[19:11:43] warn: Running test using direct invoke
[19:11:43] warn: Applying metadata update for System.Reflection.Metadata.ApplyUpdate.Test.AddStaticLambda.dll, revision 1
[19:11:43] info: [2023-06-07T19:11:43.518Z] [PASS] System.Reflection.Metadata.ApplyUpdateTest.TestAddStaticLambda
[19:11:43] warn: 
[19:11:43] warn: Unhandled Exception:
[19:11:43] warn: StackOverflowException
[19:11:43] warn: 
[19:11:43] warn: Unhandled Exception:
[19:11:43] warn: StackOverflowException
[19:11:43] fail: Error: [MONO] * Assertion at D:/a/_work/1/s/src/mono/mono/mini/interp/interp.c:2088, condition `<disabled>' not met

    at ht (http://127.0.0.1:49220/dotnet.runtime.js:3:12501)
    at bt (http://127.0.0.1:49220/dotnet.runtime.js:3:12755)
    at wasm_trace_logger (http://127.0.0.1:49220/dotnet.native.wasm?dry_run=true:wasm-function[114]:0xc589)
    at eglib_log_adapter (http://127.0.0.1:49220/dotnet.native.wasm?dry_run=true:wasm-function[713]:0x40bc9)
    at monoeg_g_logv_nofree (http://127.0.0.1:49220/dotnet.native.wasm?dry_run=true:wasm-function[632]:0x3ea4a)
    at monoeg_assertion_message (http://127.0.0.1:49220/dotnet.native.wasm?dry_run=true:wasm-function[636]:0x3eb6b)
    at mono_assertion_message (http://127.0.0.1:49220/dotnet.native.wasm?dry_run=true:wasm-function[638]:0x3ebae)
    at mono_assertion_message_disabled (http://127.0.0.1:49220/dotnet.native.wasm?dry_run=true:wasm-function[637]:0x3eb81)
    at interp_runtime_invoke (http://127.0.0.1:49220/dotnet.native.wasm?dry_run=true:wasm-function[269]:0x1f233)
    at mono_jit_runtime_invoke (http://127.0.0.1:49220/dotnet.native.wasm?dry_run=true:wasm-function[3227]:0xe7b37)

@fanyang-mono
Copy link
Member Author

@radical Thanks for watching out this PR. I was aware of those failures. It seems that my previous fix didn't resolve the stack overflow issue completely. I was hoping to get more information from the non-wasm runs to see if they only happen on wasm or not. But CI has been quite over-booked and still haven't gotten a full run yet. Will investigate further, once the run is complete and I have a complete picture of the problem.

@lambdageek
Copy link
Member

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-EnC-mono Hot Reload for WebAssembly, iOS/Android, etc
Projects
None yet
4 participants