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

Remove dependency on AppDomain from a hello world app #95710

Merged
merged 7 commits into from
Dec 8, 2023

Conversation

MichalStrehovsky
Copy link
Member

@MichalStrehovsky MichalStrehovsky commented Dec 6, 2023

Saves 4% in size on a hello world.

A hello world app will bring the AppDomain type with it. AppDomain's ToString is quite expensive for what it does.

Maybe this can be done cleaner, suggestions welcome.

Cc @dotnet/ilc-contrib

Saves 4% in size on a hello world.

A hello world app will bring the `AppDomain` type with it. `AppDomain`'s `ToString` is quite expensive for what it does.

Maybe this can be done cleaner, suggestions welcome.
@ghost ghost assigned MichalStrehovsky Dec 6, 2023
@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Dec 6, 2023
@jkotas
Copy link
Member

jkotas commented Dec 7, 2023

It would be nice for scanner to figure out that s_processExit is never written to and that it is effectively null readonly field; and then optimize the whole event invocation, including AppDomain.CurrentDomain call, based on this information. What are the main problems that would need to be solved to make this possible?

@MichalStrehovsky
Copy link
Member Author

I was digging into a compiler-side fix yesterday. The easy fix I tried was to help RyuJIT get rid of this branch.

  • We currently don't get rid of it because AppContext has a static constructor that cannot be executed at compile time (it allocates a dictionary, for starters).
  • Whole program analysis knows the field is effectively readonly, but it doesn't know the value written by cctor (it doesn't know cctor doesn't write anything either). I've added new analysis for static fields that are never written (not even in the cctor): MichalStrehovsky@b32d899.
  • This helped get rid of AppDomain but the damage done to the whole program view could not be undone (we had virtual slots assigned from whole program view that are now unused because AppDomain.ToString was gone from the output, but still part of the whole program view). This leaves majority of size savings on the table.

As you say, we need to be able to get rid of this in the scanner. It's too late in RyuJIT.

And we cannot get rid of this in the scanner, because the field is not read only. It can be considered read only once we finish whole program analysis.

We'd need to run whole program analysis twice and then do this substitution when scanning the second time. Running whole program analysis twice is an option. It will be a significant compile throughput regression. We could potentially only do it when optimizing for size/speed and skip it in blended mode?

@jkotas
Copy link
Member

jkotas commented Dec 8, 2023

We'd need to run whole program analysis twice and then do this substitution when scanning the second time.

It does not seem to be worth it.

Here is a variant that leverages the fact that the ProcessExit events is exposed via AppDomain instance fields: main...jkotas:runtime:appdomain . It would work better in case we decide to fix the sender on the FirstChanceException and UnhandledException events. Naot is passing in null as the sender, but Mono is passing in the appdomain instance. Either variant is fine with me. Do you have a preference?

@MichalStrehovsky
Copy link
Member Author

Do you have a preference?

Yours looks much better! GitHub won't let me approve my own PR even though it's all your commit, so LGTM!

@jkotas jkotas merged commit b995d33 into dotnet:main Dec 8, 2023
176 of 178 checks passed
@MichalStrehovsky MichalStrehovsky deleted the processexit branch December 9, 2023 05:48
@github-actions github-actions bot locked and limited conversation to collaborators Jan 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants