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

Add NetCoreAppCurrent configurations to every library that targets .NETStandard or .NETCoreApp #54012

Closed
49 tasks done
ViktorHofer opened this issue Jun 10, 2021 · 19 comments · Fixed by #61867
Closed
49 tasks done

Comments

@ViktorHofer
Copy link
Member

ViktorHofer commented Jun 10, 2021

With the evolution of .NET (net5.0 and above), building .NETCoreApp libraries becomes more important as they benefit from features like linking, compiler optimizations and the platform analyzer which is not enabled in dotnet/runtime for .NETStandard and .NETFramework tfms. To go even further, nullability warnings are more mature when targeting the very latest .NETCoreApp configuration and newer APIs become available to target which are often more efficient or easier to read. An example of such an API is the string.Contains usage over string.IndexOf which .NET even warns about now. Last but not least this avoid the need to reference the netstandard shim which means we don't cross assembly universes.

I propose to add a NetCoreAppCurrent configuration to any library that doesn't have one and targets .NETStandard or .NETCoreApp. Please note that this won't increase the overall build times as by default, only the best applicable tfm is built per project in a traversal build. That said, when building an individual library, build times will increase slightly as filtering doesn't happen there.

@marek-safar and @eerhardt especially expressed their interest in this.

Here's the list of the 49 libraries which don't have a NetCoreAppCurrent (and neither any .NETCoreApp) configuration:

cc @ericstj @Anipik @safern @stephentoub @jkotas

@ViktorHofer ViktorHofer added design-discussion Ongoing discussion about design without consensus area-Infrastructure-libraries labels Jun 10, 2021
@ViktorHofer ViktorHofer added this to the 6.0.0 milestone Jun 10, 2021
@ghost
Copy link

ghost commented Jun 10, 2021

Tagging subscribers to this area: @Anipik, @safern, @ViktorHofer
See info in area-owners.md if you want to be subscribed.

Issue Details

With the evolution of .NET (net5.0 and above), building .NETCoreApp libraries becomes more important as they benefit from features like linking, compiler optimizations and the platform analyzer which is not enabled in dotnet/runtime for .NETStandard and .NETFramework tfms. To go even further, nullability warnings are more mature when targeting the very latest .NETCoreApp configuration and newer APIs become available to target which are often more efficient or easier to read. An example of such an API is the string.Contains usage over string.IndexOf which .NET even warns about now. Last but not least this avoid the need to reference the netstandard shim which means we don't cross universes anymore.

I propose to add a NetCoreAppCurrent configuration to any library that doesn't have one and targets .NETStandard or .NETCoreApp. Please note that this won't increase the overall build times as by default, only the best applicable tfm is built per project in a traversal build. That said, when building an individual library, build times will increase slightly as filtering doesn't happen there.

@marek-safar and @eerhardt especially expressed their interest in this.

Here's the list of the 50 (source) libraries which don't have a NetCoreAppCurrent (and neither any .NETCoreApp) configuration:

Microsoft.Extensions.Caching.Abstractions
Microsoft.Extensions.Caching.Memory
Microsoft.Extensions.Configuration
Microsoft.Extensions.Configuration.Abstractions
Microsoft.Extensions.Configuration.Binder
Microsoft.Extensions.Configuration.CommandLine
Microsoft.Extensions.Configuration.EnvironmentVariables
Microsoft.Extensions.Configuration.FileExtensions
Microsoft.Extensions.Configuration.Ini
Microsoft.Extensions.Configuration.Json
Microsoft.Extensions.Configuration.UserSecrets
Microsoft.Extensions.Configuration.Xml
Microsoft.Extensions.DependencyInjection
Microsoft.Extensions.DependencyInjection.Abstractions
Microsoft.Extensions.DependencyInjection.Specification.Tests
Microsoft.Extensions.DependencyModel
Microsoft.Extensions.FileProviders.Abstractions
Microsoft.Extensions.FileProviders.Composite
Microsoft.Extensions.FileProviders.Physical
Microsoft.Extensions.FileSystemGlobbing
Microsoft.Extensions.Hosting
Microsoft.Extensions.Hosting.Abstractions
Microsoft.Extensions.Hosting.Systemd
Microsoft.Extensions.Hosting.WindowsServices
Microsoft.Extensions.Http
Microsoft.Extensions.Logging
Microsoft.Extensions.Logging.Configuration
Microsoft.Extensions.Logging.Debug
Microsoft.Extensions.Logging.EventLog
Microsoft.Extensions.Logging.TraceSource
Microsoft.Extensions.Options
Microsoft.Extensions.Options.ConfigurationExtensions
Microsoft.Extensions.Options.DataAnnotations
Microsoft.Win32.Registry.AccessControl
System.ComponentModel.Composition.Registration
System.Composition.AttributedModel
System.Composition.Convention
System.Composition.Hosting
System.Composition.Runtime
System.Composition.TypedParts
System.Data.OleDb
System.IO.Packaging
System.IO.Ports
System.Memory.Data
System.Net.Http.WinHttpHandler
System.Numerics.Tensors
System.Reflection.Context
System.Resources.Extensions
System.Security.Cryptography.Xml
System.Threading.AccessControl

cc @ericstj @Anipik @safern @stephentoub @jkotas

Author: ViktorHofer
Assignees: -
Labels:

Design Discussion, area-Infrastructure-libraries

Milestone: 6.0.0

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Jun 10, 2021
@ViktorHofer ViktorHofer changed the title Add NetCoreAppCurrent configurations to every library that targets .NETStandard Add NetCoreAppCurrent configurations to every library that targets .NETStandard or .NETCoreApp Jun 10, 2021
@ViktorHofer ViktorHofer removed the untriaged New issue has not been triaged by the area owner label Jun 10, 2021
@jkotas
Copy link
Member

jkotas commented Jun 10, 2021

You may be missing System.IO.Hashing that is recent addition and does not have NetCoreAppCurrent. (It has net6.0 that is not the same thing.)

@jkotas
Copy link
Member

jkotas commented Jun 10, 2021

cc @davidfowl

@ericstj
Copy link
Member

ericstj commented Jun 10, 2021

@ViktorHofer you talk about some of the benefits the .NET team may see by adding these configurations. What is the impact to our customers? What benefits will they see by us doing this, and are there any downsides?

@ViktorHofer
Copy link
Member Author

Some of the advantages that are listed in the top post are observable by customers as well but let me exclusively list the pros and cons for customers.

Pro:

  • Better nullability annotations based on nullability warnings being more mature on NetCoreAppCurrent than on .NETStandard.
  • Being able to use APIs which are only available on the very latest and greatest version of .NETCoreApp
  • NetCoreAppCurrent compiled assemblies should be more efficient than .NETStandard ones as more efficient APIs can be used under the shell and because of compiler optimizations that aren't available on .NETStandard.
  • Something that I discussed with @eerhardt offline and which he believes shouldn't be neglected is the reduced amount of infrastructure maintenance. Community members usually don't know which references they need to add to satisfy a new build configuration and by adding the configurations as part of the repository versioning, that's already handled. Of course this doesn't exclusively apply to community members, we ourselves often struggle with adding new configurations. Ultimately that results in more time for feature and productivity work which should have a positive impact on our customers.
  • Probably hard to measure but by not requiring the netstandard.dll shim, less type forwarding is happening during runtime.

Cons:

  • Bigger packages. For most of the packages listed above, usually an increase of few KB - i.e. Microsoft.Extensions.FileSystemGlobbing.dll is 37KB small. There are of course exceptions when libraries target multiple rids.
  • Not something that is introduced by this change but an overall concern: The .NETStandard configuration in the packages becomes untested as currently there's no suitable tfm in our test matrix that would pick the .NETStandard asset over the .NETCoreApp one. That of course opens the question what the .NETStandard implementation assembly is used for as .NETCoreApp and .NETFramework tfms are available which are preferred during runtime.

With the recent added support for shipping NetCoreAppCurrent assets in packages and being able to validate them in the packaging system together with the defined package support policy that dotnet/runtime now adheres I believe the pros
outweigh the cons.

@eerhardt
Copy link
Member

opens the question what the .NETStandard implementation assembly is used for as .NETCoreApp and .NETFramework tfms are available which are preferred during runtime

Xamarin, Unity, UWP, etc. That list will get smaller over time, but there are other runtimes that aren't core or desktop that get used today.

@ViktorHofer
Copy link
Member Author

cc @buyaa-n for the platform analyzer aspect of this discussion

akoeplinger added a commit to akoeplinger/runtime that referenced this issue Jun 14, 2021
System.ComponentModel.Composition, System.Diagnostics.PerformanceCounter and System.Runtime.Caching.

Helps with dotnet#54012
akoeplinger added a commit that referenced this issue Jun 15, 2021
System.ComponentModel.Composition, System.Diagnostics.PerformanceCounter and System.Runtime.Caching.

Helps with #54012
@ericstj
Copy link
Member

ericstj commented Jun 16, 2021

Bigger packages. For most of the packages listed above, usually an increase of few KB

This should be an easy thing to measure and share (even if only an estimate based on a good heuristic). We could diff that with what was previously shipped (eg: before asset deprecation) to demonstrate the net result. Perhaps we can also extrapolate over time what it would look like given our support policy. One of the arguments against additional configurations in the past was unbounded package growth, now we're saying it's not unbounded, it's only X number of configurations for in-support .NET versions. It'd be nice to say what the total impact is to all impacted packages and back up our statement about overall impact being small.

Not something that is introduced by this change but an overall concern: The .NETStandard configuration in the packages becomes untested

While that characteristic isn't introduced with this change it does become more important as it's easier for folks to create differences between configurations. I can imagine this being important for libraries like System.Text.Encodings.Web, System.Text.Json, and Microsoft.Extensions.* where we're doing active feature development and would care about how those features function on older frameworks (that are still in support). I wonder what it would take to support downlevel test runs for these? I can imagine it's much cheaper now that helix has support for deploying frameworks and we build the tests in a way that they shouldn't depend on a custom shared framework.

I really like this direction and I think it sets us up for an overall better maintainable codebase over future releases.

@ViktorHofer it seems like this and some of your other changes work together to realize an overall vision for our targeting strategy over the lifetime of .NET 6.0 onward. Do you think it makes sense to record this in a vision/design doc?

@ViktorHofer
Copy link
Member Author

ViktorHofer commented Jun 21, 2021

This should be an easy thing to measure and share (even if only an estimate based on a good heuristic). We could diff that with what was previously shipped (eg: before asset deprecation) to demonstrate the net result. Perhaps we can also extrapolate over time what it would look like given our support policy. One of the arguments against additional configurations in the past was unbounded package growth, now we're saying it's not unbounded, it's only X number of configurations for in-support .NET versions. It'd be nice to say what the total impact is to all impacted packages and back up our statement about overall impact being small.

Easy as of that I just spent 4 hours collecting and analyzing the data, sure 😄. For the analysis I respected both the assemblies sizes as well as the documentation file sizes compressed. Please see the table below that provides data of the discussed packages. The size increase in the Microsoft.Extensions.* packages are expected as those didn't benefit from the removal of unsupported assets.

In most cases we are seeing a size increase of < 100KB (i.e. Extensions packages). Packages which contain runtime specific assemblies are usually larger which is why sizes increases are larger for those. On the positive side, mose of those packages benefited very much from the "Remove unsupported frameworks" effort which is why the size diff when comparing with 5.0 is quite small. The biggest outliner is System.Data.OleDb which grow 2-3x (depending on NET 7 vs NET 8). That assemblies is quite big but also its documentation file is quite large.

