[coreclr] Support for NSAutoreleasePools has now been implemented for background threads for both MonoVM and CoreCLR. Fixes #11256.#11749
Conversation
… background threads for both MonoVM and CoreCLR.
| // Strangely enough there seems to be a race condition here, not all threads will necessarily | ||
| // have completed the autorelease by this point. Some should have though, so assert that the object | ||
| // was released on at least one thread. | ||
| Assert.That ((int) obj.RetainCount, Is.LessThan (10), "RC"); |
There was a problem hiding this comment.
Use count, which could be made a const, instead of 10.
so assert that the object was released on at least one thread.
Just by reading the NSAutoreleasePoolInThread I'm not convinced it shows the feature to be working.
E.g. If only done by a single thread (out of 10) then the issue might not be a race. It could be that it somehow get called just once.
If you use count - 1 then we know (but reading the test, not the code) this works on multiple (not just the first) threads.
There was a problem hiding this comment.
Use
count, which could be made aconst, instead of10.
Done.
If you use
count - 1then we know (but reading the test, not the code) this works on multiple (not just the first) threads.
I decided to be braver and used count / 2 instead 😄
This comment has been minimized.
This comment has been minimized.
| @@ -244,8 +244,6 @@ | |||
| void | |||
| xamarin_install_nsautoreleasepool_hooks () | |||
There was a problem hiding this comment.
@dalexsoto Because nothing needs to happen where - there's another version of this method for MonoVM:
I've added a comment to explain this.
| { | ||
| // https://github.com/xamarin/xamarin-macios/issues/11256 | ||
| fprintf (stderr, "TODO: add support for wrapping all threads with NSAutoreleasePools.\n"); | ||
| // No need to do anything here for CoreCLR. |
There was a problem hiding this comment.
it'd be good to do the same for Mono too in .NET6 so we're consistent (and not rely on the mono profiler hook to install this)
There was a problem hiding this comment.
@akoeplinger would that require any changes in Mono that we have to wait for? Or is that already implemented?
There was a problem hiding this comment.
@rolfbjarne it should already be implemented
Ok, filed: #11788
This comment has been minimized.
This comment has been minimized.
…s-background-threads
✅ [PR Build] Tests passed on Build. ✅Tests passed on Build. API diff✅ API Diff from stable View API diffAPI & Generator diff✅ API Diff (from PR only) (no change) GitHub pagesResults can be found in the following github pages (it might take some time to publish): 🎉 All 108 tests passed 🎉Pipeline on Agent XAMBOT-1104.BigSur' |
Fixes #11256.