[foundation] Cache parts of `NSObject.ConformsToProtocol` by spouliot · Pull Request #15294 · dotnet/macios

spouliot · 2022-06-17T01:00:10Z

Note that the call to native code still always happen (not cached) since the application could use class_addProtocol to add conformance to a protocol at runtime.

So the cache is limited to the .net specific reflection code that is present (only) when the dynamic registrar is included inside applications. This is the default for macOS apps, but not iOS / tvOS or MacCatalyst apps.

The linker/trimmer will remove the caching code when the dynamic registrar is removed. IOW this PR should not have any impact, performance or size, for most iOS apps (where the dynamic registrar is removed by default).

Fix #14065

Running Dope on macOS, a 2 minutes benchmark, shows the following times (in seconds and percentage) spent calling this API:

Before

RemoveFromSuperview 7.99s (6.4%)
- NSObject.ConformsToProtocol 3.26s (2.6%)

After

RemoveFromSuperview 4.67s (3.8%)
- NSObject.ConformsToProtocol 0.32s (.26%)

So a 10x improvements on ConformsToProtocol which helps a lot the code path calling RemoveFromSuperview.

Note that the call to native code still _always_ happen (not cached) since the application could use `class_addProtocol` to add conformance to a protocol at runtime. So the cache is limited to the .net specific reflection code that is present (only) when the dynamic registrar is included inside applications. This is the default for macOS apps, but not iOS / tvOS or MacCatalyst apps. The linker/trimmer will remove the caching code when the dynamic registrar is removed. IOW this PR should not have any impact, performance or size, for most iOS apps (where the dynamic registrar is removed by default). Fix dotnet#14065 Running Dope on macOS, a 2 minutes benchmark, shows the following times (in seconds and percentage) spent calling this API: **Before** * `RemoveFromSuperview` 7.99s (6.4%) * `NSObject.ConformsToProtocol` 3.26s (2.6%) **After** * `RemoveFromSuperview` 4.67s (3.8%) * `NSObject.ConformsToProtocol` 0.32s (.26%) So a 10x improvements on `ConformsToProtocol` which helps a lot the code path calling `RemoveFromSuperview`.

chamons

This is some deep magic, and really needs @rolfbjarne 's 👀 , but seems reasonable to me.

Cute map = new (); btw.

rolfbjarne

This is great!

Just a few minor things

rolfbjarne · 2022-06-17T20:29:17Z

/azp run

azure-pipelines · 2022-06-17T20:29:58Z

Azure Pipelines successfully started running 1 pipeline(s).

…NET is defined)

vs-mobiletools-engineering-service2 · 2022-06-17T22:07:52Z

✅ API diff for current PR / commit

Legacy Xamarin (No breaking changes)

~~iOS~~ (no change detected)
~~tvOS~~ (no change detected)
~~watchOS~~ (no change detected)
~~macOS~~ (no change detected)

NET (empty diffs)

~~iOS~~: (empty diff detected)
~~tvOS~~: (empty diff detected)
~~MacCatalyst~~: (empty diff detected)
~~macOS~~: (empty diff detected)

✅ API diff vs stable

Legacy Xamarin (No breaking changes)

iOS: vsdrops gist (No breaking changes)
tvOS: vsdrops gist (No breaking changes)
watchOS: vsdrops gist (No breaking changes)
macOS: vsdrops gist (No breaking changes)

.NET (No breaking changes)

iOS: vsdrops gist (No breaking changes)
tvOS: vsdrops gist (No breaking changes)
MacCatalyst: vsdrops gist (No breaking changes)
macOS: vsdrops gist (No breaking changes)
Microsoft.iOS vs Microsoft.MacCatalyst: vsdrops gist

Legacy Xamarin (stable) vs .NET

iOS: vsdrops gist
tvOS: vsdrops gist
macOS: vsdrops gist

✅ Generator diff

Generator diff is empty

Pipeline on Agent
Hash: ddf76f4b8daee8fedc72049f63278174aa482fe2 [PR build]

Therzok · 2022-06-18T02:29:57Z

+			// the linker/trimmer will remove the following code if the dynamic registrar is removed from the app
+			var classHandle = ClassHandle;
+			bool new_map = false;
+			lock (protocol_cache) {


Theoretically, the class map and the protocol map could use separate locks, but not sure how often this is called concurrently for it to matter.

Alternatively, ConcurrentDictionary - if it's already linked in - could also be a good choice.

We don't use ConcurrentDictionary anywhere else, so that would mean it could never be linked away.

Theoretically, the class map and the protocol map could use separate locks

Yes, but that did not help the benchmarks (which is totally not concurrent [1] since the calls all comes from the main/ui thread) and I prefer doing the simplest PR to achieve my objectives 😄

[1] but, beside my benchmark, this has to be safe to call from many threads (hence the lock)

Alternatively, ConcurrentDictionary

Same as @rolfbjarne mentioned, in addition to my non-concurrency comment.

Now if a different test case comes up where concurrency has a measurable impact then it would be worth to consider both suggestions.

rolfbjarne · 2022-06-21T06:28:19Z

/azp run

azure-pipelines · 2022-06-21T06:29:56Z

Azure Pipelines successfully started running 1 pipeline(s).

vs-mobiletools-engineering-service2 · 2022-06-21T08:10:09Z

✅ API diff for current PR / commit

Legacy Xamarin (No breaking changes)

~~iOS~~ (no change detected)
~~tvOS~~ (no change detected)
~~watchOS~~ (no change detected)
~~macOS~~ (no change detected)

NET (empty diffs)

~~iOS~~: (empty diff detected)
~~tvOS~~: (empty diff detected)
~~MacCatalyst~~: (empty diff detected)
~~macOS~~: (empty diff detected)

✅ API diff vs stable

Legacy Xamarin (No breaking changes)

iOS: vsdrops gist (No breaking changes)
tvOS: vsdrops gist (No breaking changes)
watchOS: vsdrops gist (No breaking changes)
macOS: vsdrops gist (No breaking changes)

.NET (No breaking changes)

iOS: vsdrops gist (No breaking changes)
tvOS: vsdrops gist (No breaking changes)
MacCatalyst: vsdrops gist (No breaking changes)
macOS: vsdrops gist (No breaking changes)
Microsoft.iOS vs Microsoft.MacCatalyst: vsdrops gist

Legacy Xamarin (stable) vs .NET

iOS: vsdrops gist
tvOS: vsdrops gist
macOS: vsdrops gist

✅ Generator diff

Generator diff is empty

Pipeline on Agent
Hash: d03f9362806fe3c75fe7b54508a2259df6522e6b [PR build]

…ve the field

rolfbjarne · 2022-06-21T14:42:16Z

/azp run

azure-pipelines · 2022-06-21T14:42:52Z

Azure Pipelines successfully started running 1 pipeline(s).

vs-mobiletools-engineering-service2 · 2022-06-21T16:21:40Z

💻 [PR Build] Tests on macOS Mac Catalina (10.15) passed 💻

✅ All tests on macOS Mac Catalina (10.15) passed.

Pipeline on Agent
Hash: 54246cb1365b40cc8f5eb163d688268295616831 [PR build]

vs-mobiletools-engineering-service2 · 2022-06-21T16:25:45Z

📚 [PR Build] Artifacts 📚

Packages generated

View packages

Pipeline on Agent XAMBOT-1035.Monterey
Hash: 54246cb1365b40cc8f5eb163d688268295616831 [PR build]

vs-mobiletools-engineering-service2 · 2022-06-21T16:26:31Z

✅ API diff for current PR / commit

Legacy Xamarin (No breaking changes)

~~iOS~~ (no change detected)
~~tvOS~~ (no change detected)
~~watchOS~~ (no change detected)
~~macOS~~ (no change detected)

NET (empty diffs)

~~iOS~~: (empty diff detected)
~~tvOS~~: (empty diff detected)
~~MacCatalyst~~: (empty diff detected)
~~macOS~~: (empty diff detected)

✅ API diff vs stable

Legacy Xamarin (No breaking changes)

iOS: vsdrops gist (No breaking changes)
tvOS: vsdrops gist (No breaking changes)
watchOS: vsdrops gist (No breaking changes)
macOS: vsdrops gist (No breaking changes)

.NET (No breaking changes)

iOS: vsdrops gist (No breaking changes)
tvOS: vsdrops gist (No breaking changes)
MacCatalyst: vsdrops gist (No breaking changes)
macOS: vsdrops gist (No breaking changes)
Microsoft.iOS vs Microsoft.MacCatalyst: vsdrops gist

Legacy Xamarin (stable) vs .NET

iOS: vsdrops gist
tvOS: vsdrops gist
macOS: vsdrops gist

✅ Generator diff

Generator diff is empty

Pipeline on Agent
Hash: 54246cb1365b40cc8f5eb163d688268295616831 [PR build]

vs-mobiletools-engineering-service2 · 2022-06-21T16:36:30Z

💻 [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
Hash: 54246cb1365b40cc8f5eb163d688268295616831 [PR build]

vs-mobiletools-engineering-service2 · 2022-06-21T23:13:28Z

❌ [CI Build] Tests failed on VSTS: simulator tests iOS ❌

Tests failed on VSTS: simulator tests iOS.

Test results

1 tests failed, 147 tests passed.

Failed tests

fsharp/watchOS 32-bits - simulator/Debug: Crashed

Pipeline on Agent XAMBOT-1023.Monterey'
Merge 54246cb into e3bc284

rolfbjarne · 2022-06-22T08:09:01Z

Test failure is unrelated (https://github.com/xamarin/maccore/issues/2558).

spouliot requested review from chamons and rolfbjarne as code owners June 17, 2022 01:00

spouliot mentioned this pull request Jun 17, 2022

[WIP] perf(macOS): Cache ConformsToProtocol results unoplatform/uno#9006

Closed

6 tasks

chamons reviewed Jun 17, 2022

View reviewed changes

rolfbjarne requested changes Jun 17, 2022

View reviewed changes

Comment thread src/Foundation/NSObject2.cs Outdated

Comment thread src/Foundation/NSObject2.cs Outdated

Apply feedback: Move variable to avoid compiler warning (unused when …

62cebea

…NET is defined)

This comment has been minimized.

Sign in to view

Therzok reviewed Jun 18, 2022

View reviewed changes

This comment has been minimized.

Sign in to view

Apply feedback

d03f936

rolfbjarne approved these changes Jun 21, 2022

View reviewed changes

Comment thread src/ObjCRuntime/Runtime.cs Outdated

This comment has been minimized.

Sign in to view

Apply feedback: better position protocol_cache so the linker can remo…

54246cb

…ve the field

dalexsoto approved these changes Jun 21, 2022

View reviewed changes

rolfbjarne added community Community contribution ❤ performance If an issue or pull request is related to performance labels Jun 22, 2022

rolfbjarne merged commit 7995740 into dotnet:main Jun 22, 2022

spouliot deleted the cache-nsobject-conformstoprotocol branch June 22, 2022 13:53

Conversation

spouliot commented Jun 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Before

After

Uh oh!

chamons left a comment

Choose a reason for hiding this comment

Uh oh!

rolfbjarne left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rolfbjarne commented Jun 17, 2022

Uh oh!

azure-pipelines Bot commented Jun 17, 2022

Uh oh!

This comment has been minimized.

vs-mobiletools-engineering-service2 commented Jun 17, 2022

✅ API diff for current PR / commit

✅ API diff vs stable

✅ Generator diff

Uh oh!

This comment has been minimized.

This comment has been minimized.

Therzok Jun 18, 2022

Choose a reason for hiding this comment

Uh oh!

rolfbjarne Jun 20, 2022

Choose a reason for hiding this comment

Uh oh!

spouliot Jun 21, 2022

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

Uh oh!

rolfbjarne commented Jun 21, 2022

Uh oh!

azure-pipelines Bot commented Jun 21, 2022

Uh oh!

This comment has been minimized.

This comment has been minimized.

vs-mobiletools-engineering-service2 commented Jun 21, 2022

✅ API diff for current PR / commit

✅ API diff vs stable

✅ Generator diff

Uh oh!

This comment has been minimized.

rolfbjarne commented Jun 21, 2022

Uh oh!

azure-pipelines Bot commented Jun 21, 2022

Uh oh!

vs-mobiletools-engineering-service2 commented Jun 21, 2022

💻 [PR Build] Tests on macOS Mac Catalina (10.15) passed 💻

Uh oh!

vs-mobiletools-engineering-service2 commented Jun 21, 2022

📚 [PR Build] Artifacts 📚

Packages generated

Uh oh!

vs-mobiletools-engineering-service2 commented Jun 21, 2022

✅ API diff for current PR / commit

✅ API diff vs stable

✅ Generator diff

Uh oh!

vs-mobiletools-engineering-service2 commented Jun 21, 2022

💻 [PR Build] Tests on macOS M1 - Mac Big Sur (11.5) passed 💻

Uh oh!

vs-mobiletools-engineering-service2 commented Jun 21, 2022

❌ [CI Build] Tests failed on VSTS: simulator tests iOS ❌

Test results

Failed tests

Uh oh!

rolfbjarne commented Jun 22, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

spouliot commented Jun 17, 2022 •

edited

Loading