[browser][mt] Update memory views after growth, refactor string processing, fix SharedArrayBuffer detection

[IsaMorphic]

"memory pooling or reclamation mechanism that they use internally that causes SharedArrayBuffer to cross the iframe boundary"

• test for SharedArrayBuffer via buffer[Symbol.toStringTag] === "SharedArrayBuffer"
  • @IsaMorphic adapted a fix to this issue from this PR in the emscripten repository: Fix TextDecoder exception on decoding SharedArrayBuffer emscripten-core/emscripten#16994
• added copyBufferIfNecessary helper
  • the method is NOOP in non-threaded build
• added receiveWorkerHeapViews
  • the method is NOOP in non-threaded build
• replaced Module.HEAPxx with new localHeapViewXXX()
  • which calls receiveWorkerHeapViews and produces updated short lived "local" view
• added -s TEXTDECODER=0 emcc linker flag for MT build
• wrap Module.UTF8ArrayToString and Module.UTF8ToString with our own utf8ToString and decodeUTF8
  • this will copy the bytes and use TextDecoder for longer strings instead of emscripten byte-by-byte loop
• move code into encodeUTF8 and utf8ToStringRelaxed of strings.ts
• prefer HEAPU8 over HEAP8 for code consistency
• string.ts refactoring

Fixes#86696
Fixes#86642

Related issues:
emscripten-core/emscripten#15217

[IsaMorphic]

@dotnet-policy-service agree

[kg]

Thanks for your contribution! If I understand correctly, this instanceof bug affects scenarios involving iframes because the iframe is a different realm and has its own builtins like Array and SharedArrayBuffer, right? So any case where we are using instanceof is potentially broken in that case, which is what motivates APIs like Array.isArray.

[IsaMorphic]

Hi @kg!

That's exactly the scenario I came across that motivated this fix. I'll get to work right away on a solution that avoids the call to toString 😁

I'm glad to help!

[pavelsavara]

How could dotnet wasm linear memory come from iframe ?
Are we talking about 2 runtimes sharing memory across the iframe boundary ?

I would appreciate small demo project which reproduces the issue before we merge this.
Adapting one of the samples would be ideal.

Possibly related issue #86642

[ghost]

Tagging subscribers to 'arch-wasm': @lewing
See info in area-owners.md if you want to be subscribed.

Issue Details

---

When performing tasks such as allocating or marshalling large amounts of memory in multithreaded .NET land, inconsistent and frequent crashing behavior is exhibited.

I adapted a fix to this issue from this PR in the emscripten repository:
emscripten-core/emscripten#16994

Related issues:
emscripten-core/emscripten#15217
#86696

Author:	IsaMorphic
Assignees:	-
Labels:	arch-wasm, area-System.Runtime.InteropServices.JavaScript, community-contribution, needs-area-label
Milestone:	-

[IsaMorphic]

@pavelsavara
To answer your question, yes: in my implementation specific use case, I am using a Blazor app as an embeddable image renderer to demonstrate a future product. In my application I am using 2 or more runtimes per page (each in their own iframe). However, it seems (from the related emscripten bug) that all modern browsers might have some type of memory pooling or reclamation mechanism that they use internally that causes buffers to cross the iframe boundary. Though it still could be misuse on the part of the .net runtime, as well :P

Please see the latest commit on my fork's broken-sample branch. Use the typical launch commands for the browser-threading-minimal sample, but make sure to browse to http://localhost:<port>/embeds.html and read the text on the page. This updated sample replicates the issue extremely consistently.

I have not tried to see if my patch helps, as I have not built the runtime with the -sTEXTDECODER=0 flag and tested it in this way yet.

Hope this is helpful.

Update:
I've just tried out my sample with the current patchset of my PR + full runtime build with export EMCC_CFLAGS="-sTEXTDECODER=0" There's still errors of the same vein as we've been discussing at startup, only now it is inside dotnet.native.js and it is consistently generated from the same callsite. Otherwise there are no more errors of this nature at runtime.

I don't know where in the code this corresponds to. If someone can point me to it so I can update the file with the same Symbol.toStringTag fix that would be very helpful. Thanks!

[pavelsavara]

UTF8ArrayToString is emscripten method and the issue is the same emscripten-core/emscripten#18034
TEXTDECODER=0 should have fixed that for you.

"memory pooling or reclamation mechanism that they use internally that causes buffers to cross the iframe boundary" could you please point me to more details about it ?

[pavelsavara]

Also, if there is buffer reuse across iframes, it should also manifest without threads, right ?
We have iframe based benchmark which we run daily. I don't remember such issue to manifest there.

@radekdoulik would we capture/notice the error if one of the AppStart measures failed to start the runtime in the iframe ?

[pavelsavara]

I get it now, in our benchmark it's not a SharedArrayBuffer. I guess we should create MT flavor of the benchmark soon anyway.

[pavelsavara]

Please see the latest commit on my fork's broken-sample branch.

main...IsaMorphic:runtime-wasmfix:broken-sample

I will have look soon, probably next week. Thanks a lot!

I would still love to read about how this is not a browser bug. :-D

[pavelsavara]

I used your sample and indeed I can reproduce it.

I will push more comprehensive fix into this PR

[pavelsavara]

I processed further feedback from @kg about not calling updateGrowableHeapViews() from tight loops.

While doing that I noticed that we have too many loops where we convert strings with different encodings, so I moved all of it into string.ts.
And while I worked on that I dropped StringDecoder class because it was not helping anything.
I dropped few legacy methods and renamed the rest to camelCase.

@ilonatommy I touched some of the hybrid locale stuff, please have look.

There is bit of risk that this change will introduce perf regression, but it was that already yesterday.
@radekdoulik could you please run benchmark on this before I merge it ?

[pavelsavara]

/azp run runtime-wasm

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[pavelsavara]

/azp run runtime-wasm

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[ilonatommy]

Globalization pointer math checked.

[pavelsavara]

/azp run runtime-wasm

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[pavelsavara]

/azp run runtime-wasm

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[pavelsavara]

I guess this is that we eliminated bunch of checks in the non-MT string conversion.

https://radekdoulik.github.io/WasmPerformanceMeasurements/?startDate=2023-05-24T10%3A51%3A33.000Z&endDate=2023-05-29T21%3A06%3A42.000Z&tasks=%2CJSInterop