PackageId CurrentSize ApplicableTFM Size 5.0 Package Size 6.0 New Package Size 7.0 Package Size 8.0 Package Size
Microsoft.Extensions.Caching.Abstractions 39.67 KB 7.91 KB 47.58 KB 55.49 KB 63.39 KB
Microsoft.Extensions.Caching.Memory 48.28 KB 13.81 KB 62.09 KB 75.9 KB 89.7 KB
Microsoft.Extensions.Configuration 44.11 KB 10.24 KB 54.35 KB 64.59 KB 74.83 KB
Microsoft.Extensions.Configuration.Abstractions 36.13 KB 7.12 KB 43.25 KB 50.38 KB 57.5 KB
Microsoft.Extensions.Configuration.Binder 42.21 KB 11.33 KB 53.54 KB 64.87 KB 76.19 KB
Microsoft.Extensions.Configuration.CommandLine 32.15 KB 6.41 KB 38.56 KB 44.96 KB 51.37 KB
Microsoft.Extensions.Configuration.EnvironmentVariables 27.39 KB 4.08 KB 31.47 KB 35.54 KB 39.62 KB
Microsoft.Extensions.Configuration.FileExtensions 36.24 KB 7.68 KB 43.92 KB 51.6 KB 59.28 KB
Microsoft.Extensions.Configuration.Ini 32.47 KB 6.34 KB 38.81 KB 45.15 KB 51.49 KB
Microsoft.Extensions.Configuration.Json 43.13 KB 7.38 KB 50.5 KB 57.88 KB 65.26 KB
Microsoft.Extensions.Configuration.UserSecrets 34.34 KB 6.58 KB 40.92 KB 47.49 KB 54.07 KB
Microsoft.Extensions.Configuration.Xml 41.1 KB 10.37 KB 51.47 KB 61.84 KB 72.21 KB
Microsoft.Extensions.DependencyInjection 119.04 KB 32.38 KB 151.42 KB 183.8 KB 216.19 KB
Microsoft.Extensions.DependencyInjection.Abstractions 76.95 KB 15.29 KB 92.23 KB 107.52 KB 122.8 KB
Microsoft.Extensions.DependencyModel 83.39 KB 30.42 KB 113.81 KB 144.24 KB 174.66 KB
Microsoft.Extensions.FileProviders.Abstractions 31.09 KB 4.98 KB 36.07 KB 41.04 KB 46.02 KB
Microsoft.Extensions.FileProviders.Composite 27.35 KB 3.64 KB 30.99 KB 34.64 KB 38.28 KB
Microsoft.Extensions.FileProviders.Physical 55.55 KB 15.54 KB 71.1 KB 86.64 KB 102.18 KB
Microsoft.Extensions.FileSystemGlobbing 55.43 KB 16.58 KB 72.01 KB 88.58 KB 105.16 KB
Microsoft.Extensions.Hosting 85.88 KB 20.65 KB 106.54 KB 127.19 KB 147.85 KB
Microsoft.Extensions.Hosting.Abstractions 53.79 KB 8.31 KB 62.1 KB 70.41 KB 78.72 KB
Microsoft.Extensions.Hosting.Systemd 22.83 KB 5.69 KB 28.51 KB 34.2 KB 39.89 KB
Microsoft.Extensions.Hosting.WindowsServices 36.93 KB 6.42 KB 43.35 KB 49.76 KB 56.18 KB
Microsoft.Extensions.Http 75.5 KB 25.7 KB 101.19 KB 126.89 KB 152.58 KB
Microsoft.Extensions.Logging 74.54 KB 16.51 KB 91.06 KB 107.57 KB 124.09 KB
Microsoft.Extensions.Logging.Configuration 35.6 KB 7.79 KB 43.4 KB 51.19 KB 58.99 KB
Microsoft.Extensions.Logging.Debug 26.09 KB 3.66 KB 29.75 KB 33.41 KB 37.07 KB
Microsoft.Extensions.Logging.EventLog 33.5 KB 7.01 KB 40.51 KB 47.52 KB 54.54 KB
Microsoft.Extensions.Logging.TraceSource 28.77 KB 4.76 KB 33.53 KB 38.29 KB 43.04 KB
Microsoft.Extensions.Options 96.31 KB 20.98 KB 117.29 KB 138.27 KB 159.24 KB
Microsoft.Extensions.Options.ConfigurationExtensions 31.57 KB 5.78 KB 37.34 KB 43.12 KB 48.9 KB
Microsoft.Extensions.Options.DataAnnotations 34.22 KB 4.68 KB 38.89 KB 43.57 KB 48.25 KB
Microsoft.Win32.Registry.AccessControl 48.68 KB 5.96 KB 207 KB 54.65 KB 60.61 KB 66.57 KB
System.ComponentModel.Composition.Registration 43.79 KB 16.75 KB 71 KB 60.54 KB 77.29 KB 94.04 KB
System.Composition.AttributedModel 28.75 KB 3.59 KB 86 KB 32.34 KB 35.93 KB 39.53 KB
System.Composition.Convention 61.05 KB 18.9 KB 144 KB 79.95 KB 98.85 KB 117.75 KB
System.Composition.Hosting 67.24 KB 21.56 KB 154 KB 88.8 KB 110.37 KB 131.93 KB
System.Composition.Runtime 36.53 KB 7.34 KB 99 KB 43.87 KB 51.21 KB 58.55 KB
System.Composition.TypedParts 67.98 KB 24.01 KB 157 KB 91.99 KB 116 KB 140.01 KB
System.Data.OleDb 379.89 KB 179.01 KB 385 KB 558.91 KB 737.92 KB 916.93 KB
System.IO.Packaging 126.06 KB 52.31 KB 234 KB 178.37 KB 230.67 KB 282.98 KB
System.IO.Ports 197.48 KB 35.27 KB 241 KB 232.75 KB 268.02 KB 303.28 KB
System.Memory.Data 44.34 KB 10.03 KB 54 KB 54.37 KB 64.4 KB 74.43 KB
System.Net.Http.WinHttpHandler 286.55 KB 60.39 KB 396 KB 346.94 KB 407.33 KB 467.73 KB
System.Numerics.Tensors 68.52 KB 19.29 KB 93 KB 87.81 KB 107.1 KB 126.38 KB
System.Reflection.Context 64 KB 32.75 KB 154 KB 96.75 KB 129.5 KB 162.24 KB
System.Resources.Extensions 69.76 KB 24.04 KB 90 KB 93.8 KB 117.84 KB 141.87 KB
System.Threading.AccessControl 104.18 KB 21.71 KB 276 KB 125.89 KB 147.61 KB 169.32 KB

@ViktorHofer
Copy link
Member Author

@ViktorHofer it seems like this and some of your other changes work together to realize an overall vision for our targeting strategy over the lifetime of .NET 6.0 onward. Do you think it makes sense to record this in a vision/design doc?

Probably would make sense but do people still read design docs in 10+ years from now (/sarcasm)? Usually I prefer to put as much detail as possible into commit messages, PRs, issues and docs as those are preserved over time. I already mentioned to couple folks that I would like to present the versioning and packaging changes in a libraries teams specific talk. If there's still a need for a design document after that or if you explicitly want it to be written, I'm happy to do so.

@ericstj
Copy link
Member

ericstj commented Jun 22, 2021

After 8.0 we assume the sizes are steady-state, right? Did any of the size differences surprise you or suggest that we should do things to reduce the size (like move to runtime checks from cross compilation)?

Repo docs might serve the same purpose as design. Just hoping we can record what we do and why, and hopefully get a comprehensive view of everything that might be related so that we can be sure to deliver a complete story. Definitely the talk (and associated resources) could help.

@ViktorHofer
Copy link
Member Author

After 8.0 we assume the sizes are steady-state, right?

Correct. Every even major version, starting with 8.0, will carry along three NETCoreApp assets (if the project doesn't target rids and also exkluding ref assemblies) and every odd number two (LTS + Current).

The one that stands out is OleDb but not because of it targeting rids but because of the overall assembly and public api (xml) size.

We already know that there are a few OOBs that target many rids and are therefore quite large (i.e. Odbc) but those are already in main and aren't part of this specific discussion.

@ViktorHofer
Copy link
Member Author

The majority of libraries now have a net6.0 configuration and the remaining ones will get one in the main branch. Moving to 7.0.

@ViktorHofer
Copy link
Member Author

@maryamariyan @eerhardt I won't be around the next months but it would be great if we could stay on top of this and #43605 which in combination will enable nullability annotations and targeting the latest .NETCoreApp version for all the extensions projects.

@eerhardt eerhardt self-assigned this Nov 19, 2021
eerhardt added a commit to eerhardt/runtime that referenced this issue Nov 20, 2021
This makes all Extensions projects consistent in which TFMs they target. This way we don't need to add new TFMs during development of a new feature.

Fix dotnet#54012
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Nov 20, 2021
eerhardt added a commit that referenced this issue Dec 1, 2021
#61867)

* Add NetCoreAppCurrent configurations to Microsoft.Extensions libraries

This makes all Extensions projects consistent in which TFMs they target. This way we don't need to add new TFMs during development of a new feature.

Fix #54012
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Dec 1, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jan 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants