Skip to content

Conversation

kg
Copy link
Member

@kg kg commented Jun 8, 2024

Copy link
Contributor

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

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, I'd use mono_get_root_domain more than mono_domain_get. it's cheaper and doesn't invite questions of "does this function care about being called on an unattached thread?"

@kg kg marked this pull request as ready for review June 10, 2024 21:54
@kg kg requested a review from marek-safar as a code owner June 10, 2024 21:54
@kg
Copy link
Member Author

kg commented Jun 11, 2024

Does anyone know why https://github.com/dotnet/runtime/pull/103186/files#diff-1492a44134220d0f70af248aa7524cfc5e03795d922b90381eaa3b2fea5955deR96 would be failing? How could we have another domain?

@lambdageek
Copy link
Member

lambdageek commented Jun 11, 2024

Does anyone know why https://github.com/dotnet/runtime/pull/103186/files#diff-1492a44134220d0f70af248aa7524cfc5e03795d922b90381eaa3b2fea5955deR96 would be failing? How could we have another domain?

It's called from here

mono_profiler_set_domain_loaded_callback (prof, appdomain_load);

But the mono_root_domain global is assigned after the profiler event is raised:

domain = create_root_domain ();
mono_root_domain = domain;

MONO_PROFILER_RAISE (domain_loaded, (domain));

So when mono_de_domain_add executes, mono_get_root_domain is still returning null

@kg
Copy link
Member Author

kg commented Jun 11, 2024

Does anyone know why https://github.com/dotnet/runtime/pull/103186/files#diff-1492a44134220d0f70af248aa7524cfc5e03795d922b90381eaa3b2fea5955deR96 would be failing? How could we have another domain?

It's called from here

mono_profiler_set_domain_loaded_callback (prof, appdomain_load);

But the mono_root_domain global is assigned after the profiler event is raised:

domain = create_root_domain ();
mono_root_domain = domain;

MONO_PROFILER_RAISE (domain_loaded, (domain));

So when mono_de_domain_add executes, mono_get_root_domain is still returning null

Thanks! Do you think we should assign the global before firing the profiler event? That seems right to me

@lambdageek
Copy link
Member

Do you think we should assign the global before firing the profiler event? That seems right to me

I think so too

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.

3 participants