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

Introduce a way to opt-out of source info in Diagnostics StackFrame #34910

Open
eerhardt opened this issue Apr 13, 2020 · 9 comments
Open

Introduce a way to opt-out of source info in Diagnostics StackFrame #34910

eerhardt opened this issue Apr 13, 2020 · 9 comments
Labels
area-System.Diagnostics feature-request size-reduction Issues impacting final app size primary for size sensitive workloads
Milestone

Comments

@eerhardt
Copy link
Member

The source line info support for System.Diagnostics.StackTrace/StackFrame comes with a noticeable cost for workloads which are size sensitive and don't contain PDB information anyway.

With change #32023, we added a PreserveDependency attribute here:

// This is necessary because linker can't add new assemblies to the closure when recognizing Type.GetType
// so the code below is actually recognized by linker, but fails to resolve the type since the System.Diagnostics.StackTrace
// is not always part of the closure linker works on.
// PreserveDependencyAttribute on the other hand can pull in additional assemblies.
[PreserveDependency("GetSourceLineInfo", "System.Diagnostics.StackTraceSymbols", "System.Diagnostics.StackTrace")]
[PreserveDependency(".ctor()", "System.Diagnostics.StackTraceSymbols", "System.Diagnostics.StackTrace")]
internal void InitializeSourceInfo(int iSkip, bool fNeedFileInfo, Exception? exception)

As noted in the PR, the change added ~200K to the "Console Hello World" app, bringing in the following assemblies, which were not needed before:

image

We should consider adding a feature switch (dotnet/designs#99) or similar functionality to allow app developers to opt-out of this behavior.

cc @vitek-karas @jkotas @stephentoub @marek-safar

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-System.Diagnostics untriaged New issue has not been triaged by the area owner labels Apr 13, 2020
@danmoseley danmoseley added this to To do in Linker optimization work via automation Apr 13, 2020
@danmoseley
Copy link
Member

Added to Linker project which I guess is how we keep track of all such work.

@marek-safar
Copy link
Contributor

I don't think this should be feature switch in the design sense as above but illinker property which controls debugging support.

@krwq krwq added this to the 5.0 milestone May 11, 2020
@krwq krwq removed the untriaged New issue has not been triaged by the area owner label May 11, 2020
@krwq
Copy link
Member

krwq commented May 11, 2020

@eerhardt I'm marking this as 5.0 so it doesn't disappear from the radar, please change milestone to Future if you feel like this is not important for 5.0

@eerhardt eerhardt added the linkable-framework Issues associated with delivering a linker friendly framework label May 18, 2020
@eerhardt
Copy link
Member Author

@marek-safar @vargaz - I just discovered that this code isn't used on Mono's CoreLib. Instead, I see that Mono has a native C reader for PPDBs.

[MethodImplAttribute(MethodImplOptions.InternalCall)]
private static extern bool get_frame_info(int skipFrames, bool needFileInfo,
out MethodBase method, out int ilOffset, out int nativeOffset, out string file, out int line, out int column);

Is this the long-term solution for Mono? I'm wondering how important it is for us to solve the above Diagnostics StackFrame issue in .NET 5. If Blazor isn't going to be affected by it, it might not be critical to solve.

@marek-safar
Copy link
Contributor

I think we'll investigate using managed ppds reader but we might not be able to do it for .NET5 but long term this looks attractive to me.

#32023 is not relevant to size sensitive workloads and will become only once the logic is moved to shared SPC. What is however still relevant to us is the "forced" StackTraceSymbol persistence in S.D.S which I'm not sure why it's still there

@eerhardt
Copy link
Member Author

eerhardt commented Jun 1, 2020

What is however still relevant to us is the "forced" StackTraceSymbol persistence in S.D.S which I'm not sure why it's still there

I'll look into removing it today. Thanks for the pointer.

@eerhardt
Copy link
Member Author

eerhardt commented Jul 9, 2020

This is not necessary for Blazor WASM in 5.0. Moving to 6.0.

@marek-safar marek-safar added size-reduction Issues impacting final app size primary for size sensitive workloads and removed linkable-framework Issues associated with delivering a linker friendly framework labels Dec 9, 2020
@eerhardt eerhardt modified the milestones: 6.0.0, Future May 11, 2021
@eerhardt
Copy link
Member Author

Moving to 'Future' as this isn't high priority for 6.

@jkotas
Copy link
Member

jkotas commented Jun 30, 2023

This should be tagged on StackTraceSupport introduced in #88235 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-System.Diagnostics feature-request size-reduction Issues impacting final app size primary for size sensitive workloads
Projects
Development

No branches or pull requests

7 participants