[perf] Cache Runtime.IsUserType results
#15149
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is computed **twice** _per instance_, when the instance is * created: `NSObject.CreateManagedRef` * released: `NSObject.ReleaseManagedRef` However its (boolean) value remains identical for all instances of the same type. This shows up as consuming a lot of time in the logs attached to xamarin#15145 Basically two things are cached * the selector for `xamarinSetGCHandle:flags:`, which makes it 15% faster; * some platforms, but not macOS, have an optimization for the `Selector.GetHandle` API * the result is cached into a `Dictionrary<IntPtr,bool>` Note that the optimizations are not specific to CoreCLR nor macOS (so they are not fixes for the CoreCLR performance regression of the above mentioned issue). OTOH it will also help performance for legacy and other net6 (mono) platforms. ``` BenchmarkDotNet=v0.12.1, OS=macOS 12.3.1 (21E258) [Darwin 21.4.0] Apple M1, 1 CPU, 8 logical and 8 physical cores .NET Core SDK= 6.0.100 [/usr/local/share/dotnet/sdk] [Host] : .NET Core 6.0 (CoreCLR 6.0.522.21309, CoreFX 6.0.522.21309), Arm64 RyuJIT Job=InProcess Toolchain=InProcessEmitToolchain IterationCount=3 LaunchCount=1 WarmupCount=3 ``` | Method | Length | Mean | Error | StdDev | Ratio | |----------------- |------- |---------------:|-------------:|------------:|------:| | Original | 16 | 7,729.8 ns | 212.61 ns | 11.65 ns | 1.00 | | CachedSelector | 16 | 6,552.6 ns | 202.70 ns | 11.11 ns | 0.85 | | CachedIsUserType | 16 | 162.0 ns | 14.86 ns | 0.81 ns | 0.02 | | | | | | | | | Original | 256 | 123,183.0 ns | 4,724.95 ns | 258.99 ns | 1.00 | | CachedSelector | 256 | 104,570.3 ns | 2,029.20 ns | 111.23 ns | 0.85 | | CachedIsUserType | 256 | 2,489.5 ns | 390.86 ns | 21.42 ns | 0.02 | | | | | | | | | Original | 4096 | 1,970,381.7 ns | 66,393.09 ns | 3,639.23 ns | 1.00 | | CachedSelector | 4096 | 1,676,773.0 ns | 12,149.92 ns | 665.98 ns | 0.85 | | CachedIsUserType | 4096 | 39,933.3 ns | 7,426.74 ns | 407.08 ns | 0.02 | [Benchmark source code](https://gist.github.com/spouliot/42fd43e94c5a9ce90164f0f6f9b35018)
This was referenced May 29, 2022
Closed
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
dalexsoto
added
community
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Therzok
reviewed
May 30, 2022
rolfbjarne
requested changes
May 30, 2022
src/ObjCRuntime/Runtime.cs
Outdated
| internal static bool IsUserType (IntPtr self) | ||
| { | ||
| var cls = Class.object_getClass (self); | ||
| if (!usertype_cache.TryGetValue (cls, out var result)) { |
There was a problem hiding this comment.
This would need a lock to protect against race conditions.
There was a problem hiding this comment.
Would a ConditionalWeakTable work here instead? I imagine class objects are bound to application lifetime.
There was a problem hiding this comment.
Both the key and the value are value types, and I believe ConditionalWeakTable only works with reference types.
Also the key represents the native Class object, and that will never be freed, so I don't see much value in trying to keep track of lifetimes on the managed side.
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Looks like that ran into trouble: Seems to happen when running monotouch-test on .NET (but not legacy Xamarin for some reason). |
rolfbjarne
reviewed
May 31, 2022
src/ObjCRuntime/Runtime.cs
Outdated
| internal static bool IsUserType (IntPtr self) | ||
| { | ||
| var cls = Class.object_getClass (self); | ||
| lock (usertype_cache) { | ||
| #if NET | ||
| var result = CollectionsMarshal.GetValueRefOrAddDefault (usertype_cache, cls, out var exists); |
There was a problem hiding this comment.
I think this is needed to fix the test failures:
Suggested change
| var result = CollectionsMarshal.GetValueRefOrAddDefault (usertype_cache, cls, out var exists); | |
| ref var result = ref CollectionsMarshal.GetValueRefOrAddDefault (usertype_cache, cls, out var exists); |
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
💻 [PR Build] Tests on macOS Mac Catalina (10.15) passed 💻✅ All tests on macOS Mac Catalina (10.15) passed. Pipeline on Agent |
💻 [PR Build] Tests on macOS Mac Catalina (10.15) passed 💻✅ All tests on macOS Mac Catalina (10.15) passed. Pipeline on Agent |
📋 [PR Build] API Diff 📋API diff (for current PR)ℹ️ API Diff (from PR only) (please review changes) .NETXamarin vs .NETAPI diff (vs stable)✅ API Diff from stable .NETXamarin vs .NETGenerator diff✅ Generator Diff (no change) Pipeline on Agent XAMBOT-1164.Monterey' |
💻 [PR Build] Tests on macOS M1 - Mac Big Sur (11.5) passed 💻✅ All tests on macOS M1 - Mac Big Sur (11.5) passed. Pipeline on Agent |
📋 [PR Build] API Diff 📋API diff (for current PR)ℹ️ API Diff (from PR only) (please review changes) .NETXamarin vs .NETAPI diff (vs stable)✅ API Diff from stable .NETXamarin vs .NETGenerator diff✅ Generator Diff (no change) Pipeline on Agent XAMBOT-1166.Monterey |
❌ [PR Build] Tests on macOS M1 - Mac Big Sur (11.5) failed ❌Failed tests are:
Pipeline on Agent |
❌ [CI Build] Tests failed on VSTS: simulator tests iOS ❌Tests failed on VSTS: simulator tests iOS. Test results3 tests failed, 145 tests passed.Failed tests
Pipeline on Agent XAMBOT-1108.Monterey' |
|
Test failures are unrelated:
|
rolfbjarne
approved these changes
Jun 1, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is computed twice per instance, when the instance is
NSObject.CreateManagedRefNSObject.ReleaseManagedRefHowever its (boolean) value remains identical for all instances of the same type.
This shows up as consuming a lot of time in the logs attached to #15145
Basically two things are cached
xamarinSetGCHandle:flags:, which makes it 15% faster;Selector.GetHandleAPIDictionrary<IntPtr,bool>Note that the optimizations are not specific to CoreCLR nor macOS (so they are not fixes for the CoreCLR performance regression of the above mentioned issue).
OTOH it will also help performance for legacy and other net6 (mono) platforms.
Benchmark source code