NSAutoreleasePool instance should be added to all threads. by AaronRobinsonMSFT · Pull Request #52023 · dotnet/runtime

AaronRobinsonMSFT · 2021-04-28T23:34:23Z

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 dotnet/macios#11256

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

item and pass it to corerun.

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.

src/libraries/Native/Unix/System.Native/pal_autoreleasepool.m

src/libraries/System.Private.CoreLib/src/System/Threading/Thread.OSX.cs

…ool. If feature is not enabled, allocating a NSAutoreleasePool is a no-op.

AaronRobinsonMSFT · 2021-04-29T02:39:26Z

@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

runtime/src/mono/mono/metadata/threads.c

Lines 1160 to 1173 in 01b7e73

    
           	/* 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 · 2021-04-29T03:08:37Z

@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

runtime/src/mono/mono/metadata/threads.c

Lines 1600 to 1601 in 01b7e73


	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:

runtime/src/mono/mono/metadata/threads.c

Lines 878 to 898 in 01b7e73

    
           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) )

linker can keep even though the runtime won't call the function.

src/coreclr/vm/threads.cpp

src/tests/Interop/ObjectiveC/autorelease.mm

Co-authored-by: Jan Kotas <jkotas@microsoft.com>

needed or possible in some cases (e.g. GC).

src/coreclr/vm/threads.h

with the thread type needed - GC.

src/coreclr/vm/threads.h

docs/workflow/trimming/feature-switches.md

Remove GetUniqueEEId() logic. Remove check for AbortRequest from TransferStartedThread(). - This can be done because it is only called from HasStarted() so the user has no chance to make the request and we don't support custom hosting. - The only other place this work occurs is during a FuncEval - which again means the thread is running manage code and after the call to the HasStarted() call.

src/coreclr/vm/threads.cpp

src/libraries/System.Private.CoreLib/src/System/Threading/Thread.cs

Move main thread initialization/cleanup closer to when Main is called.

src/mono/System.Private.CoreLib/src/ILLink/ILLink.Descriptors.xml

src/coreclr/vm/ceemain.cpp

src/coreclr/vm/threads.h

src/libraries/System.Private.CoreLib/src/System/Threading/Thread.OSX.cs

jkotas

LGTM otherwise. Thank you!

src/libraries/System.Private.CoreLib/src/System/Threading/AutoreleasePool.cs

eventually be when the Objective-C interop support is added.

AaronRobinsonMSFT · 2021-05-10T14:48:42Z

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

lambdageek

LGTM. Thanks!

rolfbjarne · 2021-05-11T12:37:36Z

@akoeplinger yes, I believe so (yay!)

AaronRobinsonMSFT added 2 commits April 28, 2021 12:05

Platform scripts now respect the MSBuild RuntimeHostConfigurationOption

52680ba

item and pass it to corerun.

AaronRobinsonMSFT added the area-Interop-coreclr label Apr 28, 2021

AaronRobinsonMSFT added this to the 6.0.0 milestone Apr 28, 2021

jkoritzinsky reviewed Apr 28, 2021

View reviewed changes

src/libraries/Native/Unix/System.Native/pal_autoreleasepool.m Outdated Show resolved Hide resolved

Build issues.

66110bc

jkotas reviewed Apr 29, 2021

View reviewed changes

src/libraries/Native/Unix/System.Native/pal_autoreleasepool.m Outdated Show resolved Hide resolved

jkotas reviewed Apr 29, 2021

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/Threading/Thread.OSX.cs Outdated Show resolved Hide resolved

jkotas reviewed Apr 29, 2021

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/Threading/Thread.OSX.cs Outdated Show resolved Hide resolved

AaronRobinsonMSFT added 2 commits April 28, 2021 19:03

Remove unnecessary new native export for allocating an NSAutoreleaseP…

4b2e859

…ool. If feature is not enabled, allocating a NSAutoreleasePool is a no-op.

Update header.

b34e3f3

rolfbjarne approved these changes Apr 29, 2021

View reviewed changes

rolfbjarne mentioned this pull request Apr 29, 2021

CoreCLR: NSAutoreleasePool for every thread dotnet/macios#11256

Closed

AaronRobinsonMSFT added 4 commits April 29, 2021 17:27

Initialize for platform prior to transferring control.

97be019

Address Linker issues by defining a throwing version of the API the

9827a3d

linker can keep even though the runtime won't call the function.

Update files for building based on their names.

543ed7a

TargetsUnix is also set for OSX - sigh.

83ae701

jkotas reviewed May 1, 2021

View reviewed changes

src/coreclr/vm/threads.cpp Outdated Show resolved Hide resolved

jkotas reviewed May 1, 2021

View reviewed changes

src/tests/Interop/ObjectiveC/autorelease.mm Outdated Show resolved Hide resolved

Update src/coreclr/vm/threads.cpp

81c532a

Co-authored-by: Jan Kotas <jkotas@microsoft.com>

This comment has been minimized.

Sign in to view

Handle other cases where specific platform initialization isn't

6258fc6

needed or possible in some cases (e.g. GC).

jkotas reviewed May 2, 2021

View reviewed changes

src/coreclr/vm/threads.h Outdated Show resolved Hide resolved

Remove the high level ThreadStoreLock flag and instead associate it

17d10bd

with the thread type needed - GC.

jkotas reviewed May 2, 2021

View reviewed changes

src/coreclr/vm/threads.h Outdated Show resolved Hide resolved

jkotas reviewed May 2, 2021

View reviewed changes

src/coreclr/vm/threads.h Outdated Show resolved Hide resolved

marek-safar reviewed May 3, 2021

View reviewed changes

docs/workflow/trimming/feature-switches.md Show resolved Hide resolved

jkotas reviewed May 8, 2021

View reviewed changes

src/coreclr/vm/threads.cpp Outdated Show resolved Hide resolved

jkotas reviewed May 8, 2021

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/Threading/Thread.cs Outdated Show resolved Hide resolved

AaronRobinsonMSFT added 2 commits May 7, 2021 23:15

Review feedback.

6d6257e

Add cleanup code to Finalizer.

b42024e

Move main thread initialization/cleanup closer to when Main is called.

marek-safar reviewed May 8, 2021

View reviewed changes

src/mono/System.Private.CoreLib/src/ILLink/ILLink.Descriptors.xml Outdated Show resolved Hide resolved

AaronRobinsonMSFT added 2 commits May 8, 2021 12:32

Add Descriptors file for OSX.

3030488

Fix contract for new managed thread initialization from native.

14f97d9