Skip to content
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

[wasm] Don't use SyncTextWriter in single-threaded wasm builds #101221

Merged
merged 4 commits into from
Jul 5, 2024

Conversation

kg
Copy link
Member

@kg kg commented Apr 18, 2024

(New description, PR was revised)

We normally wrap custom console streams with SyncTextWriter, which forces the mono interpreter/jit/aot compiler to use synchronized wrappers for all calls through the class. Wrappers have optimization force-enabled, which means all the methods they call get inlined into the wrapper every time, increasing the size of the wrapper's code and the amount of time we spend in codegen for it.

The default wasm configuration right now is single-threaded, so we can skip using SyncTextWriter and as a result skip the synchronized wrapper. While this doesn't fully prevent monitors from becoming involved in startup, it removes a bunch of inlined monitor API calls, which should be an improvement to startup time for any wasm application that touches the console.

@kg kg added the arch-wasm WebAssembly architecture label Apr 18, 2024
@kg kg requested a review from marek-safar as a code owner April 18, 2024 00:23
Copy link
Contributor

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

@lambdageek
Copy link
Member

The fast path should always be taken in ST builds and everything should inline. Why is this necessary?

Also, monitor.isentered will always return false this way

@kg
Copy link
Member Author

kg commented Apr 18, 2024

The fast path should always be taken in ST builds and everything should inline. Why is this necessary?

Also, monitor.isentered will always return false this way

A sizable percentage of startup time is interp codegen, so I've been seeing how many subtrees of useless methods I can prune out easily.

The monitor.isentered thing is a good point, I guess that rules this optimization out.

@lambdageek
Copy link
Member

A sizable percentage of startup time is interp codegen, so I've been seeing how many subtrees of useless methods I can prune out easily.

ah ok. so actually rewriting this part in managed was a bad idea for startup perf. we only measured benchmark (steady state) perf once the interpreter was tiering up the callers. (so Monitor.Enter/Exit loops without contention on ST interp were the same as or a bit faster than the old JIT icall monitor implementation)

@lambdageek
Copy link
Member

Do you think it makes sense to use a simpler lockword representation for ST? (for example we don't have to use Interlocked ops to increment/decrement the count - we can just use normal reads/writes). possibly hand-inline more code from ObjectHeader into Monitor.Enter/Exit?

@kg
Copy link
Member Author

kg commented Apr 18, 2024

Do you think it makes sense to use a simpler lockword representation for ST? (for example we don't have to use Interlocked ops to increment/decrement the count - we can just use normal reads/writes). possibly hand-inline more code from ObjectHeader into Monitor.Enter/Exit?

I think that makes sense, we could just provide simple implementations of the primitives for that configuration which would prune out most of the dependencies without breaking it. I'll make a note to try that.

@kg
Copy link
Member Author

kg commented Apr 24, 2024

I'm thinking that normal Blazor apps rarely use C# Console. Only our samples do so. Do I understand it right that it would not get triggered unless you actually use it ?

It only gets triggered if you write to the console, yes, but don't we write to the console at least once during startup? I thought I recalled seeing a console message

@kg
Copy link
Member Author

kg commented Apr 24, 2024

They do use other streams tho, like HTTP stream. Is there similar problem?

This wrapper overhead applies to any synchronized method that is in the startup path, yes

@kg kg merged commit 5b7e77d into dotnet:main Jul 5, 2024
143 of 146 checks passed
Comment on lines +161 to +162
// Browser bypasses SyncTextWriter for faster startup
if (!OperatingSystem.IsBrowser())
Copy link
Member

@akoeplinger akoeplinger Jul 7, 2024

Choose a reason for hiding this comment

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

this should use PlatformDetection.IsThreadingSupported too instead of checking for browser so it applies to single-threaded wasi too

@@ -22,6 +22,8 @@
<IsBigEndian Condition="'$(Platform)' == 's390x'">true</IsBigEndian>
<Is64Bit Condition="'$(Platform)' == 'arm64' or '$(Platform)' == 'x64' or '$(Platform)' == 's390x' or '$(Platform)' == 'loongarch64' or '$(Platform)' == 'ppc64le' or '$(Platform)' == 'riscv64'">true</Is64Bit>
<UseMinimalGlobalizationData Condition="'$(TargetsBrowser)' == 'true' or '$(TargetsWasi)' == 'true'">true</UseMinimalGlobalizationData>
<FeatureWasmManagedThreads Condition="'$(WasmEnableThreads)' == 'true'">true</FeatureWasmManagedThreads>
<DefineConstants Condition="'$(FeatureWasmManagedThreads)' == 'true'">$(DefineConstants);FEATURE_WASM_MANAGED_THREADS</DefineConstants>
Copy link
Member

Choose a reason for hiding this comment

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

this duplicates the conditions from src/mono/System.Private.CoreLib/System.Private.CoreLib.csproj, I don't think we need them here

@@ -759,7 +759,11 @@ public static TextWriter Synchronized(TextWriter writer)
{
ArgumentNullException.ThrowIfNull(writer);

#if !TARGET_BROWSER || FEATURE_WASM_MANAGED_THREADS
Copy link
Member

Choose a reason for hiding this comment

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

should also check for WASI

akoeplinger added a commit to akoeplinger/runtime that referenced this pull request Jul 12, 2024
Follow-up to dotnet#101221 so we match behavior between single-threaded Browser and WASI.
akoeplinger added a commit that referenced this pull request Jul 15, 2024
Follow-up to #101221 so we match behavior between single-threaded Browser and WASI.
@github-actions github-actions bot locked and limited conversation to collaborators Aug 7, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-wasm WebAssembly architecture
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants