[Blazor][Fixes #11964] Limit the amount of pending renders #12763

javiercn · 2019-07-31T15:41:33Z

Adds a limit to the number of unacknowledged pending batches.
After a client reaches that limit we stop procesing render requests from the components.
We still process events, but we don't update the UI.
When a client is not able to ACK renders as fast as the server produces them, the result is that the server slows down to the clients pace.
When a client stops acknowledging renders all together the server simply stops producing new UI updates.
The max memory the server will use in this situation is the size of the queued pending batches + the size of the queue of components to render.
- Components extending ComponentBase will not try to queue more than 1 render request at a time.
- Components implementing IComponent directly are responsible for not queuing successive more than one render request at a time.

javiercn · 2019-07-31T15:43:07Z

src/Components/Components/src/Rendering/Renderer.cs

@@ -423,6 +432,7 @@ private void ProcessRenderQueue()
            {
                // Ensure we catch errors while running the render functions of the components.
                HandleException(e);
+                return;


This was a bug. After an exception we should stop everything immediately and not run anything else, like the code after this try..catch..finally block.

src/Components/Server/src/Circuits/RemoteRenderer.cs

rynowak · 2019-08-01T00:59:34Z

src/Components/Server/src/Circuits/RemoteRenderer.cs

+                // Invke ProcessBufferedRenderRequests so that we might produce any additional batch that is
+                // missing.
+                // Its also safe to use the discard as ProcessRenderQueue won't throw.
+                _ = Dispatcher.InvokeAsync(() => ProcessRenderQueue());


Why do we need to dispatch here? None of the above logic is thread-safe.

Basically you either have the sync context or you don't 👍

If you do InvokeAsync should just run the code inline

If you don't then a lot of this code seems unsafe.

I agree no need to use InvokeAsync.

Also, calling ProcessRenderQueue does a bunch of work that is almost always unnecessary in this case. Can I suggest you put a guard clause at the top of ProcessRenderQueue so it just returns if _batchBuilder.ComponentRenderQueue.Count == 0? Then we can also remove the existing check for that condition from the bottom of ProcessRenderQueue itself.

Why do we need to dispatch here? None of the above logic is thread-safe.

Component renders must run inside the sync context, as components require a sync context when they add to the render queue, which happens as part of processing the queue.
We also rely on the sync context to serialize render processing (meaning only 1 thread at a time operates on the queue and produces a new render). That's why its a queue and not a concurrent queue and why we don't have Interlocked.Exchange all over the place.

If you don't then a lot of this code seems unsafe.

Acknowledging the batch can happen outside of the sync context because of the comment mentioned above regarding signalr dispatching only 1 message a time and because the OnAfterRenderAsync calls get queued inside the sync context within ProcessRenderQueue when the UpdateDisplayAsync task doesn't complete synchronously.

Also, calling ProcessRenderQueue does a bunch of work that is almost always unnecessary in this case. Can I suggest you put a guard clause at the top of ProcessRenderQueue so it just returns if _batchBuilder.ComponentRenderQueue.Count == 0? Then we can also remove the existing check for that condition from the bottom of ProcessRenderQueue itself.

_batchBuilder is a private member of the base class and i think it's worth exposing it. We can afford having an extra comparison on the base class, even if its not super common, I don't think it affects perf. An alternative would be to add a HasPendingRenders method/property on Renderer or a new method on Renderer that wraps the call to ProcessRenderQueue with that check.

But I don't think either of those are a better solution for simply avoiding a check.

Acknowledging the batch can happen outside of the sync context because of the comment mentioned above regarding signalr dispatching only 1 message a time and because the OnAfterRenderAsync calls get queued inside the sync context within ProcessRenderQueue when the UpdateDisplayAsync task doesn't complete synchronously.

Right, I see. This has confused us multiple times now, so maybe we should reconsider relying on that. If your change here means we definitely have to do an InvokeAsync for the ProcessRenderQueue call, could we instead force the entire execution of OnRenderCompleted to run on the sync context? That would more closely match our intuitions, and only results in the same amount of dispatching. It's not like we're really losing parallelism by doing this, since as you note, SignalR serializes the calls anyway.

_batchBuilder is a private member of the base class and i think it's worth exposing it

Sorry if I was unclear. I meant putting the guard clause at the top of ProcessRenderQueue in the base class, which already has access to _batchBuilder.

Sorry if I was unclear. I meant putting the guard clause at the top of ProcessRenderQueue in the base class, which already has access to _batchBuilder.

I think I addressed this?

rynowak

This is looking a lot simpler than the earlier iteration 👍

src/Components/Components/src/Rendering/Renderer.cs

SteveSandersonMS · 2019-08-01T15:08:59Z

src/Components/Server/src/Circuits/RemoteRenderer.cs

@@ -8,6 +8,7 @@
 using System.Threading;


It's really great that you got this new behavior into RemoteRenderer and not in the Renderer base class 👍

src/Components/Server/src/Circuits/RemoteRenderer.cs

src/Components/Components/src/Rendering/Renderer.cs

src/Components/Server/src/ComponentHub.cs

src/Components/test/E2ETest/ServerExecutionTests/RemoteRendererBufferLimitTest.cs

javiercn · 2019-08-05T14:24:15Z

🆙 📅

@SteveSandersonMS Maybe we need to have a chat to hash out the public API for this, I think I gave you good arguments for keeping it as it is (with the addition that I added) but if you don't feel that way, lets talk about it.

javiercn · 2019-08-05T15:57:42Z

🆙 📅

BrennanConroy · 2019-08-05T15:58:20Z

Commits don't look right

@SteveSandersonMS

Suggestions from @SteveSandersonMS Co-Authored-By: Steve Sanderson <[email protected]>

javiercn · 2019-08-05T16:23:06Z

Commits don't look right

I fixed them.

SteveSandersonMS · 2019-08-06T13:39:11Z

src/Components/Components/src/Rendering/Renderer.cs

        private void ProcessRenderQueue()
        {
+            EnsureSynchronizationContext();


This can go into ProcessPendingRender, since that's the external entrypoint, right?

I thought it couldn't because it was on the virtual method, but not that I think again it can, as people is forced to invoke the base to do any meaningful work.

I'll change this in a separate PR as I don't want to re-run the build

SteveSandersonMS

Great! This is a complex area but this PR gives me confidence we're handling it well.

javiercn requested a review from SteveSandersonMS as a code owner July 31, 2019 15:41

javiercn commented Jul 31, 2019

View reviewed changes

javiercn requested a review from rynowak July 31, 2019 16:02

Pilchie added the area-blazor Includes: Blazor, Razor Components label Jul 31, 2019

rynowak reviewed Aug 1, 2019

View reviewed changes

src/Components/Server/src/Circuits/RemoteRenderer.cs Outdated Show resolved Hide resolved

rynowak reviewed Aug 1, 2019

View reviewed changes

src/Components/Server/src/Circuits/RemoteRenderer.cs Outdated Show resolved Hide resolved

rynowak reviewed Aug 1, 2019

View reviewed changes