Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.

Improve CustomAttribute #21832

Merged
merged 6 commits into from
Jan 7, 2019
Merged

Improve CustomAttribute #21832

merged 6 commits into from
Jan 7, 2019

Conversation

benaadams
Copy link
Member

@benaadams benaadams commented Jan 6, 2019

Contributes to dotnet/wpf#94
Contributes to https://github.com/dotnet/corefx/issues/34283#issuecomment-451015598

Originally started as reducing local scopes in AddCustomAttributes (moving declaration and assignment closer to use; to decrease lifetime) (First commit a43f3d2)

Then tidied up the rest of the file while I was there (Second commit 996bed3)

Including making the following structs readonly CustomAttributeNamedArgument, CustomAttributeTypedArgument, CustomAttributeEncodedArgument, CustomAttributeNamedParameter, CustomAttributeCtorParameter, CustomAttributeType,

Final commit

Total bytes of diff: -602 (-0.02% of base)
    diff is an improvement.

Total byte diff includes -13 bytes from reconciling methods
        Base had    3 unique methods,     2765 unique bytes
        Diff had    4 unique methods,     2752 unique bytes

Top file improvements by size (bytes):
        -602 : System.Private.CoreLib.dasm (-0.02% of base)

1 total files with size differences (1 improved, 0 regressed), 0 unchanged.

Top method regessions by size (bytes):
        1665 : System.Private.CoreLib.dasm - CustomAttributeData:.ctor(ref,struct,byref):this (0/1 methods)
        1048 : System.Private.CoreLib.dasm - CustomAttribute:FilterCustomAttributeRecord(struct,byref,ref,struct,ref,bool,byref,byref,byref,byref,byref):bool (0/1 methods)
          20 : System.Private.CoreLib.dasm - CustomAttributeRecord:.ctor(int,struct):this (0/1 methods)
          19 : System.Private.CoreLib.dasm - MetadataImport:GetCustomAttributeRecord(int,byref,byref):this (0/1 methods)
           6 : System.Private.CoreLib.dasm - CustomAttributeTypedArgument:ToString(bool):ref:this

Top method improvements by size (bytes):
       -1663 : System.Private.CoreLib.dasm - CustomAttributeData:.ctor(ref,struct):this (1/0 methods)
       -1083 : System.Private.CoreLib.dasm - CustomAttribute:FilterCustomAttributeRecord(struct,struct,ref,struct,ref,bool,byref,byref,byref,byref,byref):bool (1/0 methods)
        -218 : System.Private.CoreLib.dasm - CustomAttribute:AddCustomAttributes(byref,ref,int,ref,bool,byref)
        -148 : System.Private.CoreLib.dasm - CustomAttributeData:.ctor(ref):this
        -121 : System.Private.CoreLib.dasm - CustomAttribute:IsCustomAttributeDefined(ref,int,ref,int,bool):bool
         -23 : System.Private.CoreLib.dasm - CustomAttributeData:get_ConstructorArguments():ref:this
         -23 : System.Private.CoreLib.dasm - CustomAttribute:GetAttributeUsage(ref):ref
         -19 : System.Private.CoreLib.dasm - CustomAttributeData:GetCustomAttributes(ref,int):ref
         -19 : System.Private.CoreLib.dasm - MetadataImport:GetCustomAttributeProps(int,byref,byref):this (1/0 methods)
         -16 : System.Private.CoreLib.dasm - PseudoCustomAttribute:GetMarshalAsCustomAttribute(int,ref):ref
         -13 : System.Private.CoreLib.dasm - PseudoCustomAttribute:GetDllImportCustomAttribute(ref):ref
         -10 : System.Private.CoreLib.dasm - PseudoCustomAttribute:GetStructLayoutCustomAttribute(ref):ref
          -4 : System.Private.CoreLib.dasm - CustomAttributeNamedArgument:.ctor(ref,ref):this

18 total methods with size differences (13 improved, 5 regressed), 17539 unchanged.

Commit 2

Total bytes of diff: -305 (-0.01% of base)
    diff is an improvement.

Total byte diff includes 0 bytes from reconciling methods
        Base had    0 unique methods,        0 unique bytes
        Diff had    0 unique methods,        0 unique bytes

Top file improvements by size (bytes):
        -305 : System.Private.CoreLib.dasm (-0.01% of base)

1 total files with size differences (1 improved, 0 regressed), 0 unchanged.

Top method regessions by size (bytes):
           6 : System.Private.CoreLib.dasm - CustomAttributeTypedArgument:ToString(bool):ref:this

Top method improvements by size (bytes):
        -148 : System.Private.CoreLib.dasm - CustomAttributeData:.ctor(ref):this
         -97 : System.Private.CoreLib.dasm - CustomAttribute:AddCustomAttributes(byref,ref,int,ref,bool,byref)
         -23 : System.Private.CoreLib.dasm - CustomAttributeData:get_ConstructorArguments():ref:this
         -16 : System.Private.CoreLib.dasm - PseudoCustomAttribute:GetMarshalAsCustomAttribute(int,ref):ref
         -13 : System.Private.CoreLib.dasm - PseudoCustomAttribute:GetDllImportCustomAttribute(ref):ref
         -10 : System.Private.CoreLib.dasm - PseudoCustomAttribute:GetStructLayoutCustomAttribute(ref):ref
          -4 : System.Private.CoreLib.dasm - CustomAttributeNamedArgument:.ctor(ref,ref):this

8 total methods with size differences (7 improved, 1 regressed), 17552 unchanged.

Commit 3

Total bytes of diff: -410 (-0.01% of base)
    diff is an improvement.

Total byte diff includes 68 bytes from reconciling methods
        Base had    3 unique methods,     2765 unique bytes
        Diff had    4 unique methods,     2833 unique bytes

Top file improvements by size (bytes):
        -410 : System.Private.CoreLib.dasm (-0.01% of base)

1 total files with size differences (1 improved, 0 regressed), 0 unchanged.

Top method regessions by size (bytes):
        1663 : System.Private.CoreLib.dasm - CustomAttributeData:.ctor(ref,byref):this (0/1 methods)
        1048 : System.Private.CoreLib.dasm - CustomAttribute:FilterCustomAttributeRecord(struct,byref,ref,struct,ref,bool,byref,byref,byref,byref,byref):bool (0/1 methods)
         116 : System.Private.CoreLib.dasm - CustomAttributeData:GetCustomAttributeRecords(ref,int):ref
         102 : System.Private.CoreLib.dasm - MetadataImport:GetCustomAttributeRecord(int,byref):this (0/1 methods)
          20 : System.Private.CoreLib.dasm - CustomAttributeRecord:.ctor(int,struct):this (0/1 methods)
           6 : System.Private.CoreLib.dasm - CustomAttributeTypedArgument:ToString(bool):ref:this

Top method improvements by size (bytes):
       -1663 : System.Private.CoreLib.dasm - CustomAttributeData:.ctor(ref,struct):this (1/0 methods)
       -1083 : System.Private.CoreLib.dasm - CustomAttribute:FilterCustomAttributeRecord(struct,struct,ref,struct,ref,bool,byref,byref,byref,byref,byref):bool (1/0 methods)
        -218 : System.Private.CoreLib.dasm - CustomAttribute:AddCustomAttributes(byref,ref,int,ref,bool,byref)
        -148 : System.Private.CoreLib.dasm - CustomAttributeData:.ctor(ref):this
        -121 : System.Private.CoreLib.dasm - CustomAttribute:IsCustomAttributeDefined(ref,int,ref,int,bool):bool
         -24 : System.Private.CoreLib.dasm - CustomAttributeData:GetCustomAttributes(ref,int):ref
         -23 : System.Private.CoreLib.dasm - CustomAttributeData:get_ConstructorArguments():ref:this
         -23 : System.Private.CoreLib.dasm - CustomAttribute:GetAttributeUsage(ref):ref
         -19 : System.Private.CoreLib.dasm - MetadataImport:GetCustomAttributeProps(int,byref,byref):this (1/0 methods)
         -16 : System.Private.CoreLib.dasm - PseudoCustomAttribute:GetMarshalAsCustomAttribute(int,ref):ref
         -13 : System.Private.CoreLib.dasm - PseudoCustomAttribute:GetDllImportCustomAttribute(ref):ref
         -10 : System.Private.CoreLib.dasm - PseudoCustomAttribute:GetStructLayoutCustomAttribute(ref):ref
          -4 : System.Private.CoreLib.dasm - CustomAttributeNamedArgument:.ctor(ref,ref):this

19 total methods with size differences (13 improved, 6 regressed), 17538 unchanged.

Commit 4

Total bytes of diff: -602 (-0.02% of base)
    diff is an improvement.

Total byte diff includes -13 bytes from reconciling methods
        Base had    3 unique methods,     2765 unique bytes
        Diff had    4 unique methods,     2752 unique bytes

Top file improvements by size (bytes):
        -602 : System.Private.CoreLib.dasm (-0.02% of base)

1 total files with size differences (1 improved, 0 regressed), 0 unchanged.

