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

Optimize (WasmRoot|WasmExternalRoot).clear #77521

Merged
merged 3 commits into from
Nov 8, 2022

Conversation

kg
Copy link
Member

@kg kg commented Oct 26, 2022

This PR optimizes the wasm root clear and set methods to not use a write barrier when zeroing a root, because the write barrier is expensive and not really necessary. The downside is this could cause an object to live slightly too long in some cases, but we call clear a LOT for correctness so the cost has a significant impact on interop speed, at least according to browser-bench.

Before:

measurement time
JSInterop, LegacyExportInt 6.9038ms
JSInterop, LegacyExportString 33.4786ms
JSInterop, JSExportInt 3.5533ms
JSInterop, JSExportString 31.2959ms

After:

measurement time
JSInterop, LegacyExportInt 6.1863ms
JSInterop, LegacyExportString 30.1000ms
JSInterop, JSExportInt 3.6297ms
JSInterop, JSExportString 28.9896ms

This optimization improves the legacy export performance since we use a root to store the return value. String stuff uses roots too, as do some of the other JS wrappers and such.

Note that these tests have significant timing variance from run to run because they spend a lot of time in the JS and C# garbage collectors (mostly due to the string allocations)

@ghost
Copy link

ghost commented Oct 26, 2022

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

Issue Details

ManagedError's constructor unintentionally retrieves the value of Error.stack, which is very expensive. This PR fixes that by using a workaround of constructing a temporary Error instead (still expensive, but much less expensive) so that we can only fetch the stack on demand when necessary. A potential future optimization would be to only do this in Chrome, since super.stack apparently works in Firefox (maybe Safari, too?) but the complication is probably not worth it at this point.

This PR also optimizes the root clear and set methods to not use a write barrier when zeroing a root, because the write barrier is expensive and not really necessary. The downside is this could cause an object to live slightly too long in some cases, but we call clear a LOT for correctness so the cost has a significant impact on interop speed, at least according to browser-bench.

Before:

measurement time
JSInterop, LegacyExportInt 6.9038ms
JSInterop, JSExportInt 3.5533ms
JSInterop, LegacyExportString 33.4786ms
JSInterop, JSExportString 31.2959ms
JSInterop, JSImportInt 3.3081ms
JSInterop, JSImportString 40.0709ms
JSInterop, JSImportManyArgs 227.4000ms
JSInterop, JSImportTask 299.7647ms
JSInterop, JSImportTaskFail 1089.6000ms
JSInterop, JSImportFail 165.3438ms

After:

measurement time
JSInterop, LegacyExportInt 6.1863ms
JSInterop, JSExportInt 3.6297ms
JSInterop, LegacyExportString 30.1000ms
JSInterop, JSExportString 28.9896ms
JSInterop, JSImportInt 3.6116ms
JSInterop, JSImportString 38.5294ms
JSInterop, JSImportManyArgs 231.2609ms
JSInterop, JSImportTask 298.1176ms
JSInterop, JSImportTaskFail 809.1429ms
JSInterop, JSImportFail 159.5152ms

JSImportTaskFail improved considerably due to the ManagedError stack optimization. The legacy exports meaningfully improved due to clear being optimized (part of the reason this matters is that right now to invoke a managed method, we HAVE to box the return value which means using a root that has to be cleared at the end.)

Note that these tests have significant timing variance from run to run because they spend a lot of time in the JS and C# garbage collectors (mostly due to the string allocations)

This fixes #77473

Author: kg
Assignees: kg
Labels:

arch-wasm, area-System.Runtime.InteropServices.JavaScript

Milestone: -

@kg
Copy link
Member Author

kg commented Oct 27, 2022

Looks like using the class syntax to override stack isn't working, at least on CI. I'll have to try something else.

@kg kg added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Oct 27, 2022
@pavelsavara

This comment was marked as outdated.

@kg
Copy link
Member Author

kg commented Oct 27, 2022

Adding a test is a good idea, yours looks like it should also benchmark fine. The defineProperty is expensive, though.

@kg
Copy link
Member Author

kg commented Oct 28, 2022

This approach to optimizing ManagedError can't work because of a chrome bug: https://bugs.chromium.org/p/chromium/issues/detail?id=1379185
Pavel's PR is probably the best we can do for that bit.

const destinationAddress = this.__buffer.get_address(this.__index);
cwraps.mono_wasm_write_managed_pointer_unsafe(destinationAddress, <ManagedPointer>value);
if (value === <any>0) {
// Don't use .set since it involves a gc write barrier, and the write barrier
Copy link
Member

Choose a reason for hiding this comment

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

Are we concerned only about the case when the root is just newly allocated?
Or this is also when somebody free the root ?
Do you mean that it doesn't matter that GC on another thread would hold the root a bit longer ?
What is the semantics of clear method ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Clear zeroes out a root after you're done with it. This happens both when freeing the root (to avoid leaking object references) and when done with a reusable scratch root. The most common example of something where this would happen a lot is the root we use to store return values from function calls.

@pavelsavara
Copy link
Member

I suggest that you revert the ManagedError change from this PR and we further explore the WasmRoot here.
Please also update the description and subject of the PR.

@kg kg force-pushed the wasm-interop-optimizations-1026 branch from 2330716 to 9b40657 Compare November 5, 2022 00:49
@kg kg changed the title Optimize ManagedError ctor and (WasmRoot|WasmExternalRoot).clear Optimize (WasmRoot|WasmExternalRoot).clear Nov 5, 2022
@kg
Copy link
Member Author

kg commented Nov 5, 2022

Rebased and updated to remove the error stuff @pavelsavara

@kg kg force-pushed the wasm-interop-optimizations-1026 branch from 4fc70b8 to 8a986c1 Compare November 7, 2022 17:54
@kg kg merged commit 86611ab into dotnet:main Nov 8, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Dec 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-wasm WebAssembly architecture area-System.Runtime.InteropServices.JavaScript NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants