Use feature switch to perform hot reload trimming

[pranavkm]

Contributes to dotnet/runtime#51159

[eerhardt]

Should this bool use the same feature switch?

[eerhardt]

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

[pranavkm]

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

[eerhardt]

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.

[pranavkm]

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.

[eerhardt]

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

[marek-safar]

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.

[pranavkm]

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

[SteveSandersonMS]

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!

[SteveSandersonMS]

Nice!

[SteveSandersonMS]

@eerhardt I just added an approval here and didn't anticipate that was going to trigger an auto-merge even though there was still one of your feedback items pending: https://github.com/dotnet/aspnetcore/pull/32506/files#r629441507

Do you still want a follow-up on that one?

Auto-merge is a bit of an awkward thing if the PR isn't already approved, because it means subsequent reviewers have to (1) check if auto-merge is present and if so, (2) make sure there's no pending feedback from anyone else before marking the PR as approved.

[eerhardt]

Do you still want a follow-up on that one?

I'm fine with whatever you decide is the best approach here. I personally wouldn't have the extra entry, as I don't see any real value in it. But if you decide it is worth it, I won't block on it.

[ghost]

Hi @eerhardt. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

[pranavkm]

FYI, this was resolved. @SteveSandersonMS / @eerhardt