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 trimming XML files to FSharp.Core. #13853

Merged
merged 22 commits into from
Sep 14, 2022

Conversation

teo-tsirpanis
Copy link
Contributor

Contributes to #12819.

This PR adds two XML files as embedded resources in FSharp.Core that when trimming instruct the linker to:

  • Remove the optimization and signature info blobs from the assembly, yielding an easy 1.2 MB size reduction.
  • Remove instances of all attributes declared in FSharp.Core if the _AggressiveAttributeTrimming feature switch is enabled. Only four of the fifty one attributes are accessed at runtime by FSharp.Core, and by removing them we might observe different behavior (or maybe not), but the feature switch's documentation allows them.

@vzarytovskii
Copy link
Member

@teo-tsirpanis question - trimming is opt-in now, right?

@teo-tsirpanis
Copy link
Contributor Author

Yes, it can be enabled only on self-contained deployments, but since .NET 7 all assemblies are trimmed by default if we enable trimming.

@teo-tsirpanis teo-tsirpanis changed the title Add trimming xml files to FSharp.Core. Add trimming XML files to FSharp.Core. Sep 8, 2022
@vzarytovskii
Copy link
Member

@teo-tsirpanis so, what would be a default behaviour now for the app which packages FSharp.Core in .NET7? Will it be trimming resources and attributes if it's an aggressive mode?

@dsyme
Copy link
Contributor

dsyme commented Sep 8, 2022

@teo-tsirpanis What automated testing can we have for this?

@teo-tsirpanis
Copy link
Contributor Author

@vzarytovskii yes, the resources will be trimmed away always if trimming is enabled; these resource files are used only by the compiler.

@dsyme no, the repository doesn't seem to have any tests related to trimmability at all.

@vzarytovskii
Copy link
Member

vzarytovskii commented Sep 8, 2022

@vzarytovskii yes, the resources will be trimmed away always if trimming is enabled; these resource files are used only by the compiler.

Yeah, I understand that. What about annotating library itself (FSharp.Core) - can we somehow put some attribute on types/members to mark them safe/unsafe to trim?

@teo-tsirpanis
Copy link
Contributor Author

teo-tsirpanis commented Sep 8, 2022

What about annotating library itself

This is part 2 of #12819. I started doing it and will submit it in a separate PR. Can't do 100% of it though, it gets extremely tedious after one point and with diminishing returns.

Trimming away the resources and paving the way to remove the attributes is the trimming enhancement I am more interested in.

Copy link
Member

@KevinRansom KevinRansom left a comment

Choose a reason for hiding this comment

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

There is the ability to compress the sig and optdata files, the names are different, we will be moving over to compressed metadata for FSharp.Core shortly.

    <resource name="FSharpOptimizationInfo.FSharp.Core" action="remove" />
    <resource name="FSharpSignatureInfo.FSharp.Core" action="remove" />

Also include:

    <resource name="FSharpOptimizationCompressedData.FSharp.Core" action="remove" />
    <resource name="FSharpSignatureCompressedData.FSharp.Core" action="remove" />

@teo-tsirpanis
Copy link
Contributor Author

Done.

Copy link
Member

@KevinRansom KevinRansom left a comment

Choose a reason for hiding this comment

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

Thanks

@dsyme
Copy link
Contributor

dsyme commented Sep 8, 2022

@dsyme no, the repository doesn't seem to have any tests related to trimmability at all.

@vzarytovskii Can we have them added please? :))

@vzarytovskii
Copy link
Member

@dsyme no, the repository doesn't seem to have any tests related to trimmability at all.

@vzarytovskii Can we have them added please? :))

We can, I'm not entirely sure what we would like test though. Just the lack of resources and types in resulting assembly after trimming? I guess it will make more sense to add such tests to trimmer instead.

This will also involve the whole SDK (as oppose to just running fsc most of the times).

@dsyme
Copy link
Contributor

dsyme commented Sep 9, 2022

We can, I'm not entirely sure what we would like test though. Just the lack of resources and types in resulting assembly after trimming? I guess it will make more sense to add such tests to trimmer instead.

I guess, we would test whether a trim of a simple app compiles, runs and the trimmed binary size is below some threshold. If it goes above, we should fail the test, forcing us to adjust a baseline if we regress

It would be ok to use the SDK we're compiling with to do this, as long as it's compiling with our compiler and using our FSHarp.Core.

@vzarytovskii
Copy link
Member

vzarytovskii commented Sep 12, 2022

We can, I'm not entirely sure what we would like test though. Just the lack of resources and types in resulting assembly after trimming? I guess it will make more sense to add such tests to trimmer instead.

I guess, we would test whether a trim of a simple app compiles, runs and the trimmed binary size is below some threshold. If it goes above, we should fail the test, forcing us to adjust a baseline if we regress

It would be ok to use the SDK we're compiling with to do this, as long as it's compiling with our compiler and using our FSHarp.Core.

I'm thinking of the following:

  1. Build our compiler and FSharp.Core
  2. Package FSharp.Core locally
  3. Have a project which is pointing to our compiler, has disabled implicit FSharp.Core, and references local FSharp.Core
  4. Publish trimmed app (self-contained app, which uses some FSharp.Core functionality)
  5. Run it, check output, check size
  6. Put it into separate CI leg

@vzarytovskii
Copy link
Member

@teo-tsirpanis @dsyme I've added a new leg which will use locally built compiler and FSharp.Core in the project, and will publish and check trimmed build. @teo-tsirpanis can you please take a look if it makes sense.

Let's see if it passes the CI, since I've probably broken something in yml.

The program itself is just a hello world, please feel free to add more FSharp.Core functions.

@vzarytovskii
Copy link
Member

On a second thought, we should probably test for exact size of the FSharp Core, and not <=

@teo-tsirpanis
Copy link
Contributor Author

Exact size? That sounds super brittle. What I had in mind was to check if the optimization and signature data resources are present in the FSharp.Core assembly (which is in the strictest sense what we want to check in this PR).

A more complex idea I thought of is to somehow monitor the trimmed size (less objective; what constitutes an average F# app?) number of trimming warnings on FSharp.Core and alert us if they increased.

@vzarytovskii
Copy link
Member

vzarytovskii commented Sep 13, 2022

Exact size? That sounds super brittle.

I would like us to check for exact size, because we would like know why/when the change (either codegen change, or framework upgrade) caused the trimming size change.

What I had in mind was to check if the optimization and signature data resources are present in the FSharp.Core assembly (which is in the strictest sense what we want to check in this PR).

This will be implicit, I've checked that optimization and signature data are not in the resources. If this will ever change, the size check will fail. I will also add matrix to check both compressed and uncompressed resources.

A more complex idea I thought of is to somehow monitor the trimmed size (less objective; what constitutes an average F# app?) number of trimming warnings on FSharp.Core and alert us if they increased.

I did not see any trimming warnings for this particular test yet, I can enable warnings as errors, and as we evolve this particular test suite, we can see if we hit any.

I will make the check explicit - check for exact size, let's see if it's too painful for us. We can change it any moment.

@vzarytovskii
Copy link
Member

vzarytovskii commented Sep 13, 2022

Funny enough, it seems it trims out resources locally (they aren't in the dll), but does not in CI:
From CI:
image

And local:
image

Update: binlog is here: https://dev.azure.com/dnceng-public/public/_build/results?buildId=14883&view=artifacts&pathAsName=false&type=publishedArtifacts

@teo-tsirpanis do you know what could be the problem here?

@vzarytovskii
Copy link
Member

Ok, I think I know why.
My local build was rolled forward to rc1, and remote one was on p6

@vzarytovskii vzarytovskii merged commit e05bdc2 into dotnet:main Sep 14, 2022
@teo-tsirpanis teo-tsirpanis deleted the illink-xmlfiles branch September 14, 2022 22:43
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.

4 participants