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

Hosting API to enable hot reload at startup #47274

Closed
lambdageek opened this issue Jan 21, 2021 · 41 comments
Closed

Hosting API to enable hot reload at startup #47274

lambdageek opened this issue Jan 21, 2021 · 41 comments
Assignees
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-Host
Milestone

Comments

@lambdageek
Copy link
Member

lambdageek commented Jan 21, 2021

Related to #44806 and #45629.

When running without a debugger attached, we need a way to tell the runtime that certain assemblies will be modified with EnC deltas. For Mono we need to do this early enough that we set the right interpreter optimization flags before we execute any managed code from the affected assemblies.

The information given to the runtime should probably include the set of assemblies that will be modified (as opposed to informing the runtime that any assembly at all could potentially be modified).

The current proposal:

Pass an environment variable DOTNET_MODIFIABLE_ASSEMBLIES with the case-insensitive keyword "debug" to enable all debug built assemblies for hot reload. This can be expanded on with other keywords like "all" or a comma separated list of assemblies when there are clear scenarios for the options.

Alternative approaches:

  1. Pass a new MODIFIABLE_ASSEMBLIES property to coreclr_initialize that includes a list of assemblies that the runtime should expect to be modified. The presence of the property would indicate that hot reload support should be enabled. We could pass the property via the .runtimeconfig.dev.json file without any modifications to the (desktop) host. Mobile and wasm workloads would need to adjust their hosts.
  2. Pass an environment variable DOTNET_MODIFIABLE_ASSEMBLIES with the same information. On mobile and wasm workloads, the hosts wouldn't need to be adjusted, MonoVM would consult the environment variable itself. On other workloads the host could interpret the environment variable or leave it to the runtimes.
@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Jan 21, 2021
@lambdageek
Copy link
Member Author

/cc @tommcdon @mikem8361 @jkotas @vargaz

@lambdageek lambdageek added api-suggestion Early API idea and discussion, it is NOT ready for implementation area-Host labels Jan 21, 2021
@ghost
Copy link

ghost commented Jan 21, 2021

Tagging subscribers to this area: @vitek-karas, @agocke
See info in area-owners.md if you want to be subscribed.

Issue Details

Related to #44806 and #45629.

When running without a debugger attached, we need a way to tell the runtime that certain assemblies will be modified with EnC deltas. For Mono we need to do this early enough that we set the right interpreter optimization flags before we execute any managed code from the affected assemblies.

The information given to the runtime should probably include the set of assemblies that will be modified (as opposed to informing the runtime that any assembly at all could potentially be modified).

There are two approaches we could take:

  1. Pass a new MODIFIABLE_ASSEMBLIES property to coreclr_initialize that includes a list of assemblies that the runtime should expect to be modified. The presence of the property would indicate that hot reload support should be enabled. This will require some changes to the host.
  2. Pass an environment variable ComPlus_ModifiableAssemblies with the same information. This could be done used by tooling without altering the host.

My loosely held opinion is that an environment variable is better.

Author: lambdageek
Assignees: -
Labels:

api-suggestion, area-Host, untriaged

Milestone: -

@jkotas
Copy link
Member

jkotas commented Jan 21, 2021

My loosely held opinion is that an environment variable is better.

It assumes that the app in EnC mode is launched by a controller that sets this environment variable for you. Is it a safe assumption to make?

ComPlus_ModifiableAssemblies

DOTNET_MODIFIABLE_ASSEMBLIES would be a better name.

@vitek-karas
Copy link
Member

Passing additional properties to the runtime through coreclr_initialize is already possible by modifying the .runtimeconfig.json or via .runtimeconfig.dev.json. Using those would not need any host changes. We could also add the env. variable support to the host for this and potentially even command line support if necessary.

In this sense it would be similar to DOTNET_STARTUP_HOOKS which is an env. variable read by the host and passed to the runtime via the runtime properties. For this one we only allow runtimeconfig and env. variable - there's no command line, at least yet.

I guess it depends on the needs in this case:

  • Do we need to support something like "lazy attach", so that the app starts without the "controller" which "attaches" much later on?
  • Do we need to support this on "random" app, that is app which was produced using any build configuration without knowing that it will be used this way?

If we only need to support this in cases where we build the app with hot reload in mind (similar to "Debug" configuration), then using the .runtimeconfig.dev.json and the runtime property would probably be a good fit - with no changes necessary to the host.

@lambdageek
Copy link
Member Author

Hmm.. using .runtimeconfig.dev.json is appealing, but I don't think the content of that file is passed along to mobile (or wasm) workloads.

@vitek-karas
Copy link
Member

Do we pass along the .runtimeconfig.json? That one would also work - if we're really doing a special build for this case.

@lambdageek
Copy link
Member Author

lambdageek commented Jan 22, 2021

Do we pass along the .runtimeconfig.json? That one would also work - if we're really doing a special build for this case.

My understanding is that at this time the mobile embedders don't bundle the json files (or otherwise make the content available). And also apparently they're not using monovm_initialize/coreclr_intialize yet.

I'm back to mildly preferring to use an environment variable.

Update: we added work items for XI and XA to make sure they call monovm_initialize xamarin/xamarin-macios#10504 and dotnet/android#5537, although that still leaves the mobile hosts with funnelling the runtimeconfig properties into the runtime.

@lambdageek
Copy link
Member Author

It assumes that the app in EnC mode is launched by a controller that sets this environment variable for you. Is it a safe assumption to make?

I think so. Hot Reload would most likely need a controller that is responsible for launching the app. (That seems the easiest way to guarantee that the controller has a consistent view of the source code, the managed assemblies and queued up EnC deltas)

@vitek-karas
Copy link
Member

Will we use managed APIs to actually do the hot-reload editing?
If true, then how are we going to inject the necessary managed code to the app?
I'm thinking that maybe something like startup hooks would be a better fit - for both reasons. It could setup the necessary communications channels (all in managed) and also tell the runtime which assemblies should be enabled for "Editing".

@lambdageek
Copy link
Member Author

Will we use managed APIs to actually do the hot-reload editing?

Still TBD - it's what I'm prototyping in Mono, but ultimately we'll have to align on a design with @tommcdon.

I'm thinking that maybe something like startup hooks would be a better fit - for both reasons. It could setup the necessary communications channels (all in managed) and also tell the runtime which assemblies should be enabled for "Editing".

I can think of reasons why that won't work right now for Mono's hot reload, but I have a harder time thinking of why this wouldn't work in principle.

Assuming we choose the right execution engine (interp, not JIT or AOT), the information that hot reload needs is: don't do inlining of any methods that could change. Which in practice means don't inline methods from a modifiable assembly. So a startup hook out to work to tell the runtime what the modifiable assemblies are. Currently it's all-or-nothing in mono - we turn off all inlining, because there's no flag for modifiable assemblies yet. But it seems reasonable to add such a flag and it shouldn't slow down the interpreter fast path unreasonably.

Setting up a communication channel through a startup hook, may work, but I'm not certain. The "main assembly" on Xamarin.iOS is the Xamarin Objective C runtime. So setting up a communication channel would have to be done without using any managed iOS functionality. I'm not sure if that's an onerous limitation. Probably it's ok. I think XA works similarly. I suppose worst case the managed startup hook could call into a native library for the bulk of the work. A managed startup hook would work on wasm, I think.

Mono and all three of those hosts don't have startup hook support right now, so that would need to be done. Xamarin and wasm will require tooling work to bundle the startup hook assemblies into the application bundles (and maybe AOT them for ios).

@vitek-karas
Copy link
Member

Slightly different way to think about it (.. my way 😉):

  • Hot reload will need a piece of custom code in the target app which facilitates the communication and edits
    • This can be either built in (into the framework/runtime) - but then it makes every app larger (unless we basically make it a feature switch so that linker can trim it and so on). This would also come with versioning restrictions (would ship with the runtime)
    • Or it can be "injected" as a separate component - which has much more flexible versioning scheme (versions with the SDK), but then it needs a way to be injected. In CoreCLR that's startup hook for the runtime part and basically a file copy for the deployment part. We would need some other mechanism for Xamarin/WASM.
  • In any case, this piece of code will need to only rely on the basic functionality provided by runtime and core framework - it needs to act as part of the underlying framework, so that it doesn't "change" the app in meaningful ways.
  • Assuming the editing in hot-reload would only apply to application code (editing the framework seems crazy honestly), then it kind of doesn't matter on which part of the framework the hot-reload piece takes a dependency on - it can depend on the entire framework. It just needs to run before the runtime loads anything from the app itself.
  • I don't understand enough about the interop on Xamarin though - if all of it is in one big assembly (both framework and app's interop), then that could be a problem.

@jkotas
Copy link
Member

jkotas commented Jan 22, 2021

I do like @vitek-karas idea of driving the whole thing via the existing startup hook mechanism.

@lambdageek
Copy link
Member Author

Ok, let's table this for now. I'll make a workitem for myself to add startup hook support for Mono, and then take a closer look at using a hook for wasm and xamarin hot reload.

Thanks!

@tmat
Copy link
Member

tmat commented Jan 27, 2021

I don't think the set of assemblies is known at startup time. It is known for each assembly just before it loads, but not necessarily at the startup.

@lambdageek
Copy link
Member Author

lambdageek commented Jan 27, 2021

I don't think the set of assemblies is known at startup time. It is known for each assembly just before it loads, but not necessarily at the startup.

It's the set of assemblies that are built from the sources in the current solution. Isn't it? An over-approximation is fine - we want the set of assemblies that could be changing.

@tmat
Copy link
Member

tmat commented Jan 27, 2021

It gets complicated. The IDE does not currently tell the debugger what assemblies are editable. The current EnC impl in CLR does not need this information at startup. If I am not mistaken, the debugger determines whether EnC should be enabled for an assembly and notifies the CLR as/before the assembly is loaded.

@lambdageek
Copy link
Member Author

It gets complicated. The IDE does not currently tell the debugger what assemblies are editable. The current EnC impl in CLR does not need this information at startup. If I am not mistaken, the debugger determines whether EnC should be enabled for an assembly and notifies the CLR as/before the assembly is loaded.

I don't think Mono needs this at startup either (other than perhaps a global "there will be hot reload in this run" flag). Just before/while the assembly is loaded should be fine for Mono, too.

How does the debugger make the determination of which assemblies are editable? Hot reload tooling would need to do something similar @tommcdon

@tmat
Copy link
Member

tmat commented Jan 27, 2021

It checks an attribute on the assembly. The compiler stamps assemblies built in DEBUG configuration with certain attributes.

@tmat
Copy link
Member

tmat commented Jan 27, 2021

DEBUG build is required for EnC to work at C#/VB compiler level.

@tommcdon
Copy link
Member

How does the debugger make the determination of which assemblies are editable? Hot reload tooling would need to do something similar

As tmat correctly pointed out there is a flag on the assembly. In coreclr we also can set COMPLUS_ForceEnc.

@mikem8361
Copy link
Member

What about if we by default to enabling hot reload/ENC on debug built modules? The DOTNET_MODIFIABLE_ASSEMBLIES env var can be used to opt-out of this default by either specifying no modules (empty list) or a list of hot reloadable modules regardless of how they are built. We will need to investigate the performance impact of enabling ENC for debug modules over just debug modules. Is there any difference in JIT codegen for ENC modules over debug built modules?

/cc: @noahfalk, @AndyAyersMS

@noahfalk
Copy link
Member

Keying on debug build configuration seems like a pretty good heuristic to me.

Also do we need to support scenarios where we don't know the full list of assemblies at startup, for example an application that will load plugins? If so that means these unpredictable assemblies can never be included in any opt-out or opt-in environment variable. Whatever the default behavior is for unlisted assemblies is what all plugins would get.

Is there any difference in JIT codegen for ENC modules over debug built modules?

Last I knew, yes there is a difference and it is controlled by the flag CORJIT_FLAG_DEBUG_EnC passed to the JIT when we compile methods in EnC assemblies. My understanding is that these differences force more regularity on stack layout and exception handling clauses which allows the debugger to do the OSR portion of EnC. If you don't need OSR you don't need to specify this flag. @AndyAyersMS probably knows the detail of the codegen differences better than I.

