Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Remove dependency on AppDomain from a hello world app#95710

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
jkotas merged 7 commits intodotnet:mainfromMichalStrehovsky:processexit
Dec 8, 2023

Conversation

@MichalStrehovsky
Copy link
Member

@MichalStrehovskyMichalStrehovsky commentedDec 6, 2023
edited
Loading

Saves 4% in size on a hello world.

A hello world app will bring theAppDomain type with it.AppDomain'sToString is quite expensive for what it does.

Maybe this can be done cleaner, suggestions welcome.

Cc @dotnet/ilc-contrib

GerardSmit, huoyaoyuan, jkoritzinsky, danmoseley, and PaulusParssinen reacted with rocket emoji
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.
@ghostghost added the needs-area-labelAn area label is needed to ensure this gets routed to the appropriate area owners labelDec 6, 2023
@jkotas
Copy link
Member

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

@MichalStrehovsky
Copy link
MemberAuthor

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 becauseAppContext 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 ofAppDomain 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 becauseAppDomain.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

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 thesender 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
MemberAuthor

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 reacted with thumbs up emoji

@jkotasjkotas mentioned this pull requestDec 8, 2023
@jkotasjkotas merged commitb995d33 intodotnet:mainDec 8, 2023
@MichalStrehovskyMichalStrehovsky deleted the processexit branchDecember 9, 2023 05:48
@github-actionsgithub-actionsbot locked and limited conversation to collaboratorsJan 8, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.

Reviewers

@jkotasjkotasjkotas approved these changes

Labels

needs-area-labelAn area label is needed to ensure this gets routed to the appropriate area owners

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@MichalStrehovsky@jkotas

[8]ページ先頭

©2009-2025 Movatter.jp