Add always trimmed attributes list for browser config #39000

marek-safar · 2020-07-09T16:14:44Z

Example of size reduction on SPC with the most minimalistic Hello
World console sample.

System.Private.CoreLib	Size (kB)	Delta
Trimmed Before	1358 kB
Trimmed After	1271 kB	-87 kB

Trimming the CarChecker app with this change results in

CarChecker	Size	Delta
Trimmed Before	3,584,512 bytes
Trimmed After	3,474,432 bytes	-108 kB
Trimmed After with removing CompilerGenerated	3,453,952 bytes	-128 kB

@eerhardt @vitek-karas

Example of size reduction on SPC with the most minimalistic Hello World sample. | System.Private.CoreLib | Size (kB) | Delta | |-|-|-| | Original | 1358 kB | - | | Trimmed | 1271 kB | - 87 kB |

Dotnet-GitSync-Bot · 2020-07-09T16:14:55Z

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

src/mono/netcore/System.Private.CoreLib/System.Private.CoreLib.csproj

src/mono/netcore/System.Private.CoreLib/src/ILLink/ILLink.LinkAttributes.wasm.xml

stephentoub · 2020-07-09T17:02:20Z

src/mono/netcore/System.Private.CoreLib/src/ILLink/ILLink.LinkAttributes.wasm.xml

+    </type>
+    <type fullname="System.Runtime.CompilerServices.CompilerGeneratedAttribute">
+      <attribute internal="RemoveAttributeInstances" />
+    </type>


CompilerGenerated is checked by StackTrace.ToString, so this will affect Exception.StackTrace/ToString(), for example.

Hmm, that's ugly. Looking at the code it looks like CompilerGeneratedAttribute usage there is optimization like check.

We could remove the check or if it's really needed we could remove this attribute under Debug.IsEnabled condition only.

Should we remove this one and add it with the rest of the Debug-only attributes under the Debugger.IsSupported feature switch?

Examples of the Debug-only attributes that can be removed are:

DebuggerTypeProxyAttribute

DebuggerDisplayAttribute

Looking at the code it looks like CompilerGeneratedAttribute usage there is optimization like check.

I think it's more there in case the types in question were implemented by someone manually; it's trying to only remove the relevant frames when they're on compiler generated types for async and iterators. It may be over protective, but I'm hesitant to remove it for the purposes of this PR.

Should we remove this one and add it with the rest of the Debug-only attributes under the Debugger.IsSupported feature switch?

That sounds reasonable.

Changed it to be conditional

just to make sure I understand: in cases where debugger is disabled the StackTrace would now include the compiler-generated frames?

Correct: the code looking for the attribute wouldn't find it, and so wouldn't skip the frames.

src/mono/netcore/System.Private.CoreLib/src/ILLink/ILLink.LinkAttributes.wasm.xml

eng/illink.targets

Add ExcludeFromCodeCoverageAttribute.

Add note about Obsolete. Only use assembly fullname="*" for compiler generated attributes.

jkotas · 2020-07-10T13:14:03Z

src/mono/netcore/System.Private.CoreLib/src/ILLink/ILLink.LinkAttributes.wasm.xml

+    </type>
+    <type fullname="System.Runtime.CompilerServices.CompilerGeneratedAttribute" feature="System.Diagnostics.Debugger.IsSupported" featurevalue="false">
+      <!--
+        The attribute is used during pretty stack trace printing


StackTraceHidden is used for the same purpose. You can add it with the same condition.

Yep, there is a bunch of others which should be added as conditional (e.g. whole ETW). I'd prefer to add them in follow up PRs

eerhardt · 2020-07-10T14:12:07Z

Setting No Merge on this PR because as soon as we do, it will break publishing any Blazor application until dotnet/linker#1341 is fixed.

eerhardt · 2020-07-10T21:57:48Z

Removing the No Merge label now that the fix for dotnet/linker#1341 has been merged into dotnet/sdk.

eerhardt

I think this is ready to be merged. Any other feedback?

ghost · 2020-07-13T16:09:23Z

Hello @eerhardt!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

Add always trimmed attributes list for browser config

c294b47

Example of size reduction on SPC with the most minimalistic Hello World sample. | System.Private.CoreLib | Size (kB) | Delta | |-|-|-| | Original | 1358 kB | - | | Trimmed | 1271 kB | - 87 kB |

marek-safar requested review from EgorBo, akoeplinger, steveisok and vargaz as code owners July 9, 2020 16:14

