Skip to content

[release/8.0] [Blazor] Improvements to SSR component activation #50848

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

Merged
merged 43 commits into from
Oct 2, 2023

Conversation

MackinnonBuck
Copy link
Member

@MackinnonBuck MackinnonBuck commented Sep 20, 2023

[release/8.0] [Blazor] Improvements to SSR component activation

Improves how components rendered from a Blazor endpoint get activated for interactivity.

Description

This PR makes the following improvements:

  1. An update from a Blazor endpoint cannot supply new parameters to an existing component, unless the component specifies a formattable, deterministic @key that uniquely identifies it.
  2. The number of interactive Server root components originating from a Blazor endpoint cannot exceed the configured CircuitRootComponentOptions.MaxJSRootComponents option at any point in time.

Fixes #50849

Customer Impact

This change helps to limit the quantity of server resources used by Blazor Server interactivity and ensure correctness when dynamically supplying component parameter updates.

Regression?

  • Yes
  • No

Risk

  • High
  • Medium
  • Low

The change that limits the total number of interactive server root components is fairly low-risk, and the default limit of 100 interactive server root components can be increased for apps that need it.

However, the change to how new parameters are supplied is going to modify the behavior of Blazor Web apps built on previous .NET 8 preview releases. Furthermore, the code changes touch some of the core functionality of Blazor's new .NET 8 features. However, apps built prior to .NET 8 should continue to work the same way without any changes in behavior.

Verification

  • Manual (required)
  • Automated

Packaging changes reviewed?

  • Yes
  • No
  • N/A

@ghost ghost added the area-blazor Includes: Blazor, Razor Components label Sep 20, 2023
@MackinnonBuck
Copy link
Member Author

Some thoughts:

  1. The gap between disposing and re-initializing a component can cause the component to flicker (particularly on Blazor Server). I see a few options for addressing this:
    1. Update the core rendering logic to allow swapping which root component instance is associated with which ComponentState (so the component ID, etc. gets reused), and let the diffing algorithm handle updates without the client ever knowing that it's working with a new component instance. I haven't investigated thoroughly the feasibility or drawbacks of this.
    2. Wait to clear the DOM associated with the old component until the new component takes its place. The user could still interact with the old DOM until that transition happens. I haven't yet investigated whether drawbacks come with that approach. Technically, there already is a gap between component disposal and the DOM updating accordingly, so this might just be barely extending that gap, which I don't think would be problematic.
    3. Don't do anything and improve this further in .NET 9.
  2. We might be able to securely allow components to accept new component parameters:
    • We already allow specifying a @key to give the client a hint about how to match up components.
    • We could add the key to the data-protected payload and check on the server if the @key matches that of the existing component. If it does (and if the key is specified at all), we update the parameters of the component. Otherwise, we dispose and re-initialize the component if its parameters haven't changed.
    • We would have to think carefully about whether something like this could be done after .NET 8 without it being considered breaking.

What do you think? @SteveSandersonMS @javiercn

@SteveSandersonMS
Copy link
Member

SteveSandersonMS commented Sep 21, 2023

I'm glad you noticed the flicker and pointed it out. It is the sort of thing that would bother people a lot if it happens in common cases and there isn't a good way to eliminate it.

It seems like we can mitigate the issue in two main ways:

  1. Make interactive-root-parameter-changes less likely to happen by mistake
  2. When it is intentional, give developers a way to control it

Making it less likely to happen by mistake

It could happen a lot by mistake because people will pass object-valued parameters, and DefinitelyEquals considers those all to be "possibly changed" since arbitrary objects are mutable.

However, we know the parameters passed across a rendermode boundary are serializable by definition. So rather than the very limited, hyper-perf-sensitive logic we have in DefinitelyEquals, we could use different comparison logic:

  • If DefinitelyEquals() == true, bail out immediately - nothing to do. Or if we don't want to mess with sharing the DefinitelyEquals logic, skip this step.
  • Otherwise, compare the serialized parameters so even if you pass some arbitrary object, we can still treat it as unchanged if no changed information was going to be passed

At that point, the only times we'd ever say the parameters have changed is when they truly have, and the developer knows it.

Giving developers a mechanism for really updating parameters in place if that's important to them

Your point that we could use @key is excellent, and the more I think of it, the more important this seems. There really are valid cases for updating an interactive root's parameters in place, for example switching between pages on a grid, where each page link updates a querystring parameter.

If the default behavior is "parameter changes mean creating a new interactive root" but people can add @key to opt into retention, that seems like it solves multiple problems at once. Once there's a @key, we interpret it as the developer promising that this root component is safe to accept any set of parameters previously rendered for a root component with the same type and key.

Obviously some people will feel that it would be better still if they got retention without needing @key, but unless we're willing to accept the possibility that a random interactive root component could receive an unexpected set of parameters from another component instance, we wouldn't be able to do that.

So the rule we'd have to document/explain would be:

  • For normal components with parameter updates, we retain by default (but you can use @key force retention to happen or not happen)
  • For components at interactive roots with parameter updates, we replace by default (but you can use @key force retention to happen or not happen)

It seems manageable to me. But what does this do to the implementation cost?

@javiercn
Copy link
Member

Giving developers a mechanism for really updating parameters in place if that's important to them
Your point that we could use @key is excellent, and the more I think of it, the more important this seems. There really are valid cases for updating an interactive root's parameters in place, for example switching between pages on a grid, where each page link updates a querystring parameter.

If the default behavior is "parameter changes mean creating a new interactive root" but people can add @key to opt into retention, that seems like it solves multiple problems at once. Once there's a @key, we interpret it as the developer promising that this root component is safe to accept any set of parameters previously rendered for a root component with the same type and key.

Obviously some people will feel that it would be better still if they got retention without needing @key, but unless we're willing to accept the possibility that a random interactive root component could receive an unexpected set of parameters from another component instance, we wouldn't be able to do that.

So the rule we'd have to document/explain would be:

For normal components with parameter updates, we retain by default (but you can use @key force retention to happen or not happen)
For components at interactive roots with parameter updates, we replace by default (but you can use @key force retention to happen or not happen)
It seems manageable to me. But what does this do to the implementation cost?

We only need to use @key when there are more than one instance of a given component, isn't it? If there's only one instance, we can unambiguously pass new parameters to it. Is that not the case?
The only potential case where this could go wrong (I think) is if the component rendered after the first instance represents a different location on the render tree.

If that is the case, then using @key will have the same problems, isn't it? Unless we start requiring @key to be unique across the document (essentially making it an id) which is a whole different game.

Will we avoid the flicker if instead we wait to remove the existing component until we have the HTML for the new component ready and we swap them in one go?

Hashing the parameters seems reasonable, but it has security implications unless we use an HMAC (we can't just SHA256 the parameters, if you want to know why ask me offline)

@MackinnonBuck
Copy link
Member Author

After each batch of root component updates, the number of active Blazor Server components may not exceed the number of components rendered with the "auto" or "server" render modes in the last SSR update (prototyped, not yet in this PR).

I've taken a slightly different approach than what this comment says, and it's a different approach than what I prototyped as well. But I think it's equally valid and has some benefits over the other approach as well.

How it works:

  • Every component for the most recent SSR invocation has its component ID stored in a hash set
  • When a component gets removed, its ID gets removed from the hash set
  • When we detect that a new invocation has started, we want to guarantee that all components in the current hash set are either:
    • Updated (because they were re-rendered on the page)
    • Removed (if they were removed from the page)
  • To do this, we treat the current hash set as the hash set for the previous invocation, and start a new hash set for the new invocation. By the time all update/remove operations have occurred, we expect the preivous hash set to be empty, and the new hash set to represent the new set of SSR'd interactive components.
  • For each update operation, move the component from the old hash set to the new one.
  • For each remove operation, remove the component from the old hash set.
  • If, at this point, the old hash set still has some root components, that's a fatal error.
  • For each add operation, add the new root component to the new invocation hash set.

Benefits:

  • It doesn't require round-tripping the number of expected interactive components to the DOM and back to the circuit.
  • It avoids miscounting root components that get added some other way than via an SSR'd component from a Blazor endpoint (e.g., JS root components).
  • It could potentially allow us to automatically dispose components that the client failed to do instead of treating as an error (if we find that to be an appealing option).

I still need to add tests to verify that all scenarios work correctly, but I've drafted some formal proofs that hopefully explain why this works (although I do admit that this area is more complicated than these proofs make it out to be).

Proof 1: The total number of root components in a circuit may not exceed the number most previously rendered via Blazor endpoint.

Lemma 1: In order for the number of root components to increase, an "add" operation must occur.

Lemma 2: If the circuit currently has 0 root components, and "add" operations cannot be duplicated, then the number of root components on the circuit cannot exceed the number of "add" operations produced via Blazor endpoint.

Lemma 3: If an "add" operation occurs from an unseen endpoint invocation, all existing components must first be removed, meaning the circuit must first have 0 root components.

Lemma 4:

Suppose the circuit started with 0 root components, and all the previous lemmas are correct.

When an add operation occurs, there are three possibilities:

  1. The "add" operation comes from a previous, but not most recent endpoint invocation
  2. The "add" operation comes from the most recent endpoint invocation
  3. The "add" operation comes from a new endpoint invocation

In case 1, the add operation is invalid, because the circuit checks that the operation doesn't come from previously-seen invocation that isn't the most recent one. The circuit disconnects.

In case 2, as proven by lemma 2, the circuit has not exceeded the number of root components included in the most recent endpoint invocation, because:

  • The circuit started at 0 root components
  • The operation came from the most recent endpoint invocation
  • The operation has not been duplicated

In case 3, as described by lemma 3, all existing root components must have first been removed before the new endpoint invocation occurs. If this is not the case, the circuit disconnects. If it is the case, then do not have more root components than what was rendered by the most recent Blazor endpoint.

Since all 3 possibilities result in either the circuit disconnecting or the number of circuit components being less than or equal to the number of components rendered from the most recent Blazor endpoint, then it's not possible for the number of root components in the circuit to exceed the number of root components rendered via Blazor endpoint.

QED.

Proof 2: The client never adds any root components from a new Blazor endpoint invocation before disposing all components from the previous Blazor endpoint invocation.

Lemma 1: New circuit root components cannot be added from a new invocation until the invocation completes.

Lemma 2: Remove operations happen as soon as a new invocation starts.

Lemma 3: Update operations may happen as soon as, or any time after, an invocation starts. Update operations are equivalent to a "remove" followed by an "add". It follows that update operations can happen at the same time as remove operations, but not before.

Lemma 4:

Assume that all the previous lemmas are correct.

For an "add" operation to be sent to the circuit, there are two possibilities:

  1. It's a pure "add" operation
  2. It's happening as part of an "update" operation

In case 1, we know from lemma 1 and lemma 2 that the "add" operation must have happened after all "remove" operations.

In case 2, we know from lemma 2 and lemma 3 that "remove" operations happen at the same time or before "update" operations. It follows that "update" operations do not happen _before" remove operations, so "add" operations can't happen before "remove" operations.

Since both cases result in all "remove" operations happening before the first "add" operation, we have shown that the client never adds any root components from a new Blazor endpoint invocation before disposing all components from the previous Blazor endpoint invocation.

QED.

@MackinnonBuck
Copy link
Member Author

Otherwise, compare the serialized parameters so even if you pass some arbitrary object, we can still treat it as unchanged if no changed information was going to be passed

I like this a lot! This seems like something we should do. I'm not sure if I'll end up getting time for this, but I'll try if I do.

[About @key]... It seems manageable to me. But what does this do to the implementation cost?

The implementation cost should be fairly small. I've implemented this for Blazor Server - still need to do so for Blazor WebAssembly.

If that is the case, then using @key will have the same problems, isn't it? Unless we start requiring @key to be unique across the document (essentially making it an id) which is a whole different game.

Since adding a @key to a render mode boundary is a new scenario, could we just document this is as the behavior? I'm kind of viewing this as overloading @key with two different features rather than @key remain a single feature with consistent semantics everywhere.

Will we avoid the flicker if instead we wait to remove the existing component until we have the HTML for the new component ready and we swap them in one go?

Yep, that should work. It just adds a bit to the implementation, but it might be worth it.

So, following is the remaining work on this PR:

  • Implement @key support for WebAssembly
  • Add unit/E2E tests for everything
  • Address feedback
  • If time permits, minimize flickering by delaying replacement of interactive component DOM
  • If time permits, compare the serialized parameter values instead of their deserialized values

@MackinnonBuck
Copy link
Member Author

I've slightly changed how SSR'd components map to interactive components. The previous implementation relied on the client being able to associate SSR'd components with interactive component IDs. This was problematic because:

  1. Previously, we needed to be careful not to attempt to send an "update" or "remove" root component operation from JS to .NET until the "add" operation completes and .NET replies with the interactive component ID.
  2. Now, with the "update" operation also potentially removing and re-adding the component, we would have needed the client to delay further "update" or "remove" operations until .NET replied with the interactive component ID.

Overall, it's a bit fragile to need to keep the interactive component ID in sync between JS and .NET. Therefore, I've updated the implementation so the JS logic only has to care about its own "SSR component ID" that gets assigned as soon as the SSR'd component gets discovered. It can send root component operations in terms of the SSR component ID instead of the interactive component ID, meaning we don't have to wait for "add" or "update" operations to complete before we do further "update" or "remove" operations.

The fact that "remove" operations could have been delayed actually made invalid my "Lemma 2" in "Proof 2" in my previous comment (which stated that "remove" operations happen as soon as a new endpoint invocation starts). This new change now makes "lemma 2" true, since either:

  • We remove a component before we've even sent it to .NET yet, so we can simply stop tracking it without doing further work, or...
  • we send a "remove" operation to .NET using the synchronously allocated "SSR component ID".

However, per a discussion offline, rather than going with the approach outlined in my previous comment, we've decided to allow customers to configure a limit on the number of root components originating from a Blazor endpoint. This is a more reliable mechanism whose correctness we can be more confident in.

@MackinnonBuck
Copy link
Member Author

Remaining work:

  • Automated tests
  • Further PR feedback
  • Eliminate flickering when an "update" causes the disposal/re-instantiation of a root component

@MackinnonBuck MackinnonBuck force-pushed the mbuck/blazor-web-server-component-changes branch from 99309d3 to 4b87260 Compare September 25, 2023 17:51
@MackinnonBuck MackinnonBuck marked this pull request as ready for review September 25, 2023 17:51
@MackinnonBuck MackinnonBuck requested a review from a team as a code owner September 25, 2023 17:51
Comment on lines 35 to 39
var serverComponent = new ServerComponent(
sequence,
key,
rootComponent.Assembly.GetName().Name ?? throw new InvalidOperationException("Cannot prerender components from assemblies with a null name"),
rootComponent.FullName ?? throw new InvalidOperationException("Cannot prerender component types with a null name"),
Copy link
Member Author

Choose a reason for hiding this comment

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

The key was added to ServerComponent because it needs to be part of the data protected payload now.

Comment on lines +6 to +13
internal sealed class CircuitRootComponentOperation(RootComponentOperation operation, WebRootComponentDescriptor? descriptor = null)
{
public RootComponentOperationType Type => operation.Type;

public int SsrComponentId => operation.SsrComponentId;

public WebRootComponentDescriptor? Descriptor => descriptor;
}
Copy link
Member Author

Choose a reason for hiding this comment

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

For some background behind the reason I added this type: The way we deserialize component descriptors in Blazor Server is:

  • We have a ComponentMarker type, which contains some non-data-protected information about an SSR'd component in addition to a data-protected payload.
  • The data-protected payload from ComponentMarker gets deserialized into a ServerComponent struct.
  • Data from the ServerComponent gets upgraded to more useful information (e.g., we discover the component type and deserialize parameter values), and we put that information into ComponentDescriptor, along with other information from the ServerComponent we need post-deserialization.

With the changes in this PR, Blazor Web root components need information that wasn't in ComponentDescriptor before:

  • The Key
  • The serialized parameter values

...and they don't rely on the Sequence number being round-tripped back to the client (the order the component was initially rendered)

I created this type (and M.A.C.Server.WebRootComponentDescriptor) so that the data/work we do to instantiate root components isn't a union of the work required by both Blazor Web and pure Blazor Server scenarios. It also makes it clearer which data is intended for which scenario.

@@ -10,6 +10,5 @@ internal interface IServerComponentDeserializer
bool TryDeserializeComponentDescriptorCollection(
string serializedComponentRecords,
out List<ComponentDescriptor> descriptors);
bool TryDeserializeSingleComponentDescriptor(ComponentMarker record, [NotNullWhen(true)] out ComponentDescriptor? result);
Copy link
Member Author

Choose a reason for hiding this comment

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

This method didn't need to be on this interface

// operation.
public int? ComponentId { get; set; }
// The client side ID of the component to perform the operation on.
public int SsrComponentId { get; set; }

Copy link
Member Author

Choose a reason for hiding this comment

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

We now just use a single SsrComponentId, which simplifies the client-side logic quite a lot because it no longer needs to keep descriptors in sync with interactive component IDs.

Copy link
Member Author

Choose a reason for hiding this comment

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

I should clarify that there is a concrete motivation for doing this that relates specifically to this PR.

Since components can be removed/reinitialized upon parameter updates, that means the interactive component ID can change after the browser supplies new component parameters. This would require our JS logic to avoid supplying further parameter updates until it heard back from .NET about whether the component ID changed. This is somewhat fragile, so we would want to avoid doing this as possible.

With this improvement, the browser can supply parameter updates or initiate disposal of an interactive component even before .NET sends any data back to JS. This simplified our existing logic quite a bit, since the JS code has to worry less about asynchrony, and it saved us from complicating the logic further by having to account for the interactive component ID changing dynamically after initialization.

Comment on lines +8 to +31
#if COMPONENTS_SERVER
namespace Microsoft.AspNetCore.Components.Server.Circuits;

using Renderer = RemoteRenderer;

internal partial class RemoteRenderer
#elif COMPONENTS_WEBASSEMBLY
namespace Microsoft.AspNetCore.Components.WebAssembly.Rendering;

using Renderer = WebAssemblyRenderer;

internal partial class WebAssemblyRenderer
#else
#error WebRootComponentManager cannot be defined in this assembly.
#endif
{
private WebRootComponentManager? _webRootComponentManager;

public WebRootComponentManager GetOrCreateWebRootComponentManager()
=> _webRootComponentManager ??= new(this);

// Manages components that get added, updated, or removed in Blazor Web scenarios
// via Blazor endpoint invocations.
public sealed class WebRootComponentManager(Renderer renderer)
Copy link
Member Author

@MackinnonBuck MackinnonBuck Sep 26, 2023

Choose a reason for hiding this comment

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

I know this is a bit unusual. Normally, we would likely put this type of functionality in a base class, but we're trying to avoid changes to public API as much as possible. This class adds a WebRootComponentManager class to RemoteRenderer and WebAssemblyRenderer to manage components rendered by Blazor endpoints. It's a nested class so it can access protected members from each renderer. We also lazily instantiate the WebRootComponentManager instance for each renderer so that pure Server or WebAssembly scenarios don't have to pay the cost of the extra state.

In the future, we may want to consider pulling this into public API so we don't have to share code in this manner.

Copy link
Member

Choose a reason for hiding this comment

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

It's fine to share code in this manner if it's an implementation detail.

Comment on lines -405 to -417
this._pendingComponentsToResolve.delete(selectorId);

if (component.interactiveComponentId !== undefined) {
throw new Error('Cannot resolve a root component for the same descriptor multiple times.');
}

component.interactiveComponentId = componentId;

// The descriptor may have changed since the last call to handleUpdatedRootComponentsCore().
// We'll update this single descriptor so that the component receives the most up-to-date parameters
// or gets removed if it no longer exists on the page.
this.refreshRootComponents([component]);

Copy link
Member Author

Choose a reason for hiding this comment

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

These are the types of things we no longer need to worry about by allowing .NET to control the mapping between SSR'd components and interactive components.

Copy link
Member

Choose a reason for hiding this comment

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

Impressive

@MackinnonBuck
Copy link
Member Author

MackinnonBuck commented Sep 26, 2023

Here's an updated summary of what's implemented in this PR:

  • An SSR update can only supply parameter updates to an interactive component if a non-null/empty @key was specified
  • The "key" that we write to component markers now gets data protected for Server components
    • If any part of the component marker key gets changed (type name hash, render tree sequence number, or @key), then an exception is thrown. This further reduces the chance that matching @key values will result in the wrong parameters being supplied to the wrong component.
  • If a @key was not specified for a component:
    • An "update" operation disposes the component and instantiates a new one to take its place...
    • ...unless the component has matching parameters.
      • We compare the serialized parameter values to determine whether they match
  • Added a mechanism that prevents "flickering" caused by the disposal/re-initialization of interactive components
  • There is now a MaxInteractiveServerRootComponentCount option which limits the number of root components with an InteractiveServer render mode.
    • Defaults to 1000 components

@wtgodbe
Copy link
Member

wtgodbe commented Sep 27, 2023

@MackinnonBuck if this is still needed it should be retargeted to release/8.0

@MackinnonBuck MackinnonBuck force-pushed the mbuck/blazor-web-server-component-changes branch from af0fdf4 to c588914 Compare September 27, 2023 21:45
@MackinnonBuck MackinnonBuck requested review from wtgodbe and a team as code owners September 27, 2023 21:45
@MackinnonBuck MackinnonBuck changed the base branch from release/8.0-rc2 to release/8.0 September 27, 2023 21:45
@MackinnonBuck MackinnonBuck changed the title [release/8.0-rc2] [Blazor] Improvements to SSR component activation [release/8.0] [Blazor] Improvements to SSR component activation Sep 27, 2023
@MackinnonBuck MackinnonBuck force-pushed the mbuck/blazor-web-server-component-changes branch from 6f2efed to ca1388f Compare September 28, 2023 22:53
@mkArtakMSFT mkArtakMSFT added Servicing-approved Shiproom has approved the issue and removed Servicing-consider Shiproom approval is required for the issue labels Sep 29, 2023
@ghost
Copy link

ghost commented Sep 29, 2023

Hi @MackinnonBuck. This PR was just approved to be included in the upcoming servicing release. Somebody from the @dotnet/aspnet-build team will get it merged when the branches are open. Until then, please make sure all the CI checks pass and the PR is reviewed.

@@ -9,11 +9,12 @@ import { attachToEventDelegator as attachNavigationManagerToEventDelegator } fro
import { applyAnyDeferredValue, tryApplySpecialProperty } from './DomSpecialPropertyUtil';
const sharedTemplateElemForParsing = document.createElement('template');
const sharedSvgElemForParsing = document.createElementNS('http://www.w3.org/2000/svg', 'g');
const elementsToClearOnRootComponentRender: { [componentId: number]: LogicalElement } = {};
const elementsToClearOnRootComponentRender = new Set<LogicalElement>();
Copy link
Member

Choose a reason for hiding this comment

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

Is the reason why you no longer index by componentId just that you realised we already have the LogicalElement and hence you can look up by that instead? In other words, was this an optimization we could have done all along, or is it only possible now because of something changed in this PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's an optimization we could have done all along. But the reason I'm doing it as part of this PR is that we now reuse the "clear element on first render" logic to allow parameter updates via SSR dispose and re-initialize components without creating a flicker effect between the two operations. However, the WebRootComponentManager no longer tracks interactive component IDs, so it's just easier to work with elements instead.

function shouldPreserveContentOnInteractiveComponentDisposal(element: LogicalElement): boolean {
return element[preserveContentOnDisposalPropname] === true;
}

Copy link
Member

Choose a reason for hiding this comment

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

If this mechanism is as simple as it looks from my naive reading of it, this is a pretty neat technique.

// The output should be deterministic between endpoint invocations so that the client
// can match up component instances between renders.
// The current implementation uses the hashed component type name and its render tree
// sequence number.
Copy link
Member

@SteveSandersonMS SteveSandersonMS Sep 29, 2023

Choose a reason for hiding this comment

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

It would be slightly more intuitive to me if it used the parent component's type name and sequence number, since that uniquely identifies a point in the app's sources/binaries. (By parent, I mean the component that rendered the Component rendertree frame).

If this is using the child component's type name and its sequence number in the parent, that could theoretically clash as two different parent types could render the same child type at the same sequence number.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's an interesting idea! For some reason, I had originally thought that there might be cases where the sequence number resets to 0 within a single component, which would also create ambiguities. I'll look into changing this.

Copy link
Member Author

@MackinnonBuck MackinnonBuck Sep 29, 2023

Choose a reason for hiding this comment

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

Assuming sequence numbers are unique within a component, I think this would work. I'm just trying to figure out if there would ever be a situation where, for example, a sequence number is reused within a component, where one location correlates to one component type and another location to another component type. But I'm pretty sure the Razor compiler won't do this.

It does also make me wonder about the broader case where there's a hash collision for a component type name. We currently throw if the deserialized component type for an "update" operation doesn't match that of the existing component. Should we be instead treating that as a remove/add, similar to what we do with mismatching parameters when no @key is specified?

Copy link
Member Author

Choose a reason for hiding this comment

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

@SteveSandersonMS in your comment here, you suggested the chance of a hash collision was about 1/2^32. Is a one-in-a-million chance enough to warrant not treating a collision as a fatal error?

Copy link
Member Author

Choose a reason for hiding this comment

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

Another approach would be to use SHA256, which is what @javiercn suggested at one point.

Copy link
Member

Choose a reason for hiding this comment

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

Can we "lazily" walk up the ancestor tree and combine the info "incrementally". Meaning:

  • The first time we encounter a server root component, we calculate a hash up to the parent at each level (that we store)
  • Then if a second component gets added, we calculate the hash from the common ancestor to the unique branch of the new component.

What this means is that if we have a hierarchy

A -> B -> C -> D
A -> B -> C -> E

Where D and E are SSRed components, the first time we find D, we Compute the hash up to C (for that, we compute the hash up to B and combine it with the info from C, storing all intermediate results (In component state))

When we are about to Render E and walk up the hierarchy, we see that we already have a hash for C, and just combine it with E.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

@javiercn I like that a lot!

For the sake of minimizing further changes in this PR, do you think we can consider that as a follow-up item for 8.0 GA?

Copy link
Member

Choose a reason for hiding this comment

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

If that turns out to be very simple then it could be a nice enhancement. However it's late and there isn't room for multiple shots at this, so let's still keep erring on the side of simplicity here. We already went down a path of making it too complex for even ourselves to reason about once and then had to backtrack :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Opened #51073 to track this improvement

Copy link
Member

@SteveSandersonMS SteveSandersonMS left a comment

Choose a reason for hiding this comment

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

While there's definitely too much going on here for me to feel confident I could anticipate any strange edge cases, it looks very believable and is certainly clear you've thought this through well. The tests look good too!

Copy link
Member

@javiercn javiercn left a comment

Choose a reason for hiding this comment

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

Looks great!

@mkArtakMSFT mkArtakMSFT added * NO MERGE * Do not merge this PR as long as this label is present. and removed * NO MERGE * Do not merge this PR as long as this label is present. labels Oct 2, 2023
@mkArtakMSFT mkArtakMSFT merged commit 7a0507f into release/8.0 Oct 2, 2023
@mkArtakMSFT mkArtakMSFT deleted the mbuck/blazor-web-server-component-changes branch October 2, 2023 16:09
@ghost ghost added this to the 8.0.0 milestone Oct 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-blazor Includes: Blazor, Razor Components Servicing-approved Shiproom has approved the issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants