Cache JS->.NET string decoding results within a single renderbatch #23773

SteveSandersonMS · 2020-07-08T13:42:44Z

Profiling shows that Blazor WebAssembly spends a substantial chunk of time decoding .NET strings into JavaScript ones during rendering. This optimization improves the grid benchmark results (on my laptop) from this:

... to this:

As you can see, the biggest percentage gain is on the FastGrid benchmarks, because they already don't spend much time doing anything else. At roughly 9%, the gains in this situation easily warrant doing such a straightforward optimization.

Implementation note

Blazor WebAssembly has always relied on JS code being able to read data directly from .NET's heap. This allows us to skip serializing renderbatches entirely. Without doing this, perf would be destroyed.

In order for this to be safe, we've relied on the fact that .NET's GC can only run when .NET itself runs, which is not during the DOM-update time when we exclusively run JavaScript. So we know that any pointers we hold will certainly remain valid. For this new string caching behavior, we also rely on the same thing (that is, the mapping from "pointer value" to "decoded string" must remain valid because nothing is changing inside the .NET heap).

However until now that's been done quite informally. This PR introduces a slightly higher degree of formality by defining the notion of a ".NET heap lock" that JS code can take and release around a critical section. We can then:

Prevent any JS interop calls invoking .NET during the critical section (which should never have happened before anyway, but now at least we have extra verification of that)
Make use of other optimizations, such as holding a cache of decoded strings which remains valid only until the lock is released

In the long term when dotnet.wasm gains true multithreading, we will have to contend with more complicated situations where .NET code can in fact run while the JS code is simultaneously executing. When we get to that point, we'll want to use this ".NET heap lock" concept to signal to .NET that it's not allowed to mutate the memory that we care about. Most likely it will be sufficient to prevent GC compaction during the critical section, but we will have to reason about that in detail at the time.

SteveSandersonMS · 2020-07-08T13:43:35Z

src/Components/Web.JS/src/Platform/Mono/MonoPlatform.ts

@@ -246,7 +264,6 @@ function createEmscriptenModuleInstance(resourceLoader: WebAssemblyResourceLoade
  module.preRun.push(() => {
    // By now, emscripten should be initialised enough that we can capture these methods for later use
    mono_wasm_add_assembly = cwrap('mono_wasm_add_assembly', null, ['string', 'number', 'number']);
-    mono_string_get_utf8 = cwrap('mono_wasm_string_get_utf8', 'number', ['number']);


Was not used

pranavkm · 2020-07-08T15:50:48Z

src/Components/Web.JS/src/Platform/Mono/MonoPlatform.ts

+        currentHeapLock.stringCache.set(fieldValue, decodedString);
+      }
+    } else {
+      decodedString = BINDING.conv_string(fieldValue as any as System_String);


Is there code that hits this code path?

Yes, any of the framework's built-in unmarshalled interop calls that read strings would hit this.

I know you could argue that they too should take a heap lock, but I'm not trying to change everything here. We only need to reason about all unmarshalled interop if/when we get multithreading.

pranavkm · 2020-07-08T15:53:20Z

In the long term when dotnet.wasm gains true multithreading, we will have to contend with more complicated situations where .NET code can in fact run while the JS code is simultaneously executing. When we get to that point, we'll want to use this ".NET heap lock" concept to signal to .NET that it's not allowed to mutate the memory that we care about. Most likely it will be sufficient to prevent GC compaction during the critical section, but we will have to reason about that in detail at the time.

Have we discussed this with the runtime folks just so that they are aware of our requirements when designing multi-threading?

pranavkm · 2020-07-08T15:54:38Z

Uhoh test failure:

OpenQA.Selenium.BrowserAssertFailedException : Xunit.Sdk.EqualException: Assert.Equal() Failure
Expected: Hello there!
Actual: 
 at Xunit.Assert.Equal[T](T expected, T actual, IEqualityComparer1 comparer) in C:\\Dev\\xunit\\xunit\\src\\xunit.assert\\Asserts\\EqualityAsserts.cs:line 40
 at Microsoft.AspNetCore.E2ETesting.WaitAssert.<>c__DisplayClass17_0.<WaitAssertCore>b__0() in /_/src/Shared/E2ETesting/WaitAssert.cs:line 80
 at Microsoft.AspNetCore.E2ETesting.WaitAssert.<>c__DisplayClass18_01.<WaitAssertCore>b__0(IWebDriver ) in //src/Shared/E2ETesting/WaitAssert.cs:line 106
Screen shot captured at 'F:\workspace\_work\1\s\artifacts\TestResults\Release\Microsoft.AspNetCore.Components.E2ETests\cfedba372afb412d9997f08b78dbeb86.png'
Encountered browser errors
[2020-07-08T14:18:35Z] [Debug] http://127.0.0.1:53406/subdir/_framework/dotnet.5.0.0-preview.7.20326.1.js 0:124704 "mono_wasm_runtime_ready" "fe00e07a-5519-4dfe-b35a-f867dbaf2e28"
[2020-07-08T14:19:55Z] [Severe] http://127.0.0.1:53406/subdir/_framework/blazor.webassembly.js 0:48043 Uncaught Error: Assertion failed - heap is currently locked
[2020-07-08T14:20:06Z] [Severe] http://127.0.0.1:53406/subdir/_framework/blazor.webassembly.js 0:48043 Uncaught Error: Assertion failed - heap is currently locked
[2020-07-08T14:20:10Z] [Severe] http://127.0.0.1:53406/subdir/_framework/blazor.webassembly.js 0:48043 Uncaught Error: Assertion failed - heap is currently locked
[2020-07-08T14:20:18Z] [Severe] http://127.0.0.1:53406/subdir/_framework/blazor.webassembly.js 0:48043 Uncaught Error: Assertion failed - heap is currently locked
[2020-07-08T14:20:39Z] [Severe] http://127.0.0.1:53406/subdir/_framework/blazor.webassembly.js 0:48043 Uncaught Error: Assertion failed - heap is currently locked
[2020-07-08T14:21:01Z] [Severe] http://127.0.0.1:53406/subdir/_framework/blazor.webassembly.js 0:48043 Uncaught Error: Assertion failed - heap is currently locked
[2020-07-08T14:21:08Z] [Severe] http://127.0.0.1:53406/subdir/_framework/blazor.webassembly.js 0:48043 Uncaught Error: Assertion failed - heap is currently locked
[2020-07-08T14:21:08Z] [Severe] http://127.0.0.1:53406/subdir/_framework/blazor.webassembly.js 0:48043 Uncaught Error: Assertion failed - heap is currently locked
[2020-07-08T14:21:08Z] [Severe] http://127.0.0.1:53406/subdir/_framework/blazor.webassembly.js 0:48043 Uncaught Error: Assertion failed - heap is currently lockedPage content:
<head>

javiercn · 2020-07-08T15:57:51Z

Have we discussed this with the runtime folks just so that they are aware of our requirements when designing multi-threading?

I believe when there's multithreading going on, some version of https://docs.microsoft.com/en-us/dotnet/api/system.gc.trystartnogcregion?view=netcore-3.1 will be what we might end up doing here.

All we care about are the pointers not moving, isn't it? The memory on the render batches should not change since all values are inmutable and we control the objects that are created?

SteveSandersonMS · 2020-07-08T16:01:55Z

Have we discussed this with the runtime folks just so that they are aware of our requirements when designing multi-threading?

Yes

All we care about are the pointers not moving, isn't it? The memory on the render batches should not change since all values are inmutable and we control the objects that are created?

Correct

Uhoh test failure:

Excellent! Now we can see if we were making a bad assumption about something before...

SteveSandersonMS · 2020-07-08T18:21:16Z

I'm very glad we got that test failure, because this has identified a possible rendering bug. Admittedly it would be incredibly hard to repro, so much that it's possible nobody ever has done, but if you:

Set up a component that mutates the DOM in such a way that the mutation itself triggers an event (this is difficult to do, but our "reordering focus retention" E2E test case does it)
... and then during the inner rendering loop, the component renders 2 or more times (so as to overwrite the original render tree in memory)

... then you could end up with an exception or undefined behavior.

I've implemented what I think is roughly the correct fix, but I want to add some proper E2E test cases dedicated to this edge case before merging this.

SteveSandersonMS · 2020-07-08T18:23:57Z

src/Components/Web.JS/src/Rendering/RendererEventDispatcher.ts

 }

-export function setEventDispatcher(newDispatcher: (eventDescriptor: EventDescriptor, eventArgs: UIEventArgs) => Promise<void>): void {
+export function setEventDispatcher(newDispatcher: (eventDescriptor: EventDescriptor, eventArgs: UIEventArgs) => void): void {


I've changed newDispatcher to return void because we only call it in one place, and that place discards the returned value anyway. Event-triggering is fire-and-forget from the JS side, because any errors are handled on the .NET side.

So having it return void is a more accurate representation of what's going on, and it simplifies a few other bits of the code.

SteveSandersonMS added area-blazor Includes: Blazor, Razor Components feature-blazor-wasm This issue is related to and / or impacts Blazor WebAssembly labels Jul 8, 2020

SteveSandersonMS requested a review from a team July 8, 2020 13:42

SteveSandersonMS commented Jul 8, 2020

View reviewed changes

javiercn approved these changes Jul 8, 2020

View reviewed changes

SteveSandersonMS self-assigned this Jul 8, 2020

SteveSandersonMS added the Working label Jul 8, 2020

SteveSandersonMS mentioned this pull request Jul 8, 2020

Cache JS->.NET string decoding results within a single renderbatch #23775

Closed

SteveSandersonMS linked an issue Jul 8, 2020 that may be closed by this pull request

Cache JS->.NET string decoding results within a single renderbatch #23775

Closed

pranavkm approved these changes Jul 8, 2020

View reviewed changes

SteveSandersonMS commented Jul 8, 2020

View reviewed changes

captainsafia approved these changes Jul 9, 2020

View reviewed changes

SteveSandersonMS added 4 commits July 9, 2020 13:07

Cache JS->.NET string decoding results within a single renderbatch

acb01f8

Fix for the recursive batch rendering bug. Needs more tests though.

a7ebaef

Add E2E test cases for events during batch rendering

f97325b

Update JS files

cf32d51

SteveSandersonMS force-pushed the stevesa/cache-string-decoding branch from 18eb523 to cf32d51 Compare July 9, 2020 12:08

SteveSandersonMS merged commit 6cb5752 into master Jul 9, 2020

SteveSandersonMS deleted the stevesa/cache-string-decoding branch July 9, 2020 13:40

Stamo-Gochev mentioned this pull request Oct 12, 2020

"Assertion failed - heap is currently locked" when an element is focused out #26809

Closed

github-actions bot locked and limited conversation to collaborators Dec 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Cache JS->.NET string decoding results within a single renderbatch #23773

Cache JS->.NET string decoding results within a single renderbatch #23773

Uh oh!

SteveSandersonMS commented Jul 8, 2020

Uh oh!

SteveSandersonMS Jul 8, 2020

Uh oh!

pranavkm Jul 8, 2020

Uh oh!

SteveSandersonMS Jul 8, 2020

Uh oh!

SteveSandersonMS Jul 8, 2020 •

edited

Loading

Uh oh!

pranavkm commented Jul 8, 2020

Uh oh!

pranavkm commented Jul 8, 2020 •

edited

Loading

Uh oh!

javiercn commented Jul 8, 2020

Uh oh!

SteveSandersonMS commented Jul 8, 2020

Uh oh!

SteveSandersonMS commented Jul 8, 2020

Uh oh!

SteveSandersonMS Jul 8, 2020

Uh oh!

Uh oh!

Cache JS->.NET string decoding results within a single renderbatch #23773

Cache JS->.NET string decoding results within a single renderbatch #23773

Uh oh!

Conversation

SteveSandersonMS commented Jul 8, 2020

Implementation note

Uh oh!

SteveSandersonMS Jul 8, 2020

Choose a reason for hiding this comment

Uh oh!

pranavkm Jul 8, 2020

Choose a reason for hiding this comment

Uh oh!

SteveSandersonMS Jul 8, 2020

Choose a reason for hiding this comment

Uh oh!

SteveSandersonMS Jul 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pranavkm commented Jul 8, 2020

Uh oh!

pranavkm commented Jul 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

javiercn commented Jul 8, 2020

Uh oh!

SteveSandersonMS commented Jul 8, 2020

Uh oh!

SteveSandersonMS commented Jul 8, 2020

Uh oh!

SteveSandersonMS Jul 8, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

SteveSandersonMS Jul 8, 2020 •

edited

Loading

pranavkm commented Jul 8, 2020 •

edited

Loading