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

Expose stack trace metadata stripping as a supported option #88235

Merged
merged 2 commits into from Jul 3, 2023

Conversation

MichalStrehovsky
Copy link
Member

Cc @dotnet/ilc-contrib

@ghost
Copy link

ghost commented Jun 30, 2023

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

Issue Details

Cc @dotnet/ilc-contrib

Author: MichalStrehovsky
Assignees: -
Labels:

area-NativeAOT-coreclr

Milestone: -

Copy link
Contributor

@Sergio0694 Sergio0694 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a thought — should we consider a different name than "StackTraceSupport" to make it easier to understand? I'm thinking that this name might make it look like you can "disable" stack traces entirely, which is not the case (since you still get them, just in a less nice way) 🤔

What about using a name similar to what .NET Native uses, but just changing it to use an affirmative form instead? That is: "EnableStackTraceMetadata". This would make it clearer that turning this off just strips additional stack trace metadata, but it's not like you can't use or see stack traces at all anymore 🙂

@@ -32,10 +32,6 @@ Native AOT supports enabling and disabling all [documented framework library fea

Since `PublishTrimmed` is implied to be true with Native AOT, some framework features such as binary serialization are disabled by default.
Copy link
Member

@jkotas jkotas Jun 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume that you are going to move this to https://learn.microsoft.com/en-us/dotnet/core/deploying/native-aot/optimizing . Should we include a link to the official doc here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll delete this doc in a separate PR. I think we just need to pull out the part of the doc that talks about HW intrinsics and delete the rest. Everything else is redundant with the official docs.

@@ -32,10 +32,6 @@ Native AOT supports enabling and disabling all [documented framework library fea

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have noticed that the "By default, the compiler tries to maximize compatibility ..." paragraph above is outdated. You can delete it while you are on it.

@@ -34,7 +34,6 @@ The .NET Foundation licenses this file to you under the MIT license.

<!-- Set up the defaults for the compatibility mode -->
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do not have a thing called compatibility mode anymore. Delete this PropertyGround and change the IlcScanReflection condition polarity to imply the default?

@jkotas
Copy link
Member

jkotas commented Jun 30, 2023

I'm thinking that this name might make it look like you can "disable" stack traces entirely, which is not the case

We have considered naming it TextualStackTraceSupport that would address your concern as well. It is a trade-off between a simple name and a precise name. We tend to prefer simple names. For example, DebuggerSupport is a property with similar issue. <DebuggerSupport>false</DebuggerSupport> does not disable debugging entirely. It just makes debugging a lot less pleasant.

@ivanpovazan
Copy link
Member

A slightly off-topic question:

If I understand the OptimizationPreference=Size option correctly, it is targeted towards JIT configuration with a size preference for code generation.
However, I was wondering whether it was considered to have a single flag or project property which would produce the "smallest" possible output where flags/configurations like StackTraceSupport would be automatically included. Or it is up to the user to fine tune the compiler to get the best out of it?

If I am not mistaken default values for feature switches are somewhat configured in that fashion (depending on the target platform and the application type).

@jkotas
Copy link
Member

jkotas commented Jun 30, 2023

If I am not mistaken default values for feature switches are somewhat configured in that fashion (depending on the target platform and the application type).

Yes, we have opinionated defaults based on the app type. They should produce smallest size that is reasonable for given app type, balancing binary size, functionality, performance and diagnosability. It is up to the user to tune these defaults if they are not happy with them.

@am11
Copy link
Member

am11 commented Jul 1, 2023

$ ilc --help
...
  -O, --optimize                                              Enable optimizations
  --optimize-space, --Os                                      Enable optimizations, favor code space
  --optimize-time, --Ot                                       Enable optimizations, favor code speed

-O maps to "blended mode" or JIT's BLENDED_CODE flag i.e. best of both worlds.

@MichalStrehovsky
Copy link
Member Author

Just a thought — should we consider a different name than "StackTraceSupport" to make it easier to understand? I'm thinking that this name might make it look like you can "disable" stack traces entirely, which is not the case (since you still get them, just in a less nice way)

The idea is that we'd use the same feature switch for #34910 which also does something to stack traces, but different (affects IL-based trimming only whereas this one affects native-based trimming only). The docs will be vague enough that people will understand something will happen to stack traces, but what exactly is implementation-defined. We need to strike the right balance between having a billion options vs having a short enough list that people would be willing to read through it. The distinction is likely not meaningful.

However, I was wondering whether it was considered to have a single flag or project property which would produce the "smallest" possible output where flags/configurations like StackTraceSupport would be automatically included.

OptimizationPreference means "make things smaller, but don't affect functionality, only perf". The other switches (like StackTraceSupport) affect functionality. I don't know if it makes sense to have an option that makes so many functional changes. People will not know what is changing and once we accumulate enough behavior-changing-opt-ins, it is likely that "turn off everything" will not be an acceptable mix of settings for anyone because everyone will run into a setting that breaks them (and the setting will be different for everyone).

MichalStrehovsky added a commit to dotnet/docs that referenced this pull request Jul 3, 2023
@MichalStrehovsky MichalStrehovsky merged commit f25bc7b into dotnet:main Jul 3, 2023
117 checks passed
@MichalStrehovsky MichalStrehovsky deleted the stacktracemd branch July 3, 2023 04:08
@MichalStrehovsky
Copy link
Member Author

Doc change at dotnet/docs#36080.

@dotnet dotnet locked as resolved and limited conversation to collaborators Aug 2, 2023
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.

None yet

5 participants