Top method regessions by size (bytes):
        1665 : System.Private.CoreLib.dasm - CustomAttributeData:.ctor(ref,struct,byref):this (0/1 methods)
        1048 : System.Private.CoreLib.dasm - CustomAttribute:FilterCustomAttributeRecord(struct,byref,ref,struct,ref,bool,byref,byref,byref,byref,byref):bool (0/1 methods)
          20 : System.Private.CoreLib.dasm - CustomAttributeRecord:.ctor(int,struct):this (0/1 methods)
          19 : System.Private.CoreLib.dasm - MetadataImport:GetCustomAttributeRecord(int,byref,byref):this (0/1 methods)
           6 : System.Private.CoreLib.dasm - CustomAttributeTypedArgument:ToString(bool):ref:this

Top method improvements by size (bytes):
       -1663 : System.Private.CoreLib.dasm - CustomAttributeData:.ctor(ref,struct):this (1/0 methods)
       -1083 : System.Private.CoreLib.dasm - CustomAttribute:FilterCustomAttributeRecord(struct,struct,ref,struct,ref,bool,byref,byref,byref,byref,byref):bool (1/0 methods)
        -218 : System.Private.CoreLib.dasm - CustomAttribute:AddCustomAttributes(byref,ref,int,ref,bool,byref)
        -148 : System.Private.CoreLib.dasm - CustomAttributeData:.ctor(ref):this
        -121 : System.Private.CoreLib.dasm - CustomAttribute:IsCustomAttributeDefined(ref,int,ref,int,bool):bool
         -23 : System.Private.CoreLib.dasm - CustomAttributeData:get_ConstructorArguments():ref:this
         -23 : System.Private.CoreLib.dasm - CustomAttribute:GetAttributeUsage(ref):ref
         -19 : System.Private.CoreLib.dasm - CustomAttributeData:GetCustomAttributes(ref,int):ref
         -19 : System.Private.CoreLib.dasm - MetadataImport:GetCustomAttributeProps(int,byref,byref):this (1/0 methods)
         -16 : System.Private.CoreLib.dasm - PseudoCustomAttribute:GetMarshalAsCustomAttribute(int,ref):ref
         -13 : System.Private.CoreLib.dasm - PseudoCustomAttribute:GetDllImportCustomAttribute(ref):ref
         -10 : System.Private.CoreLib.dasm - PseudoCustomAttribute:GetStructLayoutCustomAttribute(ref):ref
          -4 : System.Private.CoreLib.dasm - CustomAttributeNamedArgument:.ctor(ref,ref):this

18 total methods with size differences (13 improved, 5 regressed), 17539 unchanged.

@benaadams
Copy link
Member Author

@dotnet-bot test OSX10.12 x64 Checked Innerloop Build and Test
@dotnet-bot test Windows_NT arm Cross Debug Innerloop Build

@benaadams
Copy link
Member Author

benaadams commented Jan 6, 2019

Interesting error on Windows_NT arm; happened previously (before triggering a retest which failed same way)

BUILD: Build succeeded.  Finished at 17:15:38.53
BUILD: Product binaries are available at D:\j\workspace\arm_cross_deb---a3e3dfd4\bin\Product\Windows_NT.arm.Debug
[arm_cross_deb---a3e3dfd4] $ cmd /c call C:\Users\runner\AppData\Local\Temp\jenkins1035483821241007740.bat

D:\j\workspace\arm_cross_deb---a3e3dfd4>powershell -NoProfile -Command "Add-Type -Assembly 'System.IO.Compression.FileSystem';
 [System.IO.Compression.ZipFile]::CreateFromDirectory('.\bin\tests\Windows_NT.arm.Debug',
 '.\bin\tests\tests.zip')" 

 Exception calling "CreateFromDirectory" with "2" argument(s):
 "Operation did not complete successfully because the 
 file contains a virus or potentially unwanted software.
"
At line:1 char:56
+ ... ileSystem'; [System.IO.Compression.ZipFile]::CreateFromDirectory('.\b ...
+                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    + CategoryInfo          : NotSpecified: (:) [], MethodInvocationException
    + FullyQualifiedErrorId : IOException
 

D:\j\workspace\arm_cross_deb---a3e3dfd4>exit 1 

@benaadams
Copy link
Member Author

@dotnet-bot test OSX10.12 x64 Checked Innerloop Build and Test
@dotnet-bot test Windows_NT arm Cross Debug Innerloop Build

@benaadams
Copy link
Member Author

Total bytes of diff: -410 (-0.01% of base)
    diff is an improvement.

Total byte diff includes 68 bytes from reconciling methods
        Base had    3 unique methods,     2765 unique bytes
        Diff had    4 unique methods,     2833 unique bytes

Top file improvements by size (bytes):
        -410 : System.Private.CoreLib.dasm (-0.01% of base)

1 total files with size differences (1 improved, 0 regressed), 0 unchanged.

Top method regessions by size (bytes):
        1663 : System.Private.CoreLib.dasm - CustomAttributeData:.ctor(ref,byref):this (0/1 methods)
        1048 : System.Private.CoreLib.dasm - CustomAttribute:FilterCustomAttributeRecord(struct,byref,ref,struct,ref,bool,byref,byref,byref,byref,byref):bool (0/1 methods)
         116 : System.Private.CoreLib.dasm - CustomAttributeData:GetCustomAttributeRecords(ref,int):ref
         102 : System.Private.CoreLib.dasm - MetadataImport:GetCustomAttributeRecord(int,byref):this (0/1 methods)
          20 : System.Private.CoreLib.dasm - CustomAttributeRecord:.ctor(int,struct):this (0/1 methods)
           6 : System.Private.CoreLib.dasm - CustomAttributeTypedArgument:ToString(bool):ref:this

Top method improvements by size (bytes):
       -1663 : System.Private.CoreLib.dasm - CustomAttributeData:.ctor(ref,struct):this (1/0 methods)
       -1083 : System.Private.CoreLib.dasm - CustomAttribute:FilterCustomAttributeRecord(struct,struct,ref,struct,ref,bool,byref,byref,byref,byref,byref):bool (1/0 methods)
        -218 : System.Private.CoreLib.dasm - CustomAttribute:AddCustomAttributes(byref,ref,int,ref,bool,byref)
        -148 : System.Private.CoreLib.dasm - CustomAttributeData:.ctor(ref):this
        -121 : System.Private.CoreLib.dasm - CustomAttribute:IsCustomAttributeDefined(ref,int,ref,int,bool):bool
         -24 : System.Private.CoreLib.dasm - CustomAttributeData:GetCustomAttributes(ref,int):ref
         -23 : System.Private.CoreLib.dasm - CustomAttributeData:get_ConstructorArguments():ref:this
         -23 : System.Private.CoreLib.dasm - CustomAttribute:GetAttributeUsage(ref):ref
         -19 : System.Private.CoreLib.dasm - MetadataImport:GetCustomAttributeProps(int,byref,byref):this (1/0 methods)
         -16 : System.Private.CoreLib.dasm - PseudoCustomAttribute:GetMarshalAsCustomAttribute(int,ref):ref
         -13 : System.Private.CoreLib.dasm - PseudoCustomAttribute:GetDllImportCustomAttribute(ref):ref
         -10 : System.Private.CoreLib.dasm - PseudoCustomAttribute:GetStructLayoutCustomAttribute(ref):ref
          -4 : System.Private.CoreLib.dasm - CustomAttributeNamedArgument:.ctor(ref,ref):this

19 total methods with size differences (13 improved, 6 regressed), 17538 unchanged.

@benaadams
Copy link
Member Author

benaadams commented Jan 7, 2019

CustomAttributeData:GetCustomAttributeRecords regresses from the inline of the larger MetadataImport:GetCustomAttributeRecord(int,byref):this from making CustomAttributeRecord readonly and using its .ctor rather than modifying its properties directly

@jkotas
Copy link
Member

jkotas commented Jan 7, 2019

from making CustomAttributeRecord readonly

To avoid this regression - mark struct CustomAttributeRecord as readonly too; or do not mark the field readonly?

@benaadams
Copy link
Member Author

; or do not mark the field readonly?

Yep, only going 50% readonly ConstArray seems to do the trick (as MetadataToken is a single int field so doesn't benefit being passed as in)

Total bytes of diff: -602 (-0.02% of base)
    diff is an improvement.

Total byte diff includes -13 bytes from reconciling methods
        Base had    3 unique methods,     2765 unique bytes
        Diff had    4 unique methods,     2752 unique bytes

Top file improvements by size (bytes):
        -602 : System.Private.CoreLib.dasm (-0.02% of base)

1 total files with size differences (1 improved, 0 regressed), 0 unchanged.

Top method regessions by size (bytes):
        1665 : System.Private.CoreLib.dasm - CustomAttributeData:.ctor(ref,struct,byref):this (0/1 methods)
        1048 : System.Private.CoreLib.dasm - CustomAttribute:FilterCustomAttributeRecord(struct,byref,ref,struct,ref,bool,byref,byref,byref,byref,byref):bool (0/1 methods)
          20 : System.Private.CoreLib.dasm - CustomAttributeRecord:.ctor(int,struct):this (0/1 methods)
          19 : System.Private.CoreLib.dasm - MetadataImport:GetCustomAttributeRecord(int,byref,byref):this (0/1 methods)
           6 : System.Private.CoreLib.dasm - CustomAttributeTypedArgument:ToString(bool):ref:this

Top method improvements by size (bytes):
       -1663 : System.Private.CoreLib.dasm - CustomAttributeData:.ctor(ref,struct):this (1/0 methods)
       -1083 : System.Private.CoreLib.dasm - CustomAttribute:FilterCustomAttributeRecord(struct,struct,ref,struct,ref,bool,byref,byref,byref,byref,byref):bool (1/0 methods)
        -218 : System.Private.CoreLib.dasm - CustomAttribute:AddCustomAttributes(byref,ref,int,ref,bool,byref)
        -148 : System.Private.CoreLib.dasm - CustomAttributeData:.ctor(ref):this
        -121 : System.Private.CoreLib.dasm - CustomAttribute:IsCustomAttributeDefined(ref,int,ref,int,bool):bool
         -23 : System.Private.CoreLib.dasm - CustomAttributeData:get_ConstructorArguments():ref:this
         -23 : System.Private.CoreLib.dasm - CustomAttribute:GetAttributeUsage(ref):ref
         -19 : System.Private.CoreLib.dasm - CustomAttributeData:GetCustomAttributes(ref,int):ref
         -19 : System.Private.CoreLib.dasm - MetadataImport:GetCustomAttributeProps(int,byref,byref):this (1/0 methods)
         -16 : System.Private.CoreLib.dasm - PseudoCustomAttribute:GetMarshalAsCustomAttribute(int,ref):ref
         -13 : System.Private.CoreLib.dasm - PseudoCustomAttribute:GetDllImportCustomAttribute(ref):ref
         -10 : System.Private.CoreLib.dasm - PseudoCustomAttribute:GetStructLayoutCustomAttribute(ref):ref
          -4 : System.Private.CoreLib.dasm - CustomAttributeNamedArgument:.ctor(ref,ref):this

18 total methods with size differences (13 improved, 5 regressed), 17539 unchanged.

Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

Thanks!

@benaadams
Copy link
Member Author

benaadams commented Jan 7, 2019

For dotnet/wpf#94 a big chunk is in the ModuleHandle.ResolveMethod QCall (this is a pre-changes trace, prior to various improvements to the managed side)

image

Though I don't know if its easy to gain anything there.

@jkotas
Copy link
Member

jkotas commented Jan 7, 2019

cc @steveharter

@jkotas
Copy link
Member

jkotas commented Jan 7, 2019

dotnet/wpf#94

This is a combination of:

  1. WPF enumerates attributes on all loaded assemblies.
  2. The build systems like to generate a lot of assembly attributes into each assembly
  3. .NET Core has a lot of little assemblies.
  4. The custom attribute APIs expose resolved types only.

While improving 2., 3. or 4. may help somewhat, it won't really make the problem go away.

(1) is the root cause of the problem. Similar designs are known to scale poorly and always turn into performance bugs (and sometimes correctness or security bugs too). It is where the fix needs to be to make the problem go away. E.g. it can an option that allows you to specify the explicit list of assemblies/types that the Xaml parser needs to worry about.

@benaadams
Copy link
Member Author

dotnet/wpf#94

(1) is the root cause of the problem.

Ah yes, looking at the startup costs for EventSources in ASP.NET https://github.com/dotnet/corefx/issues/34283#issuecomment-451015598; ModuleHandle.ResolveMethod doesn't particularly feature (on this PR build)

Callers

image

Callees

image

@benaadams
Copy link
Member Author

I think Ubuntu arm Cross testing is busted all recent PRs are in a waiting state for them after very many hours

@benaadams
Copy link
Member Author

(pending—All nodes of label ‘ubuntu.1404.arm32.open’ are offline)

@benaadams
Copy link
Member Author

@dotnet-bot test OSX10.12 x64 Checked Innerloop Build and Test

@jkotas jkotas merged commit e5cb6c3 into dotnet:master Jan 7, 2019
@benaadams benaadams deleted the AddCustomAttributes branch January 7, 2019 19:55
sandreenko pushed a commit to sandreenko/coreclr that referenced this pull request Jan 8, 2019
* Reduce local scopes in AddCustomAttributes

* CustomAttribute.cs tidy up

* Less struct copies
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
* Reduce local scopes in AddCustomAttributes

* CustomAttribute.cs tidy up

* Less struct copies


Commit migrated from dotnet/coreclr@e5cb6c3
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants