Stop considering COR_PRF_DISABLE_OPTIMIZATIONS an immutable Event Mask

[simonferquel]

Today, COR_PRF_DISABLE_OPTIMIZATIONS is defined as an immutable flag, which means you can switch it on or off after profiler initialization.
It seems quite arbitrary as the Debugger API allows to set very similar debug bits on modules dynamically and the way the JIT knows if optimizations are disabled are in this macro: https://github.com/dotnet/runtime/blob/main/src/coreclr/vm/vars.hpp#L559 which queries pretty much in the same way the debugger and profiler flags.

For Unity Core CLR editor, we have a need to be able to switch that flag on or off dynamically to improve developer experience when debugging.

[dotnet-policy-service]

Tagging subscribers to this area: @tommcdon
See info in area-owners.md if you want to be subscribed.

[simonferquel]

In Unity fork we implemented that change: #113924
I'd be happy to give it some polish and testing, and contribute it upstream

[jkotas]

we have a need to be able to switch that flag on or off dynamically to improve developer experience when debugging.

Could you please share more about how it is going to improve developer experience when debugging?

If you just switch this flag on and off, some random set of methods is going to be compiled with optimizations on and some random set of methods is going to be compiled with optimizations off. It does not sound like a great developer experience to me.

[simonferquel]

Could you please share more about how it is going to improve developer experience when debugging?

Sure! We have a toggle in Unity UI that enables / disables improved debugging support. In Unity 6 and before, it triggers a domain reload during which we configure Mono to disable jit optimizations.

With CoreCLR, we trigger a "full code reload" (which unloads both Unity Editor and User code assembly load contexts) and reload them, while setting the profiler event mask. It actually works pretty well and allow our developers to work on optimized builds while having the possibility to temporary get into a more debugger friendly state without fully restarting the editor.

[jkotas]

cc @dotnet/dotnet-diag for comments

The global switch is going to apply to both reloaded user assemblies, and methods in the non-reloaded assemblies like CoreLib that have not been JITed yet. It means we will end up with a random mix of optimized and unoptimized methods in non-reloaded assemblies. Should we have a new profiler API to disable optimizations on module load, or is this simpler change with somewhat unpredictable behavior acceptable?

[jkotas]

If we are ok withe unpredictable behavior, we may want to relax the block on other profiler flags, like COR_PRF_DISABLE_INLINING, as well.

[simonferquel]

It is acceptable for our use case (BCL in our case is loaded in default ALC) and is not reloaded. There may be cases where some BCL methods get unoptimized - if they are not jitted before the flag has been set - but it is an acceptable compromise for us. What we ultimately want is that user code and unity code are unoptimized to facilitate stepping in/out of Unity / User code.

[noahfalk]

Right now it does look like the runtime has some assumptions that within the scope of one type the optimization status should remain consistent. For example:

MethodDesc::IsJitOptimizationDisabledForAllMethodsInChunk()

I'm open to letting the profiler change the flag on the fly, but to ensure we don't put the runtime in a weird state I think we should try to do something similar to what the debugger does where the profiler has a global flag that can change at any time, then we snapshot the value of that flag each time a module loads and use the immutable module-level snapshot to drive everything else. I think that gives you what you are looking for out of the scenario while also avoiding race conditions that could cause the optimization state to change during a single JIT or across different methods in the same type.

[simonferquel]

Thanks @noahfalk for the feedback. Consistency at the module level seems a good idea to me, and the approach seems sound.
I suppose we can add an entry in the debuginfo bit flags of the module for that one.
Where should we do the flag initialization ?

[simonferquel]

I updated #113924 with:

• making disabling inlining dynamic as well
• making them taken into account on module load to keep things consistent within a module

I have no idea how to test that (I did not find any tests related to those flags, but I am not super familiar with the codebase.
Also, I think the change introduces an issue if we have casses where PROFILING_ENABLED is set and DEBUGGING_ENABLED is not. I don't know if it can ever happen.

[simonferquel]

I found a good way to test the change using EventPipe to look at MethodLoad events and validate they have the expected OptimizationLevel.
This is part of the draft PR.
I can probably introduce a similar test for Inlining control as well.