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

[net6] xamarin_get_frame_length performance regression #14812

Closed
jeromelaban opened this issue Apr 22, 2022 · 8 comments
Closed

[net6] xamarin_get_frame_length performance regression #14812

jeromelaban opened this issue Apr 22, 2022 · 8 comments
Labels
dotnet An issue or pull request related to .NET (6) enhancement The issue or pull request is an enhancement
Milestone

Comments

@jeromelaban
Copy link

jeromelaban commented Apr 22, 2022

Description

When invoking a UIView property (and likely anything else), there's a new native method being invoked xamarin_get_frame_length which incurs a very high cost when invoking methods.

This causes a very large performance regression (25:1 in some of our test cases) when comparing with legacy Xamarin.

The instruments traces below are from a physical device running arm64.

Steps to Reproduce

Create an application which invokes UIView.get_SuperView, invoke it in a loop.

Expected Behavior

Here's the Legacy Xamarin result:
image

Actual Behavior

Here's the .NET 6.0 RC1 result:
image

Environment

.NET 6 RC1 (https://aka.ms/dotnet/maui/rc.1.json)

Build Logs

Available on demand.

Example Project (If Possible)

None, see repro steps above.

@jeromelaban jeromelaban changed the title [][xamarin] [net6] xamarin_get_frame_length performance regression Apr 22, 2022
@jeromelaban
Copy link
Author

The issue's description is now up to date.

@jeromelaban jeromelaban changed the title [net6] xamarin_get_frame_length performance regression [net6] xamarin_get_frame_length performance regression Apr 22, 2022
@spouliot
Copy link
Contributor

It might be the project options (or a different default value in dotnet vs legacy) but xamarin_dyn_objc_msgSend* APIs (that calls xamarin_get_frame_length) are used when ObjC exceptions are marshalled. If that's disabled then that (known to be slow) code should not be reached.

@rolfbjarne
Copy link
Member

It might be the project options (or a different default value in dotnet vs legacy) but xamarin_dyn_objc_msgSend* APIs (that calls xamarin_get_frame_length) are used when ObjC exceptions are marshalled. If that's disabled then that (known to be slow) code should not be reached.

That's correct, we've changed the default so that exception marshalling is now on by default.

Adding this to the csproj should get the old behavior back:

<MtouchExtraArgs>--marshal-objectivec-exceptions:disable</MtouchExtraArgs>

@jeromelaban
Copy link
Author

Thanks @rolfbjarne, @spouliot! Would you have some documentation about this change (or the feature itself) aside from #4940 and why you chose to use this mode? It's drastically changing the performance characteristics, but I wonder about the use cases, particularly if it can be enabled per method. Thanks!

@rolfbjarne
Copy link
Member

@jeromelaban exception marshaling is documented here: https://docs.microsoft.com/en-us/xamarin/ios/platform/exception-marshaling

Making it enabled by default is something we plan on putting in the release notes (it's listed here: #10574)

That said, it should be easy enough to fix that enhancement, since we'd only have to add a way to override the default logic.

@rolfbjarne rolfbjarne added enhancement The issue or pull request is an enhancement dotnet An issue or pull request related to .NET (6) dotnet-pri3 .NET 6: wishlist for stable release labels Apr 25, 2022
@rolfbjarne rolfbjarne added this to the .NET 6 milestone Apr 25, 2022
@rolfbjarne rolfbjarne added this to Pri 3 - Not Started in .NET 6 Apr 25, 2022
@jeromelaban
Copy link
Author

Thanks for the details! We'll likely use the original behavior for now even if the exception marshalling is incorrect. It's been working this way for many years and we've coped with it all this time, so we can continue doing so for a while :)

rolfbjarne added a commit to rolfbjarne/xamarin-macios that referenced this issue May 9, 2022
…s or not. Fixes xamarin#4940.

* This is a potential mitigation for slower transition to native code when
  exception marshalling is enabled (xamarin#14812).
* A minor modification was required in the linker, to make sure any modified
  assemblies are saved.

Fixes xamarin#4940.
rolfbjarne added a commit that referenced this issue May 11, 2022
…s or not. Fixes #4940. (#14961)

* This is a potential mitigation for slower transition to native code when
  exception marshalling is enabled (#14812).
* A minor modification was required in the linker, to make sure any modified
  assemblies are saved.

Fixes #4940.
@rolfbjarne
Copy link
Member

I've fixed #14961, so it will be possible at some point to improve speed at the cost of app size (not for the first stable release, but for the next Xcode release we'll support, that be either Xcode 13.4 or Xcode 14.0).

We might still want to look into if there are other performance improvements that can be done, so I'm keeping this open for now, but postponing until .NET 7.

@rolfbjarne rolfbjarne modified the milestones: .NET 6, .NET 7 May 11, 2022
@rolfbjarne rolfbjarne removed this from Pri 3 - Not Started in .NET 6 May 11, 2022
@rolfbjarne rolfbjarne removed the dotnet-pri3 .NET 6: wishlist for stable release label May 11, 2022
rolfbjarne added a commit to rolfbjarne/xamarin-macios that referenced this issue May 11, 2022
…s or not. Fixes xamarin#4940. (xamarin#14961)

* This is a potential mitigation for slower transition to native code when
  exception marshalling is enabled (xamarin#14812).
* A minor modification was required in the linker, to make sure any modified
  assemblies are saved.

Fixes xamarin#4940.
@rolfbjarne rolfbjarne modified the milestones: .NET 7, .NET 8 Aug 26, 2022
@rolfbjarne rolfbjarne added this to Optimizations in .NET 8 - Themes Sep 12, 2022
@rolfbjarne
Copy link
Member

I just did a very simple perf test on Mac Catalyst / ARM64:

var watch = Stopwatch.StartNew ();
for (var i = 0; i < counter; i++) {
	var x = view.Superview;
}
watch.Stop ();

The results are for 20.000.000 iterations:

Mode Time
Default release build 31.69s
Release build with exception marshalling disabled 2.55s
Release build with P/Invoke wrappers 2.45s

So I think the P/Invoke wrappers is the way to go here, without having to disable exception marshalling (it's even faster!):

<PropertyGroup>
    <MtouchExtraArgs>--require-pinvoke-wrappers:true</MtouchExtraArgs>
</PropertyGroup>

.NET 8 - Themes automation moved this from Optimizations to Done Sep 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dotnet An issue or pull request related to .NET (6) enhancement The issue or pull request is an enhancement
Projects
No open projects
Development

No branches or pull requests

3 participants