-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Fix DiagnosticSource to work with NativeAOT #76109
Merged
eerhardt
merged 7 commits into
dotnet:main
from
eerhardt:FixDiagnosticSourceNativeAot-main
Sep 28, 2022
Merged
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
1227d64
Fix DiagnosticSource to work with NativeAOT
eerhardt ba624bf
Enable new NativeAotTests in CI
eerhardt b633e7f
Fix NativeAOT tests
eerhardt 3b4f5bd
Fix test CI leg
eerhardt ccca518
Set IlcFrameworkNativePath correctly
eerhardt 9aeca47
Don't run NativeAot published app tests on OSX since it isn't supported
eerhardt 15824c8
Respond to PR feedback
eerhardt File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MichalStrehovsky @jkotas - with this change we now have tests ensuring EventSource works (at least in proc) with PublishAot. Do you think in .NET 8 we should re-enable
EventSourceSupport=true
by default? Or do you think we should keep it disabled by default going forward (for size savings?)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would wait for feedback on the current .NET 7 default before doing more changes.
I think it makes sense to leave it off by default and turn it on only for server app-models in their SDKs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with Jan. I kind of like the size savings. I would like to see feedback. Re-enabling it is an instant 30% size regression from 7.0 for hello world.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI - @noahfalk @tommcdon
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious, how big (in bytes) is 30%? I wouldn't have guessed the savings were such a high percentage but I don't know if that is because I overestimated the size of HelloWorld or underestimated the size of EventSource.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we have a log and I'm hoping to do some offcycle work to visualize/diff them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've love to see the results of this. Agreed with @noahfalk that this is more than I would have expected. It feels important to describe the diagnostic expectations for native AOT in .NET 8 so that we can determine how this decision fits in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a similar difference for
PublishTrimmed
as well - the size of managed code goes from 2,718,848 bytes to 3,003,008. So the cost is about 300 kB in IL terms. (The repro steps for both is basically just rundotnet publish -r win-x64 -c Release
with-p:PublishTrimmed=true
for trimming or-p:PublishAot=true
for NativeAOT. Use-p:EventSourceSupport=false
to enable/disable EventSource.)For the NativeAOT case a quick breakdown is following.
Size of code per namespace with EventSource enabled
Size of code per namespace with EventSource disabled
(This is just namespaces with more than 20 kB of code)
Most of the cost is in transitive dependencies. EventSource has a big transitive closure and depends on a lot of framework code.
I can file an issue if there's interest in reducing the transtive closure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This tells me that this is the upper bound of the size savings for having EventSource disabled. In a real world app that uses these transitive dependencies, they won't be able to be trimmed. So the size savings of disabling EventSource will be less. For example, 90 kB of size savings is coming from
System.Collections.Generic
which many apps will use. When the app uses those collections, they won't be able to be trimmed anymore, even with EventSource disabled.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That depends - when thinking about native code, one needs to think about instantiated generic code. For example the costs associated with
System.Collections.Generic.Dictionary
2<System.Type,System.Diagnostics.Tracing.TraceLoggingTypeInfo>` are exclusive to EventSource not matter what the app does.It's easy to test out theories - just pick a bigger app and see the impact of EventSource. For apps the size of current ASP.NET, an extra 600 kB overhead is negligible either way,