Skip to content

[Blazor] Apply persistence api review feedback #31147

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

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

[Blazor] Apply persistence api review feedback #31147

wants to merge 2 commits into from

Conversation

javiercn
Copy link
Member

  • Renames
    • Lifetime -> Infrastructure
      • ComponentApplicationLifetime -> ComponentStatePersistenceManager
      • ComponentApplicationState -> PersistentComponentState
      • OnPersisting -> Subscription RegisterOnPersisting(Func callback)
      • PersistState -> Persist
      • IComponentApplicationStateStore -> IComponentPersistedStateStore
  • "Spanify" APIs
    • Persist(string key, byte [] data) -> Persist(string key, Action<IBufferWriter> writer);
    • TryTake(string key out byte[] data) -> TryTake(string key, out ReadOnlySequence data)

@javiercn javiercn requested review from pranavkm and a team as code owners March 23, 2021 14:01
javiercn
javiercn commented Mar 23, 2021
View reviewed changes
javiercn
javiercn commented Mar 23, 2021
View reviewed changes
davidfowl
davidfowl reviewed Mar 23, 2021
View reviewed changes
javiercn
javiercn commented Mar 23, 2021
View reviewed changes
src/Shared/Components/PrerenderComponentApplicationStore.cs
return JsonSerializer.SerializeToUtf8Bytes(state);
// System.Text.Json doesn't support serializing ReadonlySequence<byte> so we need to buffer
// the data with a memory pool here. We will change our serialization strategy in the future here
// so that we can avoid this step.
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 serialize the dictionary directly manually since System.Text.Json doesn't support serializing ReadOnlySequence.

We do the serialization using the lower level APIs so that we can avoid having to create an extra dictionary and hold on to pooled memory. The way it works is, if the data is already in a single segment (most of the cases) we write it. If it "fits" within the stack, we copy it to a continuous buffer and write it. Otherwise, we rent a buffer from the memorypool, copy it there, write the property and return the buffer to the pool.

javiercn
javiercn commented Mar 23, 2021
View reviewed changes
@davidfowl
Copy link
Member

cc @ReubenBond it occurs to me this is very similar to orleans state persistence.

@javiercn
Copy link
Member Author

cc @ReubenBond it occurs to me this is very similar to orleans state persistence.

It's very likely we can provide a backend for circuits using Orleans. I have a conversation pending with @ReubenBond once I deep a bit more into the design for pausing and resuming circuits.

@Pilchie Pilchie added the area-blazor Includes: Blazor, Razor Components label Mar 23, 2021
@javiercn javiercn force-pushed the javiercn/apply-persistence-api-review-feedback branch from d5bb40d to 350fc7f Compare March 24, 2021 16:03
@javiercn
Copy link
Member Author

I ended up "spanifying/pooling" things all the way to the top. Here is how the current implementation behaves for about 250K for payloads with about 250K of data.

Method Entries EntrySize Mean Error StdDev Op/s Gen 0 Gen 1 Gen 2 Allocated
'Persist component state tag helper webassembly' 30 8386 919.2 us 25.44 us 75.00 us 1,087.9 - - - 17 KB

Here are some other entries and entry sizes that I've tried out to test where things break:

Method Entries EntrySize Mean Error StdDev Median Op/s Gen 0 Gen 1 Gen 2 Allocated
'Persist component state tag helper webassembly' 30 100 39.36 us 1.210 us 3.568 us 37.97 us 25,407.53 0.4883 - - 17 KB
'Persist component state tag helper webassembly' 30 8386 857.52 us 16.850 us 40.695 us 847.43 us 1,166.15 - - - 17 KB
'Persist component state tag helper webassembly' 30 100000 19,123.52 us 331.952 us 355.185 us 19,171.72 us 52.29 187.5000 187.5000 187.5000 16,579 KB
'Persist component state tag helper webassembly' 100 100 113.40 us 2.138 us 4.067 us 111.99 us 8,818.57 1.5869 - - 56 KB
'Persist component state tag helper webassembly' 100 8386 5,861.29 us 116.054 us 234.435 us 5,824.92 us 170.61 140.6250 140.6250 125.0000 5,601 KB
'Persist component state tag helper webassembly' 100 100000 68,173.85 us 1,359.268 us 3,256.719 us 68,166.04 us 14.67 250.0000 250.0000 250.0000 69,973 KB

Other improvements we could do here if we needed to are:

  • Custom array pool (bigger buffers, more buckets).
  • Chain buffers instead of copy and grow.

pranavkm
pranavkm reviewed Mar 25, 2021
View reviewed changes
SteveSandersonMS
SteveSandersonMS reviewed Mar 30, 2021
View reviewed changes
SteveSandersonMS
SteveSandersonMS reviewed Mar 30, 2021
View reviewed changes
SteveSandersonMS
SteveSandersonMS reviewed Mar 30, 2021
View reviewed changes
@SteveSandersonMS
Copy link
Member

@javiercn The use of the pooled writers comes at some long-term cost because there's now a lot of action-at-a-distance to reason about, and substantial risk of problems if we have any bugs that might leak instances in certain error cases.

I don't have a clear sense of what makes it necessarily the right way to organize it. Naively - and I know I'm probably wrong here - I would have guessed we'd just hold all the user-supplied objects in a dictionary, and then <persist-component-state> would serialize out the JSON directly to the response, without going through any other in-memory storage. Is it possible to give a brief description of why that's not sufficient?

@javiercn javiercn force-pushed the javiercn/apply-persistence-api-review-feedback branch from f171c56 to 945a4b5 Compare April 16, 2021 13:59
@javiercn javiercn force-pushed the javiercn/apply-persistence-api-review-feedback branch from 945a4b5 to a9f4f9c Compare April 16, 2021 14:46
@javiercn javiercn requested a review from dougbu as a code owner April 16, 2021 17:56
@javiercn javiercn force-pushed the javiercn/apply-persistence-api-review-feedback branch from 05f26da to 69cebf7 Compare April 19, 2021 18:13
@javiercn
Copy link
Member Author

@javiercn The use of the pooled writers comes at some long-term cost because there's now a lot of action-at-a-distance to reason about, and substantial risk of problems if we have any bugs that might leak instances in certain error cases.

I don't have a clear sense of what makes it necessarily the right way to organize it. Naively - and I know I'm probably wrong here - I would have guessed we'd just hold all the user-supplied objects in a dictionary, and then <persist-component-state> would serialize out the JSON directly to the response, without going through any other in-memory storage. Is it possible to give a brief description of why that's not sufficient?

We don't want to force things to be json serializable (they can serialize/deserialize things however they see fit, we just provide convenient json overloads) and we don't want to encourage people allocating byte arrays when they are serializing things out.

@javiercn javiercn force-pushed the javiercn/apply-persistence-api-review-feedback branch from 69cebf7 to 40da9e2 Compare August 25, 2021 19:27
@javiercn javiercn requested a review from Pilchie as a code owner August 25, 2021 19:27
@javiercn javiercn force-pushed the javiercn/apply-persistence-api-review-feedback branch 2 times, most recently from 62453cf to 37b85f4 Compare August 25, 2021 19:44
@javiercn
Copy link
Member Author

/backport to release/6.0-rc1

@github-actions
Copy link
Contributor

Started backporting to release/6.0-rc1: https://github.com/dotnet/aspnetcore/actions/runs/1168142700

@github-actions
Copy link
Contributor

@javiercn backporting to release/6.0-rc1 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: Apply API review feedback
warning: Cannot merge binary files: src/Components/Web.JS/dist/Release/blazor.webview.js (HEAD vs. Apply API review feedback)
warning: Cannot merge binary files: src/Components/Web.JS/dist/Release/blazor.server.js (HEAD vs. Apply API review feedback)
Using index info to reconstruct a base tree...
M	src/Components/Web.JS/dist/Release/blazor.server.js
M	src/Components/Web.JS/dist/Release/blazor.webview.js
M	src/Components/WebAssembly/WebAssembly/src/Hosting/WebAssemblyHostBuilder.cs
Falling back to patching base and 3-way merge...
Auto-merging src/Components/WebAssembly/WebAssembly/src/Hosting/WebAssemblyHostBuilder.cs
Auto-merging src/Components/Web.JS/dist/Release/blazor.webview.js
CONFLICT (content): Merge conflict in src/Components/Web.JS/dist/Release/blazor.webview.js
Auto-merging src/Components/Web.JS/dist/Release/blazor.server.js
CONFLICT (content): Merge conflict in src/Components/Web.JS/dist/Release/blazor.server.js
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Apply API review feedback
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

@javiercn javiercn mentioned this pull request Aug 25, 2021
Closed
Pilchie
Pilchie reviewed Aug 25, 2021
View reviewed changes
src/Components/Components/src/Infrastructure/ComponentStatePersistenceManager.cs
using System.Threading.Tasks;
using Microsoft.AspNetCore.Components.RenderTree;
using Microsoft.Extensions.Logging;

namespace Microsoft.AspNetCore.Components.Lifetime
namespace Microsoft.AspNetCore.Components.Infrastructure
Copy link
Member

Choose a reason for hiding this comment

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

Is this an existing namespace? Seems strange for something with public types in it.

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 do it in many places (to show a few).

We normally do this for things that need to be public due to layering constraints but that customers can't really interact with in a meaningful way (reference, extend or invoke APIs on) that will produce a valid outcome.

image

dougbu
dougbu reviewed Aug 25, 2021
View reviewed changes
Copy link
Contributor

@dougbu dougbu left a comment

Choose a reason for hiding this comment

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

nit

src/Components/Components/src/Microsoft.AspNetCore.Components.csproj
@@ -15,6 +15,7 @@
<Compile Include="$(ComponentsSharedSourceRoot)src\JsonSerializerOptionsProvider.cs" />
<Compile Include="$(SharedSourceRoot)LinkerFlags.cs" LinkBase="Shared" />
<Compile Include="$(SharedSourceRoot)QueryStringEnumerable.cs" LinkBase="Shared" />
<Compile Include="$(RepoRoot)src\Shared\Components\PooledByteBufferWritter.cs" LinkBase="Infrastructure" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does this single file appear in three different pseudo-folders❔ Suggest using LinkBase="Shared" everywhere

Copy link
Contributor

Choose a reason for hiding this comment

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

The inconsistency here leads me to suggest we should fix before this is merged or back-ported

@javiercn javiercn force-pushed the javiercn/apply-persistence-api-review-feedback branch from 37b85f4 to 3a03893 Compare August 26, 2021 12:21
@javiercn javiercn requested review from wtgodbe and a team as code owners August 26, 2021 12:21
@javiercn
Copy link
Member Author

We took a different path

@javiercn javiercn closed this Aug 27, 2021
@dougbu dougbu deleted the javiercn/apply-persistence-api-review-feedback branch August 30, 2021 23:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidfowl davidfowl davidfowl left review comments

@pranavkm pranavkm pranavkm left review comments

@Pilchie Pilchie Pilchie left review comments

@SteveSandersonMS SteveSandersonMS SteveSandersonMS left review comments

@captainsafia captainsafia captainsafia left review comments

@dougbu dougbu dougbu left review comments

@wtgodbe wtgodbe Awaiting requested review from wtgodbe wtgodbe is a code owner

Assignees
No one assigned
Labels
area-blazor Includes: Blazor, Razor Components
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@javiercn @davidfowl @SteveSandersonMS @pranavkm @Pilchie @captainsafia @dougbu