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.

Sign up for GitHub

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

Use feature switch to perform hot reload trimming #32506

Merged
merged 2 commits into from
May 11, 2021
+49 −95
Merged

Use feature switch to perform hot reload trimming #32506

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7bd65cf
Use feature switch to perform hot reload trimming
pranavkm May 7, 2021
93c3b32
Apply suggestions from code review
pranavkm May 10, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/Components/Components/src/Microsoft.AspNetCore.Components.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
<ItemGroup>
<Compile Include="$(ComponentsSharedSourceRoot)src\ArrayBuilder.cs" LinkBase="RenderTree" />
<Compile Include="$(ComponentsSharedSourceRoot)src\JsonSerializerOptionsProvider.cs" />
<Compile Include="$(ComponentsSharedSourceRoot)src\HotReloadEnvironment.cs" LinkBase="HotReload" />
<Compile Include="$(ComponentsSharedSourceRoot)src\HotReloadFeature.cs" LinkBase="HotReload" />
<Compile Include="$(SharedSourceRoot)LinkerFlags.cs" LinkBase="Shared" />
</ItemGroup>

Expand Down
14 changes: 4 additions & 10 deletions src/Components/Components/src/Properties/ILLink.Substitutions.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,17 +1,11 @@
<linker>
<assembly fullname="Microsoft.AspNetCore.Components" >
<!-- HotReload will not be available in a trimmed app. We'll attempt to aggressively remove all references to it. -->
<type fullname="Microsoft.AspNetCore.Components.RenderTree.Renderer">
<!-- Renderer.IsHotReloading will always return false in a trimmed app. -->
<method signature="System.Boolean get_IsHotReloading()" body="stub" value="false" />
<method signature="System.Void RenderRootComponentsOnHotReload()" body="remove" />
<method signature="System.Void InitializeHotReload(System.IServiceProvider)" body="stub" />
<method signature="System.Void CaptureRootComponentForHotReload(Microsoft.AspNetCore.Components.ParameterView,Microsoft.AspNetCore.Components.Rendering.ComponentState)" body="stub" />
<method signature="System.Void DisposeForHotReload()" body="stub" />
</type>
<assembly fullname="Microsoft.AspNetCore.Components">
<!-- Avoid any overhead in RenderHandle.IsHotReloading by aggressively trimming it -->
<type fullname="Microsoft.AspNetCore.Components.RenderHandle">
<method signature="System.Boolean get_IsHotReloading()" body="stub" value="false" />
Copy link
Member

Choose a reason for hiding this comment

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

Should this bool use the same feature switch?

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I see that you changed the implementation of the RenderHandle.IsHotReloading property. Maybe this xml entry can just be removed then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we'd specifically wanted to short-circuit this one quickly since it appears as part of the rendering loop. Seems easy enough to keep it

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I follow this reasoning. The body of this method will already be trimmed to just return false since

public bool IsHotReloading => HotReloadFeature.IsSupported && (_renderer?.IsHotReloading ?? false);

will evaluate to false && (_renderer?.IsHotReloading ?? false), which is always false.

Further, the only call I see to RenderHandle.IsHotReloading is here:

aspnetcore/src/Components/Components/src/ComponentBase.cs

Lines 98 to 105 in 07c6a62

protected void StateHasChanged()
{
if (_hasPendingQueuedRender)
{
return;
}
if (_hasNeverRendered || ShouldRender() || _renderHandle.IsHotReloading)

Which comes after a virtual ShouldRender() call.

So I'm not sure this entry is adding any benefit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is how it looks like in the absence of the explicit trim:

public bool IsHotReloading
{
	get
	{
		if (HotReloadFeature.IsSupported)
		{
		}
		return false;
	}
}

vs

public bool IsHotReloading => false;

StateHasChanged is part of the render-loop so it would be ideal to have it execute additional instructions it doesn't really need to.

Copy link
Member

Choose a reason for hiding this comment

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

cc @marek-safar - here's more feedback on the trimmer leaving the call to an unnecessary method after trimming. See dotnet/linker#1113

Copy link
Contributor

Choose a reason for hiding this comment

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

is part of the render-loop so it would be ideal to have it execute additional instructions it doesn't really need to.

@pranavkm the IL does not fully represent executed code. In this case, the whole method will be skipped by AOT or interpreter. We'll eventually optimize the IL as well but it's nice to have at this point because it does not affect the speed only size and the savings are small.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@SteveSandersonMS FYI since having this was your initial suggestion. I can remove the entry from the substitution file in a follow up.

Copy link
Member

Choose a reason for hiding this comment

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

Cool. I didn't realise the interpreter had the ability to do things like skip empty methods. I'd be interested to know if there's a particular way we should be able to reason about what optimizations would or wouldn't happen in interpreted and AOT modes.

In any case if there's no runtime cost to having a call to an empty method that's great, and it will be nice to remove this special case from the linker config!

</type>
<type fullname="Microsoft.AspNetCore.Components.HotReload.HotReloadFeature" feature="System.Diagnostics.Debugger.IsSupported" featurevalue="false">
<method signature="System.Boolean get_IsSupported()" body="stub" value="false" />
</type>
</assembly>
</linker>
3 changes: 2 additions & 1 deletion src/Components/Components/src/RenderHandle.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

using System;
using System.Diagnostics.CodeAnalysis;
using Microsoft.AspNetCore.Components.HotReload;
using Microsoft.AspNetCore.Components.RenderTree;

namespace Microsoft.AspNetCore.Components
Expand Down Expand Up @@ -46,7 +47,7 @@ public Dispatcher Dispatcher
/// <summary>
/// Gets a value that determines if the <see cref="Renderer"/> is triggering a render in response to a hot-reload change.
/// </summary>
public bool IsHotReloading => _renderer?.IsHotReloading ?? false;
public bool IsHotReloading => HotReloadFeature.IsSupported && (_renderer?.IsHotReloading ?? false);

/// <summary>
/// Notifies the renderer that the component should be rendered.
Expand Down
3 changes: 2 additions & 1 deletion src/Components/Components/src/RenderTree/RenderTreeDiffBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

using System;
using System.Collections.Generic;
using Microsoft.AspNetCore.Components.HotReload;
using Microsoft.AspNetCore.Components.Rendering;

namespace Microsoft.AspNetCore.Components.RenderTree
Expand Down Expand Up @@ -541,7 +542,7 @@ private static void UpdateRetainedChildComponent(
var oldParameters = new ParameterView(ParameterViewLifetime.Unbound, oldTree, oldComponentIndex);
var newParametersLifetime = new ParameterViewLifetime(diffContext.BatchBuilder);
var newParameters = new ParameterView(newParametersLifetime, newTree, newComponentIndex);
if (!newParameters.DefinitelyEquals(oldParameters) || diffContext.Renderer.IsHotReloading)
if (!newParameters.DefinitelyEquals(oldParameters) || (HotReloadFeature.IsSupported && diffContext.Renderer.IsHotReloading))
{
componentState.SetDirectParameters(newParameters);
}
Expand Down
53 changes: 13 additions & 40 deletions src/Components/Components/src/RenderTree/Renderer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
using System.Collections.Generic;
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
using System.Runtime.ExceptionServices;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.AspNetCore.Components.HotReload;
Expand Down Expand Up @@ -35,10 +34,9 @@ public abstract partial class Renderer : IDisposable, IAsyncDisposable
private readonly Dictionary<ulong, ulong> _eventHandlerIdReplacements = new Dictionary<ulong, ulong>();
private readonly ILogger<Renderer> _logger;
private readonly ComponentFactory _componentFactory;
private HotReloadEnvironment? _hotReloadEnvironment;
private List<(ComponentState, ParameterView)>? _rootComponents;

private int _nextComponentId = 0; // TODO: change to 'long' when Mono .NET->JS interop supports it
private int _nextComponentId = 0;
private bool _isBatchInProgress;
private ulong _lastEventHandlerId;
private List<Task>? _pendingTasks;
Expand Down Expand Up @@ -98,16 +96,7 @@ public Renderer(IServiceProvider serviceProvider, ILoggerFactory loggerFactory,
_logger = loggerFactory.CreateLogger<Renderer>();
_componentFactory = new ComponentFactory(componentActivator);

InitializeHotReload(serviceProvider);
}

private void InitializeHotReload(IServiceProvider serviceProvider)
{
// HotReloadEnvironment is a test-specific feature and may not be available in a running app. We'll fallback to the default instance
// if the test fixture does not provide one.
_hotReloadEnvironment = serviceProvider.GetService<HotReloadEnvironment>() ?? HotReloadEnvironment.Instance;

if (_hotReloadEnvironment.IsHotReloadEnabled)
if (HotReloadFeature.IsSupported)
{
HotReloadManager.OnDeltaApplied += RenderRootComponentsOnHotReload;
}
Expand Down Expand Up @@ -232,7 +221,13 @@ protected async Task RenderRootComponentAsync(int componentId, ParameterView ini
// During the asynchronous rendering process we want to wait up until all components have
// finished rendering so that we can produce the complete output.
var componentState = GetRequiredComponentState(componentId);
CaptureRootComponentForHotReload(initialParameters, componentState);
if (HotReloadFeature.IsSupported)
{
// when we're doing hot-reload, stash away the parameters used while rendering root components.
// We'll use this to trigger re-renders on hot reload updates.
_rootComponents ??= new();
_rootComponents.Add((componentState, initialParameters.Clone()));
}

componentState.SetDirectParameters(initialParameters);

Expand All @@ -247,20 +242,6 @@ protected async Task RenderRootComponentAsync(int componentId, ParameterView ini
}
}

/// <remarks>
/// Intentionally authored as a separate method call so we can trim this code.
/// </remarks>
private void CaptureRootComponentForHotReload(ParameterView initialParameters, ComponentState componentState)
{
if (_hotReloadEnvironment?.IsHotReloadEnabled ?? false)
{
// when we're doing hot-reload, stash away the parameters used while rendering root components.
// We'll use this to trigger re-renders on hot reload updates.
_rootComponents ??= new();
_rootComponents.Add((componentState, initialParameters.Clone()));
}
}

/// <summary>
/// Allows derived types to handle exceptions during rendering. Defaults to rethrowing the original exception.
/// </summary>
Expand Down Expand Up @@ -1011,7 +992,10 @@ public void Dispose()
/// <inheritdoc />
public async ValueTask DisposeAsync()
{
DisposeForHotReload();
if (HotReloadFeature.IsSupported)
{
HotReloadManager.OnDeltaApplied -= RenderRootComponentsOnHotReload;
}

if (_disposed)
{
Expand All @@ -1035,16 +1019,5 @@ public async ValueTask DisposeAsync()
}
}
}

/// <remarks>
/// Intentionally authored as a separate method call so we can trim this code.
/// </remarks>
private void DisposeForHotReload()
{
if (_hotReloadEnvironment?.IsHotReloadEnabled ?? false)
{
HotReloadManager.OnDeltaApplied -= RenderRootComponentsOnHotReload;
}
}
}
}
22 changes: 0 additions & 22 deletions src/Components/Shared/src/HotReloadEnvironment.cs
View file
Open in desktop

This file was deleted.

16 changes: 16 additions & 0 deletions src/Components/Shared/src/HotReloadFeature.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;

namespace Microsoft.AspNetCore.Components.HotReload
{
internal static class HotReloadFeature
{
/// <summary>
/// Gets a value that determines if hot reload is supported. Currently, the <c>Debugger.IsSupported</c> feature switch is used as a proxy for this.
/// Changing to a dedicated feature switch is tracked by https://github.com/dotnet/runtime/issues/51159.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI, this was resolved. @SteveSandersonMS / @eerhardt

/// </summary>
public static bool IsSupported { get; } = AppContext.TryGetSwitch("System.Diagnostics.Debugger.IsSupported", out var isSupported) ? isSupported : true;
}
}
13 changes: 3 additions & 10 deletions src/Components/WebAssembly/WebAssembly/src/Hosting/WebAssemblyHost.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
using System;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.AspNetCore.Components.HotReload;
using Microsoft.AspNetCore.Components.Lifetime;
using Microsoft.AspNetCore.Components.WebAssembly.HotReload;
using Microsoft.AspNetCore.Components.WebAssembly.Infrastructure;
Expand Down Expand Up @@ -144,11 +145,9 @@ internal async Task RunAsyncCore(CancellationToken cancellationToken, WebAssembl

await manager.RestoreStateAsync(store);

var initializeTask = InitializeHotReloadAsync();
if (initializeTask is not null)
if (HotReloadFeature.IsSupported)
{
// The returned value will be "null" in a trimmed app
await initializeTask;
await WebAssemblyHotReload.InitializeAsync();
}

var tcs = new TaskCompletionSource();
Expand Down Expand Up @@ -184,11 +183,5 @@ internal async Task RunAsyncCore(CancellationToken cancellationToken, WebAssembl
await tcs.Task;
}
}

private Task? InitializeHotReloadAsync()
{
// In Development scenarios, wait for hot reload to apply deltas before initiating rendering.
return WebAssemblyHotReload.InitializeAsync();
}
}
}
7 changes: 4 additions & 3 deletions src/Components/WebAssembly/WebAssembly/src/HotReload/WebAssemblyHotReload.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
using System.ComponentModel;
using System.Diagnostics;
using System.Threading.Tasks;
using Microsoft.AspNetCore.Components.HotReload;
using Microsoft.AspNetCore.Components.WebAssembly.Services;
using Microsoft.Extensions.HotReload;
using Microsoft.JSInterop;
Expand All @@ -27,7 +26,10 @@ public static class WebAssemblyHotReload

internal static async Task InitializeAsync()
{
if (!HotReloadEnvironment.Instance.IsHotReloadEnabled)
// Determine if we're running under a hot reload environment (e.g. dotnet-watch).
// It's insufficient to know it the app can be hot reloaded (HotReloadFeature.IsSupported),
// since the hot-reload agent might be unavailable.
if (Environment.GetEnvironmentVariable("DOTNET_MODIFIABLE_ASSEMBLIES") != "debug")
{
return;
}
Expand All @@ -45,7 +47,6 @@ internal static async Task InitializeAsync()
public static void ApplyHotReloadDelta(string moduleIdString, byte[] metadataDelta, byte[] ilDeta)
{
var moduleId = Guid.Parse(moduleIdString);
Debug.Assert(HotReloadEnvironment.Instance.IsHotReloadEnabled);

_updateDeltas[0].ModuleId = moduleId;
_updateDeltas[0].MetadataDelta = metadataDelta;
Expand Down
4 changes: 2 additions & 2 deletions ...Components/WebAssembly/WebAssembly/src/Microsoft.AspNetCore.Components.WebAssembly.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
<Project Sdk="Microsoft.NET.Sdk">
<Project Sdk="Microsoft.NET.Sdk">

<PropertyGroup>
<TargetFramework>$(DefaultNetCoreTargetFramework)</TargetFramework>
Expand Down Expand Up @@ -32,7 +32,7 @@
<Compile Include="$(SharedSourceRoot)Components\ComponentParameter.cs" Link="Prerendering/ComponentParameter.cs" />
<Compile Include="$(SharedSourceRoot)Components\PrerenderComponentApplicationStore.cs" />
<Compile Include="$(SharedSourceRoot)LinkerFlags.cs" LinkBase="Shared" />
<Compile Include="$(ComponentsSharedSourceRoot)src\HotReloadEnvironment.cs" LinkBase="HotReload" />
<Compile Include="$(ComponentsSharedSourceRoot)src\HotReloadFeature.cs" LinkBase="HotReload" />
</ItemGroup>

<ItemGroup>
Expand Down
5 changes: 2 additions & 3 deletions src/Components/WebAssembly/WebAssembly/src/Properties/ILLink.Substitutions.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
<linker>
<assembly fullname="Microsoft.AspNetCore.Components.WebAssembly" >
<!-- HotReload will not be available in a trimmed app. We'll attempt to aggressively remove all references to it. -->
<type fullname="Microsoft.AspNetCore.Components.WebAssembly.Hosting.WebAssemblyHost">
<method signature="System.Threading.Tasks.Task InitializeHotReloadAsync()" body="stub" />
<type fullname="Microsoft.AspNetCore.Components.HotReload.HotReloadFeature" feature="System.Diagnostics.Debugger.IsSupported" featurevalue="false">
<method signature="System.Boolean get_IsSupported()" body="stub" value="false" />
</type>
</assembly>
</linker>
2 changes: 0 additions & 2 deletions src/Components/test/testassets/TestServer/HotReloadStartup.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@

using System.Globalization;
using Microsoft.AspNetCore.Builder;
using Microsoft.AspNetCore.Components.HotReload;
using Microsoft.AspNetCore.Hosting;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Hosting;
Expand All @@ -14,7 +13,6 @@ public class HotReloadStartup
{
public void ConfigureServices(IServiceCollection services)
{
services.AddSingleton(new HotReloadEnvironment(isHotReloadEnabled: true));
services.AddControllers();
services.AddRazorPages();
services.AddServerSideBlazor();
Expand Down