@jkotas
Copy link
Member

jkotas commented Feb 16, 2021

@lambdageek Is it acceptable for Mono to always enable editability for any assembly compiled as debug by default? Would it mean that assemblies compiled as debug have to always use interpreter on Mono (for now at least)?

If we were to enable all assemblies compiled as debug to be editable by default, 3rd party tools will find creative ways how to use it. For example, I can imagine diagnostic tools that act as profilers will try to use it instead of the current unmanaged profiler APIs that are very inconvenient to use. Are we happy about it, or do we want to lock it down somewhat e.g. making the debug assemblies editable by default only when an environment variable or configuration switch is set?

@lambdageek
Copy link
Member Author

lambdageek commented Feb 16, 2021

Is it acceptable for Mono to always enable editability for any assembly compiled as debug by default? Would it mean that assemblies compiled as debug have to always use interpreter on Mono (for now at least)?

Are we happy about it, or do we want to lock it down somewhat e.g. making the debug assemblies editable by default only when an environment variable or configuration switch is set?

I think the optimal situation for Mono is to have some global flag (environment variable or switch) to signal that editability is desired, and after that we can choose the editable assemblies based on the assembly debug attribute.


There are two constraints:

  1. Even if we could robustly pick JIT or interpreter as the execution engine based on whether the assembly is compiled for Debug or Release, the interpreter still has an optimization pipeline that would need to be partly or totally disabled for hot reload (but not for other debug runs).
  2. We're not very good at running with a combination of JIT and interpreter (AOT/interpreter is a supported scenario and is actively tested and maintained --- JIT/interpreter isn't). So if we unconditionally picked the execution engine based on a debug attribute - I think the effective policy in Mono would be that we would globally choose the interpreter if the main assembly is compiled in Debug mode.

@mikem8361
Copy link
Member

Then lets define an opt-in env var for debug built assemblies. A binary flag name something like DOTNET_MODIFIABLE_DEBUG_ASSEMBLIES=1. I think an environment variable is better instead of a host property because it makes it a lot easier for dotnet-watcher (or other "controller") to enable it.

The DOTNET_MODIFIABLE_ASSEMBLIES env var may also still be useful to specify assemblies that are not debug built. Not sure yet if the work to do this is worth it. @pranavkm (who is working on dotnet-watch) may have some feedback on that.

@mikem8361
Copy link
Member

DOTNET_ENABLE_MODIFIABLE_DEBUG_ASSEMBLIES might be a better name.

And DOTNET_MODIFIABLE_ASSEMBLIES would be useful for testing to always enable the test assembly or assemblies regardless of how they are built.

@jkotas
Copy link
Member

jkotas commented Feb 18, 2021

Can we have just one env variable that can be set to different values? E.g. Debug, All, etc.

@mikem8361
Copy link
Member

Ok, how about just the DOTNET_MODIFIABLE_ASSEMBLIES env var with two special case-insensitive keywords "debug" and "all" and they can be used with module paths seperated by ";".

Just debug built assemblies:
set DOTNET_MODIFIABLE_ASSEMBLIES=debug

All assemblies (except "system", "dynamic", etc.):
set DOTNET_MODIFIABLE_ASSEMBLIES=all

Debug built assemblies and a test assembly:
set DOTNET_MODIFIABLE_ASSEMBLIES=debug;c:\runtime\artifacts\bin\System.Runtime.Loader.Tests\System.Runtime.Loader.Tests.dll

I'm not sure we need "all" because we already have COMPlus_ForceEnc=1.

@jkotas
Copy link
Member

jkotas commented Feb 18, 2021

I'm not sure we need "all" because we already have COMPlus_ForceEnc=1.

This is undocumented CoreCLR-specific switch. We want something that is documented and that works the same way on both CoreCLR and Mono. I do not think we would want to add COMPlus_ForceEnc to Mono.

they can be used with module paths seperated by

Are we going to actually use this for anything in near future? I sounds like unnecessary complication to me.

@jkotas
Copy link
Member

jkotas commented Feb 18, 2021

We should also clarify how All works together with inlining. Is All going to prevent inlining by turning off optimizations for the whole process?

@mikem8361
Copy link
Member

they can be used with module paths separated by
Are we going to actually use this for anything in near future? I sounds like unnecessary complication to me.

We may want it for testing to enable just a test assembly regardless of how it is built. Hopefully @pranavkm can tell us if specifying individual modules will be useful for dotnet-watch.

@jkotas
Copy link
Member

jkotas commented Feb 18, 2021

Are we going to support hot-reload e2e for the release build configuration? If not, we should rather always build the test assemblies for this as debug.

@pranavkm
Copy link
Contributor

pranavkm commented Feb 18, 2021

dotnet-watch is aware of the set of projects / assemblies it's building. It would be fairly easy for us to specify the list of assemblies being built as a value to the env variable if that makes a difference. That said, in the ordinary case specifying Debug should suffice. We can provide UX to users telling them hot reload is being disabled because they're building their app in release if that complicates things.

@mikem8361
Copy link
Member

We should also clarify how All works together with inlining. Is All going to prevent inlining by turning off optimizations for the whole process?

I don't know if the JIT_FLAG_DEBUG_EnC (set if the module is editable) disables inlining or not. It does prepare for on stack replacement which isn't needed for hot reload but may be needed on Windows for a smooth transition from hot reload edits to debugging/ENC.

Unless it has noticeable performance impact and since it is opt-in now, I say the "All" option sets the jit ENC flag just like the COMPlus_ForceENC. To keep things simple and consistent I think the "All" option should do this for Windows, Linux and MacOS, etc.

@jkotas
Copy link
Member

jkotas commented Feb 19, 2021

the JIT_FLAG_DEBUG_EnC (set if the module is editable) disables inlining or not.

It does not.

COMPlus_ForceENC is a testing hack that does not always work correctly. If we just want a hack for testing, we can keep using COMPlus_ForceENC. If we want something that actually works well, it is going to need more work.

I think we should just enable the Debug policy for now, and worry about other policies only once we have a clear scenario for them.

@mikem8361 mikem8361 removed the untriaged New issue has not been triaged by the area owner label Feb 19, 2021
@mikem8361
Copy link
Member

Does this env var need to go through an official API review? Should I mark it api-ready-for-review?

/cc: @stephentoub

@stephentoub
Copy link
Member

Does this env var need to go through an official API review?

Today, no. Though we're getting to a point with our environment variables where I do think we should start looking at them with a view to consistency, and potentially adding a tiny more amount of process around them, not to block them but just to help with that consistency. @terrajobst, thoughts?

Related: #47283

@mikem8361
Copy link
Member

@lambdageek, have you added this env var to mono yet?

@lambdageek
Copy link
Member Author

@mikem8361 Nope, got pulled away to work on a couple other things. Hope to return to it later this week.

lambdageek added a commit that referenced this issue Mar 13, 2021
Implements #47274 for Mono.

1. `DOTNET_MODIFIABLE_ASSEMBLIES` environment variable must be set to `debug` for hot reload to be enabled (in addition to the interpreter being enabled)
2. Assemblies must be compiled in Debug mode - we check `DebuggableAttribute` for the `DisableOptimizations` bit.  If an assembly doesn't have the bit set, it can't be reloaded.
3. In the interpreter - don't try to inline when hot reload is enabled and the caller or callee has the DisableOptimizations bit set.



* [mbr] Check DOTNET_MODIFIABLE_ASSEMBLIES for hot reload support

   If it's set to "debug" (case insensitive) then hot reload is enabled for assemblies compiled in debug mode.  Otherwise hot reload is disabled

* [mbr] Disable inlining for DebuggerAttribue assemblies

   with the optimizer disabled flag

* [mbr] Update samples

* [mbr] Throw InvalidOperationException is hot reload is not supported

   On a per-assembly basis

* [mbr] Stub implementations if !ENABLE_METADATA_UPDATE
@lambdageek
Copy link
Member Author

This is now done for MonoVM and CoreCLR

@ghost ghost locked as resolved and limited conversation to collaborators Apr 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-Host
Projects
None yet
Development

No branches or pull requests

9 participants