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

Consider a hot reload feature switch for linking #51159

Closed
stephentoub opened this issue Apr 13, 2021 · 12 comments
Closed

Consider a hot reload feature switch for linking #51159

stephentoub opened this issue Apr 13, 2021 · 12 comments
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-Meta blocking Marks issues that we want to fast track in order to unblock other important work linkable-framework Issues associated with delivering a linker friendly framework size-reduction Issues impacting final app size primary for size sensitive workloads
Milestone

Comments

@stephentoub
Copy link
Member

stephentoub commented Apr 13, 2021

Background and Motivation

We are going to have a variety of places across the core libraries that use MetadataUpdateHandlerAttribute to react to hot reload updates. Some of these handlers will have zero impact unless used (other than their presence in the library binaries) while others will have additional code on "normal" execution code paths, e.g. to track things that later need to be cleared. We should look at either using an existing feature switch (e.g. Debugging.IsSupported) or a new feature switch (e.g. AssemblyExtensions.IsApplyUpdateAvailable) that we tie all of these to and use to both link away and disable any expense when not required. We should aim to use the same switch across the whole stack (currently ASP.NET is using its own property).

Proposed API

namespace System.Reflection.Metadata
{
    public static class AssemblyExtensions
    {
        /// <summary>
        /// Returns true if the apply assembly update is enabled and available.
        /// </summary>
        public static bool IsApplyUpdateAvailable { get; }
    }
}

The .NET Core runtime will basically implement like this (and will cache the value in a static):

    IsApplyUpdateAvailable => Debugger.IsAttached || Environment.GetEnvironmentVariable("DOTNET_MODIFIABLE_ASSEMBLIES") != "" || Environment.GetEnvironmentVariable("COMPlus_ForceEnc") == "1";

The feature switch support in the runtime build files include this property so the conditional apply update code is elided on release builds.

Usage Examples

    if (AssemblyExtensions.IsApplyUpdateAvailable)
    {
        // Hot reload/ENC specific initialization or bookkeeping code
    }

The hot reload agent can use this to ensure that it can actually apply updates to the application. Third party libraries can use it to run some apply update specific initialization or bookkeeping code.

Risks

Low. This API is used only in very limited cases and by very few developers (mostly internal).

@stephentoub stephentoub added the size-reduction Issues impacting final app size primary for size sensitive workloads label Apr 13, 2021
@stephentoub stephentoub added this to the 6.0.0 milestone Apr 13, 2021
@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Apr 13, 2021
@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.

@ghost
Copy link

ghost commented Apr 13, 2021

Tagging subscribers to 'size-reduction': @eerhardt, @SamMonoRT, @marek-safar, @tannergooding, @CoffeeFlux
See info in area-owners.md if you want to be subscribed.

Issue Details

We are going to have a variety of places across the core libraries that use MetadataUpdateHandlerAttribute to react to hot reload updates. Some of these handlers will have zero impact unless used (other than their presence in the library binaries) while others will have additional code on "normal" execution code paths, e.g. to track things that later need to be cleared. We should look at either using an existing feature switch (e.g. Debugging.IsSupported) or a new feature switch (e.g. HotReload.IsSupported) that we tie all of these to and use to both link away and disable any expense when not required. We should aim to use the same switch across the whole stack (currently ASP.NET is using its own HotReload.IsSupported).

cc: @eerhardt, @pranavkm

Author: stephentoub
Assignees: -
Labels:

size-reduction

Milestone: 6.0.0

@ghost
Copy link

ghost commented Apr 13, 2021

Tagging subscribers to 'linkable-framework': @eerhardt, @vitek-karas, @LakshanF, @tannergooding, @sbomer
See info in area-owners.md if you want to be subscribed.

Issue Details

We are going to have a variety of places across the core libraries that use MetadataUpdateHandlerAttribute to react to hot reload updates. Some of these handlers will have zero impact unless used (other than their presence in the library binaries) while others will have additional code on "normal" execution code paths, e.g. to track things that later need to be cleared. We should look at either using an existing feature switch (e.g. Debugging.IsSupported) or a new feature switch (e.g. HotReload.IsSupported) that we tie all of these to and use to both link away and disable any expense when not required. We should aim to use the same switch across the whole stack (currently ASP.NET is using its own HotReload.IsSupported).

cc: @eerhardt, @pranavkm

Author: stephentoub
Assignees: -
Labels:

area-Meta, linkable-framework, size-reduction, untriaged

Milestone: 6.0.0

@joperezr joperezr removed the untriaged New issue has not been triaged by the area owner label Apr 13, 2021
@pranavkm
Copy link
Contributor

If I understand this correctly, would we be teaching the trimmer to remove all code paths inside a HotReload.IsSupported switch?

@SamMonoRT
Copy link
Member

@lambdageek fyi

@eerhardt
Copy link
Member

would we be teaching the trimmer to remove all code paths inside a HotReload.IsSupported switch?

If we added this switch, the trimmer would only remove code paths inside the HotReload.IsSupported switch when the switch was set to false.

There are other scenarios where the trimmer runs, but you still want to debug and hot-reload your app. One that comes to mind is Xamarin apps - See https://github.com/xamarin/xamarin-android/blob/main/Documentation/guides/OneDotNet.md#dotnet-build--publish.

@marek-safar
Copy link
Contributor

Is the logic fully detached from Debugging.IsSupported feature switch? In other words would settings like Debugging.IsSupported = false, HotReload.IsSupported = true work?

pranavkm added a commit to dotnet/aspnetcore that referenced this issue May 7, 2021
pranavkm added a commit to dotnet/aspnetcore that referenced this issue May 11, 2021
* Use feature switch to perform hot reload trimming

Contributes to dotnet/runtime#51159
pranavkm added a commit to dotnet/aspnetcore that referenced this issue May 12, 2021
* Use feature switch to perform hot reload trimming

Contributes to dotnet/runtime#51159

* Clear caches from more places on a hot reload event

* Rename variable for clarity

Co-authored-by: Steve Sanderson <SteveSandersonMS@users.noreply.github.com>
@pranavkm
Copy link
Contributor

pranavkm commented Jun 8, 2021

Do we still plan on doing something targeted here? ASP.NET Core is currently using Debugger.IsSupported for Blazor and I was planning to proliferate it in more places.

@lambdageek
Copy link
Member

lambdageek commented Jun 10, 2021

I think the need for a separate feature flag is a bit more acute with the ongoing componentization of Mono, workloads can decide whether to bundle hot reload support in some configurations or builds of the user's app.

@mikem8361 mikem8361 added api-ready-for-review API is ready for review, it is NOT ready for implementation blocking Marks issues that we want to fast track in order to unblock other important work labels Jun 21, 2021
@terrajobst
Copy link
Member

terrajobst commented Jun 22, 2021

  • At this point it seems better to make a dedicated type
    • Let's move the already approved AssemblyExtensions.ApplyUpdate()
  • Let's rename the property to IsSupported
  • We may want to leave the old APIs until preview 7 so that we keep infrastructure working
namespace System.Reflection.Metadata
{
    public static class MetadataUpdater
    {
        public static void ApplyUpdate(Assembly assembly, ReadOnlySpan<byte> metadataDelta, ReadOnlySpan<byte> ilDelta, ReadOnlySpan<byte> pdbDelta);
        public static bool IsSupported { get; }
    }
}

@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Jun 22, 2021
@mikem8361
Copy link
Member

We would need to add one more related API: GetApplyUpdateCapabilities() and probably rename it to GetCapabilities(). It is currently internal so the tests and hot reload agent/Roslyn use reflection but maybe we should make it public.

@stephentoub
Copy link
Member Author

We would need to add one more related API: GetApplyUpdateCapabilities() and probably rename it to GetCapabilities()

Yup, we discussed that, it was just left out of the summary. We landed on GetCapabilities(). We didn't discuss changing it to be public.

We also talked about how we may want to stage this by first adding the new class and changing the existing one to just delegate to it but not yet remove it, wait until the rest of the components have been updated to use the new type, and only then delete the old one (but hopefully that cycle can happen relatively quickly, as we'd want to delete the old one soon-ish).

mikem8361 added a commit to mikem8361/runtime that referenced this issue Jun 24, 2021
The old APIs AssemblyExtensions.ApplyUpdate() and AssemblyExtensions.GetApplyUpdateCapabilities() will be removed
after all the references to them have been changed to the new APIs.

Add the new IsSupported API described in issue dotnet#51159.

Change the tests to use the MetadataUpdater APIs.
mikem8361 added a commit that referenced this issue Jun 25, 2021
…54590)

Move the metadata update related APIs to the MetadataUpdater class

The old APIs AssemblyExtensions.ApplyUpdate() and AssemblyExtensions.GetApplyUpdateCapabilities() will be removed
after all the references to them have been changed to the new APIs.

Add the new IsSupported API described in issue #51159.

Change the tests to use the MetadataUpdater APIs.

Fix the ApplyUpdate qcalls and icalls

Add the ILLink substitutions for MetadataUpdater.IsSupported property

Change the old APIs to call the new ones

Update mono's MetadataUpdater.IsSupported property

Update feature switch doc

Fixed the argument checking in coreclr's MetadataUpdater.ApplyUpdate().
@ghost ghost locked as resolved and limited conversation to collaborators Jul 29, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-Meta blocking Marks issues that we want to fast track in order to unblock other important work linkable-framework Issues associated with delivering a linker friendly framework size-reduction Issues impacting final app size primary for size sensitive workloads
Projects
No open projects
Development

No branches or pull requests

9 participants