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

[WIP] use built-in .NET runtime types instead of vendored types when possible #6726

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

lucaspimentel
Copy link
Member

@lucaspimentel lucaspimentel commented Feb 27, 2025

Summary of changes

When targeting .NET Core >= 3.1, do not include the files copied ("vendored") from the .NET runtime. Instead, use the built-in types.

Reason for change

  • main goal is smaller artifacts for .NET Core 3.1 and .NET 6.0 (important for Serverless)
  • secondary goal is consistency in our code base: we currently have different ways of choosing built-in types vs vendored types: sometimes we always use the vendored type, sometimes we choose based on target runtime (TFM)

Implementation details

  • Remove most references to these namespaces from individual files.
  • In GlobalUsings.cs, use #if NETCOREAPP3_1_OR_GREATER and global using to choose namespaces based on target runtime (TFM)
  • Replace the use of some internal .NET runtime APIs with their public counterparts, for example:
    • ArrayMemoryPool ➡️ MemoryPool
    • new ImmutableArray<LocalScope>.Builder() ➡️ ImmutableArray.CreateBuilder<LocalScope>()
  • When targeting netcoreapp3.1 or net6.0, exclude most of the files in:
Vendors/System.Collections.Immutable;
Vendors/System.Memory;
Vendors/System.Private.CoreLib;
Vendors/System.Runtime.CompilerServices.Unsafe;

Note that this PR keeps the files in Vendors/System.Reflection.Metadata for now because we access non-public members from that namespace. This namespace makes up about 60% of the bytes in ``Vendors/System.*`. We can tackle this in a future PR.

This PR also keeps a few files which are used elsewhere:

Vendors/System.Private.CoreLib/src/System/Runtime/InteropServices/LibraryImportAttribute.cs
Vendors/System.Private.CoreLib/src/System/Diagnostics/CodeAnalysis/7NullableAttributes.cs
Vendors/System.Private.CoreLib/src/System/Runtime/InteropServices/StringMarshalling.cs
Vendors/System.Private.CoreLib/SR.cs

Test coverage

No behavior changes expected. All tests should still pass.

Other details

Assembly size comparison

Runtime Before After Diff %
netcoreapp3.1 7.59 MB 7.25 MB -347 KB -4.6%
net6.0 7.62 MB 7.28 MB -346 KB -4.5%

Estimated size per namespace

Before (net6.0)

VendoredMicrosoftCode 0.65 MB (12.2%)
└── System 0.65 MB (12.2%)
    ├── Reflection 0.4 MB (7.6%)
    ├── Collections 0.12 MB (2.3%)
    ├── Buffers 0.06 MB (1.1%)
    ├── Runtime < 0.01 MB (0.1%)
    ├── Linq < 0.01 MB (< 0.1%)
    ├── Diagnostics < 0.01 MB (< 0.1%)
    └── Memory < 0.01 MB (< 0.1%)

After (net6.0)

VendoredMicrosoftCode 0.42 MB (8.2%)
└── System 0.42 MB (8.2%)
    ├── Reflection 0.4 MB (7.9%)
    └── Runtime < 0.01 MB (< 0.1%)

@lucaspimentel lucaspimentel force-pushed the lpimentel/use-built-in-types-2-global-usings branch from 1f37cc8 to 6dd35fd Compare February 27, 2025 16:01
@datadog-datadog-prod-us1
Copy link

datadog-datadog-prod-us1 bot commented Feb 27, 2025

Datadog Report

All test runs 0b047b0 🔗

2 Total Test Services: 0 Failed, 2 Passed

Test Services
Service Name Failed Known Flaky New Flaky Passed Skipped Total Time Test Service View
dd-trace-dotnet 0 0 0 566125 4562 34h 54m 19.52s Link
exploration_tests 0 0 0 22085 3 1h 28m 49.47s Link

@andrewlock
Copy link
Member

andrewlock commented Feb 27, 2025

Execution-Time Benchmarks Report ⏱️

Execution-time results for samples comparing the following branches/commits:

Execution-time benchmarks measure the whole time it takes to execute a program. And are intended to measure the one-off costs. Cases where the execution time results for the PR are worse than latest master results are shown in red. The following thresholds were used for comparing the execution times:

  • Welch test with statistical test for significance of 5%
  • Only results indicating a difference greater than 5% and 5 ms are considered.

Note that these results are based on a single point-in-time result for each branch. For full results, see the dashboard.

Graphs show the p99 interval based on the mean and StdDev of the test run, as well as the mean value of the run (shown as a diamond below the graph).

gantt
    title Execution time (ms) FakeDbCommand (.NET Framework 4.6.2) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6726) - mean (69ms)  : 66, 72
     .   : milestone, 69,
    master - mean (70ms)  : 65, 74
     .   : milestone, 70,

    section CallTarget+Inlining+NGEN
    This PR (6726) - mean (1,002ms)  : 979, 1025
     .   : milestone, 1002,
    master - mean (1,008ms)  : 978, 1037
     .   : milestone, 1008,

Loading
gantt
    title Execution time (ms) FakeDbCommand (.NET Core 3.1) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6726) - mean (103ms)  : 100, 105
     .   : milestone, 103,
    master - mean (103ms)  : 101, 106
     .   : milestone, 103,

    section CallTarget+Inlining+NGEN
    This PR (6726) - mean (675ms)  : 658, 692
     .   : milestone, 675,
    master - mean (691ms)  : 675, 707
     .   : milestone, 691,

Loading
gantt
    title Execution time (ms) FakeDbCommand (.NET 6) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6726) - mean (89ms)  : 88, 91
     .   : milestone, 89,
    master - mean (89ms)  : 88, 91
     .   : milestone, 89,

    section CallTarget+Inlining+NGEN
    This PR (6726) - mean (633ms)  : 620, 646
     .   : milestone, 633,
    master - mean (648ms)  : 634, 663
     .   : milestone, 648,

Loading
gantt
    title Execution time (ms) HttpMessageHandler (.NET Framework 4.6.2) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6726) - mean (191ms)  : 187, 196
     .   : milestone, 191,
    master - mean (191ms)  : 186, 195
     .   : milestone, 191,

    section CallTarget+Inlining+NGEN
    This PR (6726) - mean (1,108ms)  : 1078, 1139
     .   : milestone, 1108,
    master - mean (1,107ms)  : 1081, 1132
     .   : milestone, 1107,

Loading
gantt
    title Execution time (ms) HttpMessageHandler (.NET Core 3.1) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6726) - mean (271ms)  : 266, 276
     .   : milestone, 271,
    master - mean (271ms)  : 267, 276
     .   : milestone, 271,

    section CallTarget+Inlining+NGEN
    This PR (6726) - mean (859ms)  : 833, 885
     .   : milestone, 859,
    master - mean (875ms)  : 843, 907
     .   : milestone, 875,

Loading
gantt
    title Execution time (ms) HttpMessageHandler (.NET 6) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6726) - mean (262ms)  : 259, 265
     .   : milestone, 262,
    master - mean (263ms)  : 259, 267
     .   : milestone, 263,

    section CallTarget+Inlining+NGEN
    This PR (6726) - mean (846ms)  : 809, 883
     .   : milestone, 846,
    master - mean (854ms)  : 826, 882
     .   : milestone, 854,

Loading

@lucaspimentel lucaspimentel force-pushed the lpimentel/use-built-in-types-2-global-usings branch 2 times, most recently from 57f246c to d09bc2e Compare March 7, 2025 16:15
@lucaspimentel lucaspimentel force-pushed the lpimentel/use-built-in-types-2-global-usings branch from d09bc2e to 0b047b0 Compare March 7, 2025 19:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants