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

NSAutoreleasePool instance should be added to all threads. #52023

Merged

Conversation

AaronRobinsonMSFT
Copy link
Member

@AaronRobinsonMSFT AaronRobinsonMSFT commented Apr 28, 2021

This expands the current support where support was only added to ThreadPool threads. Now all managed threads or new native threads entering the runtime will get an NSAutoreleasePool allocated.

New feature switch, "System.Threading.Thread.EnableAutoreleasePool", was created and the "System.Threading.ThreadPool.EnableDispatchAutoreleasePool" was removed.

Updated AutoReleaseTest for the new scenarios. Also added support for tests to pass Runtime Configuration Options to corerun - uses the syntax found here - @LakshanF, @BruceForstall.

This is a follow up PR started with #47592

Contributes to xamarin/xamarin-macios#11256

/cc @rolfbjarne @jkotas @jkoritzinsky @elinor-fung

This expands the current support where support was only added to
ThreadPool threads. Now all managed threads or new native threads
entering the runtime will get an NSAutoreleasePool allocated.

New feature switch was created and the ThreadPool one was removed.
 - System.Threading.Thread.EnableAutoreleasePool

Updated AutoReleaseTest for the new scenarios.
…ool.

If feature is not enabled, allocating a NSAutoreleasePool is a no-op.
@AaronRobinsonMSFT
Copy link
Member Author

@lambdageek I took a quick look at how to call these P/Invokes in mono. It isn't obvious to me how to try adding support. My best guess is around

/* On 2.0 profile (and higher), set explicitly since state might have been
Unknown */
if (internal->apartment_state == ThreadApartmentState_Unknown)
internal->apartment_state = ThreadApartmentState_MTA;
mono_thread_init_apartment_state ();
/* Let the thread that called Start() know we're ready */
mono_coop_sem_post (&start_info->registered);
if (mono_atomic_dec_i32 (&start_info->ref) == 0) {
mono_coop_sem_destroy (&start_info->registered);
g_free (start_info);
}

If you think this support should be easy to add, I'm willing to take a few hours to try it out.

@lambdageek
Copy link
Member

lambdageek commented Apr 29, 2021

@lambdageek I took a quick look at how to call these P/Invokes in mono. It isn't obvious to me how to try adding support. My best guess is around

https://github.com/dotnet/runtime/blob/01b7e73cd378145264a7cb7a09365b41ed42b240/src/mono/mono/metadata/threads.c#L1160-L1173

If you think this support should be easy to add, I'm willing to take a few hours to try it out.

What needs to happen? a call to Thread.AllocateThreadlocalAutoreleasePool ?

yea I think the right place is right before both callers of fire_attach_profiler_events. the spot you found is one, the other is

fire_attach_profiler_events (tid);

That one would set up an autorelease pool when a foreign thread attaches to the runtime via a pinvoke callback. (eg: managed code calls a pinvoke and passes it a delegate (or an UnmanagedCallersOnly function pointer), the native code creates a new thread and calls the unmanaged function pointer from the new thread).

In general the pattern to follow is like this:

MONO_STATIC_POINTER_INIT (MonoMethod, thread_exiting)
thread_exiting = mono_class_get_method_from_name_checked (mono_defaults.thread_class, "OnThreadExiting", -1, 0, error);
mono_error_assert_ok (error);
MONO_STATIC_POINTER_INIT_END (MonoMethod, thread_exiting)
g_assert (thread_exiting);
if (mono_runtime_get_no_exec ())
return;
HANDLE_FUNCTION_ENTER ();
ERROR_DECL (error);
gpointer args [1];
args [0] = internal;
mono_runtime_try_invoke_handle (thread_exiting, NULL_HANDLE, args, error);
mono_error_cleanup (error);

(some things to note: 1. if the method may not be present (removed by the IL Linker, or not defined on every plaform), don't assert; 2. mono_runtime_get_no_exec (check if we're the AOT compiler) is probably needed - a little bit of EE initialization happens even just during AOT compilation, and some of this thread attach code probably runs. 3. i'm assuming we want to swallow any managed exceptions, so the final mono_error_cleanup will swallow them. 4. since the method takes no arguments, the HANDLE_FUNCTION_ENTER/EXIT business is not needed and the actual call is more like mono_runtime_invoke_checked (method, NULL, NULL, error) )

src/coreclr/vm/threads.cpp Outdated Show resolved Hide resolved
Co-authored-by: Jan Kotas <jkotas@microsoft.com>
@AaronRobinsonMSFT

This comment has been minimized.

needed or possible in some cases (e.g. GC).
src/coreclr/vm/threads.h Outdated Show resolved Hide resolved
src/coreclr/vm/threads.h Outdated Show resolved Hide resolved
src/coreclr/vm/threads.h Outdated Show resolved Hide resolved
Move main thread initialization/cleanup closer to when Main is called.
src/coreclr/vm/ceemain.cpp Outdated Show resolved Hide resolved
src/coreclr/vm/threads.h Outdated Show resolved Hide resolved
Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM otherwise. Thank you!

@AaronRobinsonMSFT
Copy link
Member Author

@lambdageek I've applied the feedback to Mono. Let me know what you think.

Copy link
Member

@lambdageek lambdageek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks!

@AaronRobinsonMSFT AaronRobinsonMSFT merged commit 60735c6 into dotnet:main May 10, 2021
@AaronRobinsonMSFT AaronRobinsonMSFT deleted the autoreleasepool_per_thread branch May 10, 2021 20:31
@rolfbjarne
Copy link
Member

@akoeplinger yes, I believe so (yay!)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants