Replace manual dynamic compilation with .NET Hot Reload #5041
Conversation
smoogipoo
force-pushed
the
dotnet-hot-reload
branch
from
4bb0e9e
to
28b3b5d
Compare
|
Honestly I think this is better than what we have, even with the limitations. When working on new components, you can even add override methods as long as they are done as part of the introduction of the new class (which is usually how we write them anyway). I usually hit cmd-s to trigger a framework dynamic compile, so adjusting rider shortcuts to instead to a hot reload removes any manual overhead that may be incurred:
|
|
Eh, I disagree with the override sentiment. I don't usually do that when it comes to input for example. But I don't think there is another way to resolve issues like #5039 without rethinking the current DCC and I just don't have time for that right now. |
|
Merge at your discretion, I haven't removed the assembly dropdown in the test browser yet. |
|
I still run into cases with the current dynamic compilation falls over, I'd say around 10-20% of the time I use it. I'd much rather know up front that it's not going to work, as the overhead of waiting for it to initialise then fail can be 15s (on a good PC) or over 1 minute (as reported by @frenzibyte). |
| #if NET6_0_OR_GREATER | ||
| using System.Reflection.Metadata; | ||
| using osu.Framework.Testing; | ||
|
|
There was a problem hiding this comment.
Don't ask about this newline
There was a problem hiding this comment.
I've thoroughly tested this and it's working pretty amazingly so far for me.
To summarize the limits of it:
- Can't add new classes.
- Can't override methods.
- Can't modify code in generic-typed classes.
- Can't modify value of constant fields.
And I would say I'm fine with all these limits in favour of the performance of .NET Hot Reload.
Also added to the OP that this closes ppy/osu#9225 upon checking.
However, just to note, one thing I noticed while I was testing this is that apparently, applying this arbitrary change to BeatmapCardNormal while viewing TestSceneBeatmapCard results in InvalidProgramException: Common Language Runtime detected an invalid program:
diff --git a/osu.Game/Beatmaps/Drawables/Cards/BeatmapCardNormal.cs b/osu.Game/Beatmaps/Drawables/Cards/BeatmapCardNormal.cs
index 08befd5340..4d7792697b 100644
--- a/osu.Game/Beatmaps/Drawables/Cards/BeatmapCardNormal.cs
+++ b/osu.Game/Beatmaps/Drawables/Cards/BeatmapCardNormal.cs
@@ -168,8 +168,8 @@ private void load()
Name = @"Bottom content",
RelativeSizeAxes = Axes.X,
AutoSizeAxes = Axes.Y,
- Anchor = Anchor.BottomLeft,
- Origin = Anchor.BottomLeft,
+ Anchor = Anchor.CentreLeft,
+ Origin = Anchor.CentreLeft,
Children = new Drawable[]
{
idleBottomContent = new FillFlowContainerI'm not yet sure why, nor do I have the time to look into it, so gonna leave it here for now.
| @@ -29,7 +29,7 @@ namespace osu.Framework.Testing | |||
| { | |||
| [ExcludeFromDynamicCompile] | |||
There was a problem hiding this comment.
This attribute is now no-op with the .NET Hot Reload implementation. I think we no longer need it.
There was a problem hiding this comment.
True, but I'm going to leave it as is for the time being (not even obsoleted), at least until one of us works on a new component and sees how it is in practice.
|
I can't repro the |
|
Yeah, perhaps it is a platform-specific issue? In fact, I can repro when changing anything in that class apparently. Here's the relevant logs with the exception stack trace from the patch I sent above, in case useful: |
Notably, nested classes do work. |
|
Just going to note that this is going to require osu!-side changes too, because there is one reference to the dynamic class compiler that is no longer valid after these. I was going to do some UI work this weekend anyway, so I'll give this a proper spin and see how it behaves. Generally mostly on board with this from the testimonials above, since I can corroborate that the existing setup was pretty slow and flakey for me too, so if this is both faster and has more consistent rules about what will and what won't reload cleanly then that's a big improvement in my eyes. |
|
Also, for other ideavim users that like to use ex mode, you can do: (note - that's not a typo above, that's what it's actually called), and then you can do |
|
How did you find that? I searched |
|
I changed key mappings to bind hot reload to ctrl-s, and then opened the keymap config in a text editor and looked at the action ID there 😅 |
|
So I've spent these two past days of UI dev with a local checkout of this branch. To be quite honest my feelings have cooled off quite a bit over these two days, because it's less of an improvement than I'd hoped it would be. Warning, essay incoming. In general, yes the failures are more consistent in general, but with hot reload you can get away with a lot less than the previous setup would allow. When it works it works incredibly well, but when it doesn't, it forces a full rebuild - and I don't know if it's just me, but I don't remember those being as slow as I've found them this weekend. It may be some .NET 6 thing, it may be due to having to build framework every time, it may also be due to me having resharper build fully off (because it sometimes will fail to invalidate some cache and I will see stale code running) - but either way, my full build times were about a minute every time. Which is worse in some cases than the previous state because while yes, the first dynamic compilation was always very slow, the following ones would be much quicker. So in a way, quick failures are preferable to slow surefire compilations, in some situations. Also, sometimes on things that we have in framework, failures will be silent. Examples include: adding a new All that said, the previous setup was somewhat of a maintenace nightmare, and I've started adapting to hot reload somehow by running through a mental checklist of what I may need in advance, and then adding stubs for the initial compilation that I will then use with hot reload. I think once I get a handle on being better at that, this has the potential to be better than the previous state of affairs, but I'm not personally there yet. Unfortunately that doesn't hold for general "protectedness" and inheritance usage that we have a lot of in framework and game - there a recompilation hit will need to happen almost every time. In summary I still think it's better to merge this and test this over a longer period of time. At worst we can revert to the previous state of affairs; at best, after some adapting this may turn out to be better than what we had. And given that this solution has .NET platform buy-in, maybe hot reload can itself also improve over time as the .NET team works on it? |
|
Just adding in that |
| @@ -21,7 +21,6 @@ | |||
| <ItemGroup Label="Code Analysis"> | |||
| <PackageReference Include="Microsoft.CodeAnalysis.BannedApiAnalyzers" Version="3.3.3" PrivateAssets="All" /> | |||
| <AdditionalFiles Include="$(MSBuildThisFileDirectory)CodeAnalysis\BannedSymbols.txt" /> | |||
| <PackageReference Include="Microsoft.CodeAnalysis.NetAnalyzers" Version="6.0.0" PrivateAssets="All" /> | |||
There was a problem hiding this comment.
I'm not sure about this one. Can someone without .NET 7 SDK installed try undoing this change and testing with dotnet watch specifically?
There was a problem hiding this comment.
Works fine for me
~/Projects
❯ dotnet --version
6.0.200
[performance] 2022-02-21 03:54:12 [verbose]: TextureAtlas initialised (1024x1024)
[runtime] 2022-02-21 03:54:13 [verbose]: 🔸 Step #2 HardwareDecode
[runtime] 2022-02-21 03:54:13 [verbose]: 💨 Video(TestSceneVideo) HardwareDecode
[runtime] 2022-02-21 03:54:13 [verbose]: 🔸 Step #3 Reset clock
watch : File changed: /Users/dean/Projects/osu-framework/osu.Framework.Tests/Visual/Sprites/TestSceneVideo.cs.
watch : Hot reload of changes succeeded.
[runtime] 2022-02-21 03:54:28 [verbose]: 🔸 Step #2 HardwareDecode
[runtime] 2022-02-21 03:54:28 [verbose]: 💨 Video(TestSceneVideo) HardwareDecode
[runtime] 2022-02-21 03:54:28 [verbose]: 🔸 Step #3 Reset clock
watch : File changed: /Users/dean/Projects/osu-framework/osu.Framework.Tests/Visual/Sprites/TestSceneVideo.cs.
watch : Hot reload of changes succeeded.
[runtime] 2022-02-21 03:54:35 [verbose]: 🔸 Step #2 HardwareDecode
[runtime] 2022-02-21 03:54:35 [verbose]: 💨 Video(TestSceneVideo) HardwareDecode
[runtime] 2022-02-21 03:54:35 [verbose]: 🔸 Step #3 Reset clockThere was a problem hiding this comment.
Okay, will just have to revert this then, and I'll uninstall .NET 7 locally.
There was a problem hiding this comment.
Seems like this package is not required since .NET 5: https://docs.microsoft.com/en-us/visualstudio/code-quality/migrate-from-fxcop-analyzers-to-net-analyzers?view=vs-2022, so should be okay to leave it removed as per this PR.
Closes #5039
Closes #1270
Closes #4699
Closes #4387
Closes ppy/osu#9225
Notes:
Apply Hot Reload Changesin Rider (defaults to Alt+F10).dotnet watchfrom CLI inosu.Framework.Testsdirectory will continuously hot-reload.Self-contained publish size:
Breaking changes
Visual test projects now use native .NET 6 "Hot reload"
Over the years we have maintained our own version of hot reload, affectionately named "dynamic compilation". Even after multiple complete rewrites of the system, there are still edge cases where it will unexpectedly fall over due to being too greedy in including what it considers required for the recompile.
The startup cost when running in debug was considerable (around 10-30s initialisation) and the first-compile overhead could also be high (5-60s). In addition, the dependencies required to make it work increased the final assembly size of osu!framework, even for release deploys.
With the introduction of cross-platform support for hot reload in .NET 6 we have made the decision to switch to the natively supported version.
To use this new version:
Apply Hot Reload Changesin Rider (defaults to Alt+F10) to make it quicker to apply changes.dotnet watchfrom CLI on your test project will automatically watch files and recompile when a hange is seen.New limitations:
overridemethods orclasses are not supported. You can workaround this for the common scenario of prototyping new components by creating subclasses and adding theoverridemethods before the initial hot reload.We are interested in hearing feedback on this change, especially troubling cases where the previous behaviour worked better for you. Hope is that the limitations of the new method are outweighed by the leaner assembly, better performance, and (in general) up-front error when a change can't be applied, rather than an error that can be delayed longer than it would take to run a full recompile/restart.