marek-safar added arch-wasm WebAssembly architecture area-CoreLib-mono labels Jul 9, 2020

eerhardt reviewed Jul 9, 2020

View reviewed changes

src/mono/netcore/System.Private.CoreLib/System.Private.CoreLib.csproj Outdated Show resolved Hide resolved

jkotas reviewed Jul 9, 2020

View reviewed changes

src/mono/netcore/System.Private.CoreLib/src/ILLink/ILLink.LinkAttributes.wasm.xml Outdated Show resolved Hide resolved

jkotas reviewed Jul 9, 2020

View reviewed changes

src/mono/netcore/System.Private.CoreLib/src/ILLink/ILLink.LinkAttributes.wasm.xml Outdated Show resolved Hide resolved

stephentoub reviewed Jul 9, 2020

View reviewed changes

src/mono/netcore/System.Private.CoreLib/src/ILLink/ILLink.LinkAttributes.wasm.xml Outdated Show resolved Hide resolved

Review feedback

08d790a

stephentoub reviewed Jul 9, 2020

View reviewed changes

src/mono/netcore/System.Private.CoreLib/src/ILLink/ILLink.LinkAttributes.wasm.xml Show resolved Hide resolved

vitek-karas reviewed Jul 9, 2020

View reviewed changes

src/mono/netcore/System.Private.CoreLib/src/ILLink/ILLink.LinkAttributes.wasm.xml Show resolved Hide resolved

vitek-karas reviewed Jul 9, 2020

View reviewed changes

src/mono/netcore/System.Private.CoreLib/src/ILLink/ILLink.LinkAttributes.wasm.xml Outdated Show resolved Hide resolved

Combine and embed the ILLink.LinkAttributes.xml file into the assembly.

ffe0b6d

eerhardt mentioned this pull request Jul 9, 2020

Unnecessary warnings when using ILLink.LinkAttributes.xml to remove attributes dotnet/linker#1338

Closed

eerhardt reviewed Jul 9, 2020

View reviewed changes

eng/illink.targets Outdated Show resolved Hide resolved

Update linker command line to ignore attributes file.

bb840d2

Add ExcludeFromCodeCoverageAttribute.

eerhardt mentioned this pull request Jul 9, 2020

Removing attributes breaks when an assembly is set to "copy" dotnet/linker#1341

Closed

eerhardt assigned marek-safar and eerhardt Jul 9, 2020

eerhardt and others added 2 commits July 9, 2020 17:43

PR feedback.

64f2a45

Add note about Obsolete. Only use assembly fullname="*" for compiler generated attributes.

Update ILLink.LinkAttributes.wasm.xml

fc69953

jkotas reviewed Jul 10, 2020

View reviewed changes

eerhardt added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Jul 10, 2020

eerhardt added the blocked Issue/PR is blocked on something - see comments label Jul 10, 2020

jaredpar mentioned this pull request Jul 10, 2020

OSX machines are de-provisioned during CI / PR runs leading to failures #34472

Closed

eerhardt mentioned this pull request Jul 10, 2020

Attribute removal should not kick in for assemblies which are not linked dotnet/linker#1344

Merged

eerhardt removed NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) blocked Issue/PR is blocked on something - see comments labels Jul 10, 2020

eerhardt approved these changes Jul 10, 2020

View reviewed changes

eerhardt mentioned this pull request Jul 10, 2020

Preserve DebuggerTypeProxyAttribute classes #39126

Merged

eerhardt added the auto-merge label Jul 13, 2020

ghost merged commit c21a387 into dotnet:master Jul 13, 2020

eerhardt mentioned this pull request Jul 29, 2020

Fix ILLink warnings for removed attributes when Debugger.IsSupported is false #40048

Merged

eerhardt mentioned this pull request Aug 14, 2020

Add NativeIntegerAttribute #38683

Closed

ghost locked as resolved and limited conversation to collaborators Dec 8, 2020

This pull request was closed.

Add always trimmed attributes list for browser config #39000

Add always trimmed attributes list for browser config #39000

Uh oh!

Conversation

marek-safar commented Jul 9, 2020 • edited by eerhardt Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Dotnet-GitSync-Bot commented Jul 9, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephentoub Jul 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eerhardt commented Jul 10, 2020

Uh oh!

eerhardt commented Jul 10, 2020

Uh oh!

eerhardt left a comment

Choose a reason for hiding this comment

Uh oh!

ghost commented Jul 13, 2020

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

marek-safar commented Jul 9, 2020 •

edited by eerhardt

Loading

stephentoub Jul 10, 2020 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.