[foundation] Cache parts of NSObject.ConformsToProtocol
#15294
Conversation
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`.
There was a problem hiding this comment.
This is some deep magic, and really needs @rolfbjarne 's 👀 , but seems reasonable to me.
Cute map = new (); btw.
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
This comment has been minimized.
This comment has been minimized.
✅ API diff for current PR / commitLegacy Xamarin (No breaking changes)
NET (empty diffs)
✅ API diff vs stableLegacy Xamarin (No breaking changes).NET (No breaking changes)✅ Generator diffGenerator diff is empty Pipeline on Agent |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
src/Foundation/NSObject2.cs
Outdated
| // 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) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
We don't use ConcurrentDictionary anywhere else, so that would mean it could never be linked away.
There was a problem hiding this comment.
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.
This comment has been minimized.
This comment has been minimized.
|
/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.
✅ API diff for current PR / commitLegacy Xamarin (No breaking changes)
NET (empty diffs)
✅ API diff vs stableLegacy Xamarin (No breaking changes).NET (No breaking changes)✅ Generator diffGenerator diff is empty Pipeline on Agent |
This comment has been minimized.
This comment has been minimized.
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
💻 [PR Build] Tests on macOS Mac Catalina (10.15) passed 💻✅ All tests on macOS Mac Catalina (10.15) passed. Pipeline on Agent |
✅ API diff for current PR / commitLegacy Xamarin (No breaking changes)
NET (empty diffs)
✅ API diff vs stableLegacy Xamarin (No breaking changes).NET (No breaking changes)✅ Generator diffGenerator diff is empty Pipeline on Agent |
💻 [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 |
rolfbjarne
added
community
|
Test failure is unrelated (https://github.com/xamarin/maccore/issues/2558). |
Note that the call to native code still always happen (not cached) since the application could use
class_addProtocolto 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
RemoveFromSuperview7.99s (6.4%)NSObject.ConformsToProtocol3.26s (2.6%)After
RemoveFromSuperview4.67s (3.8%)NSObject.ConformsToProtocol0.32s (.26%)So a 10x improvements on
ConformsToProtocolwhich helps a lot the code path callingRemoveFromSuperview.