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

[mono] Remove more multi-domain functionality #48023

Closed
wants to merge 4 commits into from

Conversation

CoffeeFlux
Copy link
Contributor

Still an issue in method to ir that I need to figure out, so draft.

@ghost
Copy link

ghost commented Feb 8, 2021

Tagging subscribers to this area: @CoffeeFlux
See info in area-owners.md if you want to be subscribed.

Issue Details

Still an issue in method to ir that I need to figure out, so draft.

Author: CoffeeFlux
Assignees: -
Labels:

area-VM-meta-mono

Milestone: -

@vargaz
Copy link
Contributor

vargaz commented Feb 9, 2021

Note that whenever the current domain is set is used to detect whenever a thread is attached to the runtime or not in a couple of places, so the 'current_domain' TLS var and its set/get cannot be currently removed.

@lambdageek
Copy link
Member

I agree with Zoltan, I don't think we should get rid of the domain TLS var yet.

I think we'll end up living with MonoDomain for a while. But we should think of it as MonoRootDomain or MonoRoot or something - it's basically a singleton type now. It's still useful to have as a place to put global state, and for now it's ok to store a reference to it in the TLS var as a sentinel for whether the thread is attached or not.

It would be nice eventually to whittle down the number of TLS vars that mean "this thread is attached in some way" (ie, MonoThreadInfo, jit_tls, and current domain) but I think that can be done separately from the current cleanup that just removes passing a domain around as an argument.

@CoffeeFlux
Copy link
Contributor Author

I think we'll end up living with MonoDomain for a while. But we should think of it as MonoRootDomain or MonoRoot or something - it's basically a singleton type now.

Agreed, though actually renaming it right now seems painful.

It would be nice eventually to whittle down the number of TLS vars that mean "this thread is attached in some way" (ie, MonoThreadInfo, jit_tls, and current domain) but I think that can be done separately from the current cleanup

I don't really like the thread attach/detach code, so I'll split the actual TLS simplification off into a separate PR but don't want to abandon the effort since I think simplifying that would be valuable. I think this PR can still remove most of the actual domain check/set calls that use the TLS variable.

@vargaz
Copy link
Contributor

vargaz commented Feb 14, 2021

The test failures are relevant.

@vargaz
Copy link
Contributor

vargaz commented Feb 15, 2021

Made a PR with a more piece-by-piece approach in
#48291

@CoffeeFlux
Copy link
Contributor Author

Closing in favor of #48291

@CoffeeFlux CoffeeFlux closed this Mar 1, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Mar 31, 2021
@karelz karelz added this to the 6.0.0 milestone May 20, 2021
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.

4 participants