WIP: Merge System.Threading.Thread into Internal.Runtime.Augments.RuntimeThread. #22032

filipnavara · 2019-01-17T09:49:07Z

This is just a proof of concept. It will likely break VS Debugger in its current shape, but it depends on how the VS Debugger accesses the _managedThreadId field.

Here's a background on what I am trying to solve. It was already discussed in mono/mono#12470 (comment), but I will restate it here for clarity.

Right now CoreFX implements System.Threading.Thread using calls to Internal.Runtime.Augments.RuntimeThread in CoreLib. The CoreRT implementation is quite clean, but for historical reasons the CoreCLR implementation is not. It's split into RuntimeThread and another internal System.Threading.Thread type in CoreLib. This is an unfortunate situation where two System.Threading.Thread types exist, with different, but overlapping, functionality, in two different assemblies. To add insult to the injury some types in CoreLib reference the System.Threading.Thread directly instead of RuntimeThread, which makes code sharing with CoreRT/Mono difficult.

There are three proposed solutions for a cleanup:

Get rid of System.Threading.Thread in CoreLib and fold its functionality into RuntimeThread. This is implemented by this PR. Additionally, RuntimeThread namespace should be changed to Internal.Threading.RuntimeThread to make it match the convention used for most types like that (not done in this PR and will require changes to CoreRT and CoreFX too).
Keep System.Threading.Thread in CoreLib, but ensure that it is called only from RuntimeThread and unmanaged code. All other code would reference RuntimeThread only.
Move System.Threading.Thread from CoreFX back to CoreLib, including its transitive dependency closure and get rid of RuntimeThread.

Option 2) seems to be least favorable. The only thing going for it is that it won't break VS debugger and it's very limited in scope to the CoreCLR repository. Ultimately it doesn't solve the problem with two classes with the same name though.

Option 3) will require most shuffling of code between repositories.

…hread.

…ng to a Thread type in current namespace.

jkotas · 2019-01-17T22:11:52Z

cc @marek-safar

marek-safar · 2019-01-17T22:21:14Z

src/System.Private.CoreLib/src/Internal/Runtime/Augments/RuntimeThread.cs

@@ -14,22 +14,135 @@ namespace Internal.Runtime.Augments
 {
    public class RuntimeThread : CriticalFinalizerObject
    {
+        /*=========================================================================


Hmm, there is more code duplicating with CoreFX Thread that I expected

Do you have anything specific you have in mind?

jkotas · 2019-01-19T20:01:17Z

Let's wait for the Environment CoreFX->CoreCLR move to finish first, and then decide which option to take for this one.

filipnavara · 2019-01-20T17:32:17Z

Sure. This is low-priority one, but I decided to tackle it because it bit me couple of times that the class name resolution didn't work as I expected (ie. using Thread = ... was effectively ignored because the class was in the System.Threading namespace). I'll be happy to help with a solution (whichever one you decide on) once the work on Environment is finished.

filipnavara · 2019-02-17T20:31:48Z

Any new thoughts about this?

marek-safar · 2019-02-19T10:12:38Z

@jkotas @stephentoub could you please advice Filip on future steps

jkotas · 2019-02-19T13:13:53Z

I think we should bring the public System.Threading.Thread with its transitive closure back to CoreLib. @stephentoub @marek-safar Do you agree?

If we go with this plan, the first step should be to identify all files in CoreFX that need to be moved to CoreLib and move them to CoreLib shared partition in the CoreFX repo. (Except Thread.cs itself since it will require refactoring to integrate back to CoreLib.)

stephentoub · 2019-02-19T13:31:51Z

I think we should bring the public System.Threading.Thread with its transitive closure back to CoreLib. @stephentoub @marek-safar Do you agree?

In theory, yes. It'd be good to see the full transitive list before pulling the trigger. From a quick skim, it looks like the worst of it would be some permissions stuff via AppDomain, which should probably be in Corelib, anyway.

marek-safar · 2019-02-19T22:28:56Z

I think we should bring the public System.Threading.Thread with its transitive closure back to CoreLib.

This makes sense, all VM specific types are in CoreLib and this will hopefully make few dependencies simpler as well. I didn't check Thread dependencies chain but it's hopefully not too bad.

filipnavara · 2019-02-20T10:31:03Z

Here's the list:

System.Runtime.Extensions\src\System\CannotUnloadAppDomainException.cs
System.Runtime.Extensions\src\System\AppDomain.cs
System.Runtime.Extensions\src\System\AppDomainSetup.cs
System.Runtime.Extensions\src\System\Collections\ArrayList.cs
System.Runtime.Extensions\src\System\Security\IPermission.cs
System.Runtime.Extensions\src\System\Security\ISecurityEncodable.cs
System.Runtime.Extensions\src\System\Security\IStackWalk.cs
System.Runtime.Extensions\src\System\Security\PermissionSet.cs
System.Runtime.Extensions\src\System\Security\Permissions\PermissionState.cs
System.Runtime.Extensions\src\System\Security\SecurityElement.cs
System.Security.Principal\src\System\Security\Principal\IIdentity.cs
System.Security.Principal\src\System\Security\Principal\IPrincipal.cs
System.Security.Principal\src\System\Security\Principal\PrincipalPolicy.cs
System.Threading.Thread\src\System\LocalDataStoreSlot.cs
System.Threading.Thread\src\System\Threading\CompressedStack.cs
System.Threading.Thread\src\System\Threading\Thread.cs
System.Threading.Thread\src\System\Threading\Thread.Unix.cs
System.Threading.Thread\src\System\Threading\Thread.Windows.cs

filipnavara · 2019-02-20T12:38:19Z

I tried copying the above files into CoreRT's System.Private.CoreLib to see if it can compile. I've got to the point where only resource strings are missing, so the list above should be fairly complete.

stephentoub · 2019-02-20T12:53:42Z

Thanks for the list.

System.Runtime.Extensions\src\System\Collections\ArrayList.cs

Is this actually exposed through API, or could the implementation just use List<object>?

filipnavara · 2019-02-20T13:03:57Z

Is this actually exposed through API, or could the implementation just use List<object>?

Unfortunately it is exposed by SecurityElement. It's the biggest dependency of them all.

jkotas · 2019-02-20T15:14:30Z

ArrayList is old sibling of Hashtable. I do not feel bad about having it in CoreLib.

filipnavara · 2019-02-20T15:33:21Z

I'm going to close this PR, but feel free to continue discussion.

Is anyone planning to work on it? I'm notoriously bad at multi-repository code transfers, but if you are willing to bear with me I can try to reproduce the same PR sequence as for the Environment transfer.

jkotas · 2019-02-20T16:18:12Z

We will be glad if you can help.

stephentoub · 2019-02-20T16:19:01Z

And if you need help and get stuck, please let me know; I'll be happy to.

filipnavara · 2019-02-20T16:41:29Z

Alright. I will get to work later today.

filipnavara force-pushed the runtimethread branch from 8dedeb8 to 693da9d Compare January 17, 2019 10:06

filipnavara closed this Jan 17, 2019

filipnavara reopened this Jan 17, 2019

filipnavara added 5 commits January 17, 2019 16:13

Merge System.Threading.Thread into Internal.Runtime.Augments.RuntimeT…

e63cf03

…hread.

Always specify RuntimeThread explicitly to avoid accidentally resolvi…

a8a97c7

…ng to a Thread type in current namespace.

Update unmanaged references to System.Threading.Thread.

3a3b013

Fix build.

3f7f414

Add missing extern.

e433d8b

filipnavara force-pushed the runtimethread branch from d7c062e to e433d8b Compare January 17, 2019 15:13

marek-safar reviewed Jan 17, 2019

View reviewed changes

filipnavara mentioned this pull request Feb 4, 2019

Move SynchronizationContext to shared partition #22389

Merged

filipnavara closed this Feb 20, 2019

filipnavara mentioned this pull request Feb 20, 2019

Move Thread to shared CoreLib dotnet/corefx#35462

Merged

WIP: Merge System.Threading.Thread into Internal.Runtime.Augments.RuntimeThread. #22032

WIP: Merge System.Threading.Thread into Internal.Runtime.Augments.RuntimeThread. #22032

Uh oh!

Conversation

filipnavara commented Jan 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jkotas commented Jan 17, 2019

Uh oh!

marek-safar Jan 17, 2019

Choose a reason for hiding this comment

Uh oh!

jkotas Jan 19, 2019

Choose a reason for hiding this comment

Uh oh!

jkotas commented Jan 19, 2019

Uh oh!

filipnavara commented Jan 20, 2019

Uh oh!

filipnavara commented Feb 17, 2019

Uh oh!

marek-safar commented Feb 19, 2019

Uh oh!

jkotas commented Feb 19, 2019

Uh oh!

stephentoub commented Feb 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marek-safar commented Feb 19, 2019

Uh oh!

filipnavara commented Feb 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

filipnavara commented Feb 20, 2019

Uh oh!

stephentoub commented Feb 20, 2019

Uh oh!

filipnavara commented Feb 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jkotas commented Feb 20, 2019

Uh oh!

filipnavara commented Feb 20, 2019

Uh oh!

jkotas commented Feb 20, 2019

Uh oh!

stephentoub commented Feb 20, 2019

Uh oh!

filipnavara commented Feb 20, 2019

Uh oh!

Uh oh!

filipnavara commented Jan 17, 2019 •

edited

Loading

stephentoub commented Feb 19, 2019 •

edited

Loading

filipnavara commented Feb 20, 2019 •

edited

Loading

filipnavara commented Feb 20, 2019 •

edited

Loading