Replace OPTIMIZE_FOR_SIZE with feature switch #111743

MichalStrehovsky · 2025-01-23T10:15:55Z

When I was looking at this last time, I noticed the virtual method overrides and though it's not solvable with feature switches. But looks like the virtual methods are not actually called outside !OPTIMIZE_FOR_SIZE code so we might still be able to trim things correctly if it's a feature switch (we can remove unused virtual methods).

The only problem is maintainability - how do we remember that we should only call the virtuals under a !IsSizeOptimized check. The ifdef was a nice forcing function enforced by the compiler.

dotnet-policy-service · 2025-01-23T10:16:32Z

Tagging subscribers to this area: @dotnet/area-system-linq
See info in area-owners.md if you want to be subscribed.

MichalStrehovsky · 2025-01-23T12:10:54Z

Oh nice:

Size statistics

Pull request #111743

Project	Size before	Size after	Difference
avalonia.app-windows	19096064	18085888	-1010176
hello-minimal-windows	857600	857600	0
hello-windows	1102848	1102848	0
kestrel-minimal-windows	4898816	4804096	-94720
reflection-windows	1748992	1748992	0
webapiaot-windows	9154048	8944640	-209408
winrt-component-full-windows	5728768	5728256	-512
winrt-component-minimal-windows	1760768	1760768	0

stephentoub · 2025-01-23T14:50:57Z

I would love it if we could remove the compilation constant, have a single build, and just achieve desired size reduction in certain environments via the feature switch.

MichalStrehovsky · 2025-01-23T21:47:14Z

I would love it if we could remove the compilation constant, have a single build, and just achieve desired size reduction in certain environments via the feature switch.

Do you have any opinion on how to achieve the "The only problem is maintainability - how do we remember that we should only call the virtuals under a !IsSizeOptimized check." part? I was thinking about leaving the .SpeedOpt.cs files around to keep the virtuals separated at least in files. It won't help much, but it's at least something.

MichalStrehovsky · 2025-01-23T22:05:22Z

Do you have any opinion on how to achieve the "The only problem is maintainability - how do we remember that we should only call the virtuals under a !IsSizeOptimized check."

Hmm, I guess this is the spot where we could use the generalized feature guards. @sbomer - did we implement that part? (I'm not able to find it right now, so my guess is no, but wanted to check.)

sbomer · 2025-01-23T23:50:17Z

@sbomer - did we implement that part? (I'm not able to find it right now, so my guess is no, but wanted to check.)

Nope, only for RequiresUnreferencedCode/RequiresDynamicCode/RequiresAssemblyFiles.

MichalStrehovsky · 2025-01-24T07:54:29Z

/azp run runtime-nativeaot-outerloop

azure-pipelines · 2025-01-24T07:54:38Z

Azure Pipelines successfully started running 1 pipeline(s).

MichalStrehovsky · 2025-01-24T13:30:07Z

I marked this ready for review. Not all tests pass. It turns out we cannot fully replace this with a feature switch.

This part is the problem:

runtime/src/libraries/System.Linq/src/System/Linq/Range.SpeedOpt.cs

Line 10 in 3fef04f

private sealed partial class RangeIterator : IList<int>, IReadOnlyList<int>

We cannot express "this class only implements the interface if a feature switch is defined". And this is a popular interface.

I'll try to measure how much that matters in a separate PR (#111794). The extra interface will only really matter for AOT.

We can adjusts the test to expect this to pass, that's the least problem.

@ivanpovazan is it easy enough to measure the size impact of this on iOS? This is replacing the special build of System.Linq with a common assembly that is same across platforms. There's now a feature switch to enable size savings.

To enable the feature switch, one currently needs to add:

  <ItemGroup>
    <RuntimeHostConfigurationOption Include="System.Linq.Enumerable.IsSizeOptimized"
                                        Value="true"
                                        Trim="true" />
  </ItemGroup>

The expectation is that we would introduce a property to wrap it, and default the property on iOS/Android/etc. to true.

stephentoub · 2025-01-24T13:38:35Z

Do you have any opinion on how to achieve the "The only problem is maintainability - how do we remember that we should only call the virtuals under a !IsSizeOptimized check." part? I was thinking about leaving the .SpeedOpt.cs files around to keep the virtuals separated at least in files. It won't help much, but it's at least something.

We could have an analyzer just for that library. Better would be to build out the unimplemented parts of feature guards and then use it here. We could explore having a separate build of the library that we test but don't ship and that implements the guarded functionality to throw. But probably most simply, we could rely on size regression tests to point out any place we make a mistake and then fix it, as we do with other performance-focused issues.

ivanpovazan · 2025-01-24T14:39:21Z

@ivanpovazan is it easy enough to measure the size impact of this on iOS? This is replacing the special build of System.Linq with a common assembly that is same across platforms. There's now a feature switch to enable size savings.

Unfortunately it is tricky. Especially since we would like to measure the size impact on MAUI iOS apps and net10 builds are currently unstable.

Would it be possible to apply these changes on top of net9 branch (not sure if there are any dependencies from the point we branched to net10) ? I could then use the official net9 workloads with local build of your changes to see the impact.

MichalStrehovsky · 2025-01-24T15:17:32Z

Would it be possible to apply these changes on top of net9 branch (not sure if there are any dependencies from the point we branched to net10) ? I could then use the official net9 workloads with local build of your changes to see the impact.

Yeah, this applies pretty cleanly to 9.0 and I don't think there's prerequisites. Here's a branch: https://github.com/MichalStrehovsky/runtime/pull/new/sizeoptswitch-net9

The important bit when measuring is that we need to set the feature switch, or this will be a regression for sure.

Here are the numbers for the couple test apps with native AOT using OPTIMIZE_FOR_SIZE. It is better with the ifdef, but not by much. I'd expect a similar picture for Mono AOT.

Size statistics

Pull request #111794

Project	Size before	Size after	Difference
avalonia.app-windows	19096064	18072576	-1023488
hello-minimal-windows	857600	857600	0
hello-windows	1102848	1102848	0
kestrel-minimal-windows	4898816	4804096	-94720
reflection-windows	1748992	1748992	0
webapiaot-windows	9154048	8943104	-210944
winrt-component-full-windows	5728256	5728256	0
winrt-component-minimal-windows	1760256	1760256	0

MichalStrehovsky · 2025-01-24T15:18:58Z

Here are the numbers for the couple test apps with native AOT using OPTIMIZE_FOR_SIZE. It is better with the ifdef, but not by much.

(Need to compare with the table in my previous comment, not the "size before". The "size before" is main branch.)

MichalStrehovsky · 2025-01-24T15:20:38Z

But probably most simply, we could rely on size regression tests to point out any place we make a mistake and then fix it, as we do with other performance-focused issues.

Yeah, I guess that would work. It will require some skill to root cause but if enough people remember that some virtuals are special (and we should probably keep them special in the .SpeedOpt.cs files), it should be workable.

MichalStrehovsky · 2025-01-27T06:40:14Z

/azp run runtime-nativeaot-outerloop

azure-pipelines · 2025-01-27T06:40:23Z

Azure Pipelines successfully started running 1 pipeline(s).

This reverts commit 4f9fad1.

This reverts commit 5412071.

MichalStrehovsky · 2025-01-28T12:43:46Z

@stephentoub @dotnet/ilc-contrib this is ready for review. I tested IL-level trimming with a test app and I'm getting the same results as with native AOT (no new methods seem to be added compared to just using the ifdef, modulo the methods that implement the popular interfaces mentioned in #111743 (comment)).

We can react if there's something unexpected on iDevices later. I don't expect it based on the above testing that I am equipped to do.

I'm also planning to introduce a property to control this and default it to enabled for places that used the ifdef until now:

(@ivanpovazan are these the right places? Did I forget any? Anyone else we should loop in and might know?)

Before I add those, I'd like to have a consensus on the property name. Does anyone have suggestions? Or any objections to UseSizeOptimizedLinq? (Cc @eerhardt for ideas too)

eerhardt

For the MSBuild property, I think UseSizeOptimizedLinq sounds good and fits our existing naming patterns.

src/libraries/System.Linq/src/System.Linq.csproj

src/libraries/Common/tests/TestUtilities/System/PlatformDetection.cs

ivanpovazan · 2025-01-28T14:56:03Z

Yeah, this applies pretty cleanly to 9.0 and I don't think there's prerequisites. Here's a branch: https://github.com/MichalStrehovsky/runtime/pull/new/sizeoptswitch-net9

Thank you, here are the results for:

NativeAOT with MAUI iOS template app:

MAUI ios template app	9.0.100 release	local build of sizeoptswitch-net9	local build of sizeoptswitch-net9 with System.Linq.Enumerable.IsSizeOptimized enabled	local build of release/9.0
zipped bundled (MB)	5,85	5,99	5,87	5,87

Mono with MAUI iOS template app:

MAUI ios template app	9.0.100 release	local build of sizeoptswitch-net9	local build of sizeoptswitch-net9 with System.Linq.Enumerable.IsSizeOptimized enabled	local build of release/9.0
zipped bundled (MB)	15,45	15,83	15,61	15,61

@MichalStrehovsky It seems the optimization only cuts-off the regression introduced, is that expected?

PS I will measure Mono as a follow-up.

src/libraries/Common/tests/TestUtilities/System/PlatformDetection.cs

src/libraries/System.Linq/src/System.Linq.csproj

ivanpovazan · 2025-01-28T17:00:43Z

@MichalStrehovsky:

(@ivanpovazan are these the right places? Did I forget any? Anyone else we should loop in and might know?)

Regarding: iOS/Android

The places you linked for setting the feature switch are correct.

Regarding: wasm/wasi

There is a "top-level" properties file: https://github.com/dotnet/runtime/blob/f1901a00dfcfbcf0a4f8b6aa7459fec30846d1ab/src/mono/nuget/Microsoft.NET.Workload.Mono.Toolchain.Current.Manifest/WasmFeatures.props
which is imported before Blazor SDK that sets the defaults.
I think the new feature switch should go there.

/cc: @pavelsavara @lewing

MichalStrehovsky · 2025-01-28T21:31:57Z

@MichalStrehovsky It seems the optimization only cuts-off the regression introduced, is that expected?

Yep, this just replaces ifdef with a configurable feature switch. There were valid questions in #109978 (that introduces a feature switch for LINQ) why we should have both ifdefs and feature switches. So this is a preparatory change for that.

MichalStrehovsky · 2025-01-28T21:52:22Z

Related pull requests:

MichalStrehovsky · 2025-01-29T09:28:08Z

/azp run runtime-nativeaot-outerloop

MichalStrehovsky · 2025-01-29T09:28:17Z

/azp run runtime-extra-platforms

azure-pipelines · 2025-01-29T09:28:24Z

Azure Pipelines successfully started running 1 pipeline(s).

azure-pipelines · 2025-01-29T09:28:31Z

Azure Pipelines successfully started running 1 pipeline(s).

pavelsavara · 2025-01-29T11:17:39Z

src/libraries/System.Linq/src/System.Linq.csproj

-  <!-- DesignTimeBuild requires all the TargetFramework Derived Properties to not be present in the first property group. -->
-  <PropertyGroup>
-    <TargetPlatformIdentifier>$([MSBuild]::GetTargetPlatformIdentifier('$(TargetFramework)'))</TargetPlatformIdentifier>
-    <OptimizeForSize Condition="'$(TargetPlatformIdentifier)' == 'browser' or '$(TargetPlatformIdentifier)' == 'android' or '$(TargetPlatformIdentifier)' == 'ios' or '$(TargetPlatformIdentifier)' == 'tvos'">true</OptimizeForSize>


OptimizeForSize was set true for some targets but it's not anymore.
WasmFeatures.props set the default for wasm workload, but default runtime pack also needs to be optimized for size - when people don't use workload in Blazor.

OptimizeForSize was set true for some targets but it's not anymore.

Yes, that is the purpose of this PR. To only have one build of this library (#111743 (comment))

WasmFeatures.props set the default for wasm workload, but default runtime pack also needs to be optimized for size - when people don't use workload in Blazor.

Could you please point me to which .targets we need to update? Is it the Microsoft.NET.Sdk.BlazorWebAssembly.6_0.targets linked from #111743 (comment)?

(To be clear, the OptimizeForSize property controls whether we build System.Linq assembly in a not-fully-compatible manner (see the tests I'm touching where it behaves differently) that has better size characteristics. This PR deletes that build and changes it to always build the compatible Linq implementation. It introduces a feature switch that allows switching to the not-fully-compatible mode. The default for the feature switch is disabled. It can be enabled where the tradeoff is worth it (I have PRs out enabling it in places, linked above). The advantage is that now if a customer runs into the incompatibility, they can just set a property and unblock themselves.)

I think maybe we need both

https://github.com/dotnet/sdk/blob/b89ea044c3ac0e028228e73abcc8310b101b63b8/src/BlazorWasmSdk/Targets/Microsoft.NET.Sdk.BlazorWebAssembly.6_0.targets#L43-L50

@maraf
I hope that we run ILLink during publish when wasm-workload is not installed ?
Is there better place ?

and for workload

https://github.com/dotnet/runtime/blob/f1901a00dfcfbcf0a4f8b6aa7459fec30846d1ab/src/mono/nuget/Microsoft.NET.Workload.Mono.Toolchain.Current.Manifest/WasmFeatures.props

Thank you! Folded the BlazorWebAssembly.targets change into dotnet/sdk#46375.

WasmFeatures.props change is already part of the PR here.

Per dotnet/runtime#111743 (comment).

* main: (31 commits) More native AOT Pri-1 test tree bring up (dotnet#111994) Fix BigInteger outerloop test (dotnet#111841) JIT: Run 3-opt once across all regions (dotnet#111989) JIT: Check for profile consistency throughout JIT backend (dotnet#111684) [JIT] Add legacy extended EVEX encoding and EVEX.ND/NF feature to x64 emitter backend (dotnet#108796) [iOS][globalization] Fix IndexOf on empty strings on iOS to return -1 (dotnet#111898) System.Speech: Use intellisense xml from dotnet-api-docs (dotnet#111983) [mono][mini] Disable inlining if we encounter class initialization failure (dotnet#111754) [main] Update dependencies from dotnet/roslyn (dotnet#111946) Update dependencies from https://github.com/dotnet/arcade build 20250129.2 (dotnet#111996) Try changing the ICustomQueryInterface implementation to always return NotHandled instead of Failed to defer back to the ComWrappers impl. (dotnet#111978) Combined dependency update (dotnet#111852) Replace OPTIMIZE_FOR_SIZE with feature switch (dotnet#111743) Fix failed assertion 'FPbased == FPbased2' (dotnet#111787) Add remark to `ConditionalSelect` (dotnet#111945) JIT: fix try region cloning when try is nested in a handler (dotnet#111975) Use IRootFunctions in Tensor.StdDev (dotnet#110641) Remove zlib dependencies from Docker containers (dotnet#111939) Avoid `Unsafe.As` for `Memory<T>` and `ReadOnlyMemory<T>` conversion (dotnet#111023) Cleanup membarrier portability (dotnet#111943) ...

dotnet-issue-labeler bot added the area-System.Linq label Jan 23, 2025

dotnet-policy-service bot assigned MichalStrehovsky Jan 23, 2025

github-actions bot mentioned this pull request Jan 23, 2025

111743 MichalStrehovsky/rt-sz#104

Closed

This was referenced Jan 23, 2025

The Operation will be canceled. The next steps may not contain expected logs. dotnet/dnceng#3008

Open

restarted. Azure DevOps can't recover from restarts. dotnet/dnceng#3879

Open

MichalStrehovsky added 2 commits January 24, 2025 05:19

Temp change to force enable

5412071

Build a single System.Linq assembly

eb3fe5c

MichalStrehovsky force-pushed the sizeoptswitch branch from 5f66281 to eb3fe5c Compare January 24, 2025 07:28

MichalStrehovsky added 2 commits January 24, 2025 14:21

More temp compiler code

4f9fad1

Fix PlatformDetection

4636aba

MichalStrehovsky marked this pull request as ready for review January 24, 2025 13:21

MichalStrehovsky changed the title ~~See if we can replace OPTIMIZE_FOR_SIZE with feature switch~~ Replace OPTIMIZE_FOR_SIZE with feature switch Jan 24, 2025

Adjust tests

e31d983

build-analysis bot mentioned this pull request Jan 27, 2025

slow macOS - "##[error]The job running on agent Azure Pipelines 9 ran longer than the maximum time of 60 minutes." dotnet/dnceng#1883

Open

3 tasks

MichalStrehovsky added 2 commits January 28, 2025 13:34

Revert "More temp compiler code"

b869eea

This reverts commit 4f9fad1.

Revert "Temp change to force enable"

0c8ae74

This reverts commit 5412071.

eerhardt reviewed Jan 28, 2025

View reviewed changes

src/libraries/System.Linq/src/System.Linq.csproj Outdated Show resolved Hide resolved

src/libraries/Common/tests/TestUtilities/System/PlatformDetection.cs Outdated Show resolved Hide resolved

build-analysis bot mentioned this pull request Jan 28, 2025

SIGKILL (OOM?) while running LibraryImportGenerator.Tests w/o actionable log messages or artifacts dotnet/dnceng#2496

Open

3 tasks

stephentoub reviewed Jan 28, 2025

View reviewed changes

src/libraries/Common/tests/TestUtilities/System/PlatformDetection.cs Show resolved Hide resolved

stephentoub reviewed Jan 28, 2025

View reviewed changes

src/libraries/System.Linq/src/System.Linq.csproj Show resolved Hide resolved

stephentoub approved these changes Jan 28, 2025

View reviewed changes

FB

77f2636

Set default for WASM

60297de

MichalStrehovsky requested review from lewing and akoeplinger as code owners January 28, 2025 21:43

MichalStrehovsky mentioned this pull request Jan 28, 2025

Add UseSizeOptimizedLinq feature switch dotnet/sdk#46375

Merged

Rename IsLinqSizeOptimized method to ComputeIsLinqSizeOptimized

da36781

pavelsavara reviewed Jan 29, 2025

View reviewed changes

ivanpovazan mentioned this pull request Jan 29, 2025

Set default for UseSizeOptimizedLinq xamarin/xamarin-macios#22074

Open

MichalStrehovsky added a commit to dotnet/sdk that referenced this pull request Jan 29, 2025

Set default for Blazor

6f54e87

Per dotnet/runtime#111743 (comment).

MichalStrehovsky merged commit adf123c into dotnet:main Jan 30, 2025
149 of 157 checks passed

MichalStrehovsky deleted the sizeoptswitch branch January 30, 2025 08:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace OPTIMIZE_FOR_SIZE with feature switch #111743

Replace OPTIMIZE_FOR_SIZE with feature switch #111743

MichalStrehovsky commented Jan 23, 2025

dotnet-policy-service bot commented Jan 23, 2025

MichalStrehovsky commented Jan 23, 2025

stephentoub commented Jan 23, 2025

MichalStrehovsky commented Jan 23, 2025

MichalStrehovsky commented Jan 23, 2025

sbomer commented Jan 23, 2025

MichalStrehovsky commented Jan 24, 2025

azure-pipelines bot commented Jan 24, 2025

MichalStrehovsky commented Jan 24, 2025

stephentoub commented Jan 24, 2025

ivanpovazan commented Jan 24, 2025

MichalStrehovsky commented Jan 24, 2025

MichalStrehovsky commented Jan 24, 2025

MichalStrehovsky commented Jan 24, 2025

MichalStrehovsky commented Jan 27, 2025

azure-pipelines bot commented Jan 27, 2025

MichalStrehovsky commented Jan 28, 2025

eerhardt left a comment

ivanpovazan commented Jan 28, 2025 •

edited

Loading

ivanpovazan commented Jan 28, 2025 •

edited

Loading

MichalStrehovsky commented Jan 28, 2025

MichalStrehovsky commented Jan 28, 2025

MichalStrehovsky commented Jan 29, 2025

MichalStrehovsky commented Jan 29, 2025

azure-pipelines bot commented Jan 29, 2025

azure-pipelines bot commented Jan 29, 2025

pavelsavara Jan 29, 2025 •

edited

Loading

MichalStrehovsky Jan 29, 2025

MichalStrehovsky Jan 29, 2025

pavelsavara Jan 29, 2025

MichalStrehovsky Jan 29, 2025

Replace OPTIMIZE_FOR_SIZE with feature switch #111743

Replace OPTIMIZE_FOR_SIZE with feature switch #111743

Conversation

MichalStrehovsky commented Jan 23, 2025

dotnet-policy-service bot commented Jan 23, 2025

MichalStrehovsky commented Jan 23, 2025

Size statistics

stephentoub commented Jan 23, 2025

MichalStrehovsky commented Jan 23, 2025

MichalStrehovsky commented Jan 23, 2025

sbomer commented Jan 23, 2025

MichalStrehovsky commented Jan 24, 2025

azure-pipelines bot commented Jan 24, 2025

MichalStrehovsky commented Jan 24, 2025

stephentoub commented Jan 24, 2025

ivanpovazan commented Jan 24, 2025

MichalStrehovsky commented Jan 24, 2025

Size statistics

MichalStrehovsky commented Jan 24, 2025

MichalStrehovsky commented Jan 24, 2025

MichalStrehovsky commented Jan 27, 2025

azure-pipelines bot commented Jan 27, 2025

MichalStrehovsky commented Jan 28, 2025

eerhardt left a comment

Choose a reason for hiding this comment

ivanpovazan commented Jan 28, 2025 • edited Loading

ivanpovazan commented Jan 28, 2025 • edited Loading

MichalStrehovsky commented Jan 28, 2025

MichalStrehovsky commented Jan 28, 2025

MichalStrehovsky commented Jan 29, 2025

MichalStrehovsky commented Jan 29, 2025

azure-pipelines bot commented Jan 29, 2025

azure-pipelines bot commented Jan 29, 2025

pavelsavara Jan 29, 2025 • edited Loading

Choose a reason for hiding this comment

MichalStrehovsky Jan 29, 2025

Choose a reason for hiding this comment

MichalStrehovsky Jan 29, 2025

Choose a reason for hiding this comment

pavelsavara Jan 29, 2025

Choose a reason for hiding this comment

MichalStrehovsky Jan 29, 2025

Choose a reason for hiding this comment

ivanpovazan commented Jan 28, 2025 •

edited

Loading

ivanpovazan commented Jan 28, 2025 •

edited

Loading

pavelsavara Jan 29, 2025 •

edited

Loading