Skip to content

Conversation

@T-Gro
Copy link
Member

@T-Gro T-Gro commented Jun 6, 2023

F# compiler already knows how to produce reference assemblies.
One of the key elements for them is MVID, a deterministic hash identifier used for comparing 'before' and 'after' versions of an assembly.

If MVID of project 'x' is unchanged, projects depending on 'x' do not need to recompile and can just copy over the new 'x.dll' without being recompiled themselves. This significantly reduces costs in bigger nets of projects where a change is made in a low-level project, but the change does not alter the visible surface area - e.g. just implementation details are changed, or private members are added.

As of now, the MVID for F# reference assemblies has been calculated both as a hash of IL code, but also as hash of all the embedded resources. For F# that specifically means the Signature and Optimization data (byte streams compiled as resources) used for consumption by other F# projects, but also sections for byte arrays or constant strings for an assembly.

This PR changes the MVID calculation for reference assemblies by not including these resources, and instead replacing them with a custom hash number derived from what I call 'implied signature of a file'.

In a nutshell, it attempts to be equivalent to " auto-generate .fsi signature file and hash that", but without all the intermmediate string allocations and making things pretty - i.e. following the same TAST constructs, just hashing them instead of rendering to a string.

@T-Gro
Copy link
Member Author

T-Gro commented Jun 9, 2023

Hi @T-Gro, very cool to see this work! More stable mvids are absolutely welcome and highly valued.

I noticed that the following scenario is not passing:

[<InlineData("Adding a private let binding",
             (*BEFORE*)"""
module Foo

let a b = b - 1
""",
(*AFTER*)"""
module Foo

let a b = b - 1
let private c = 'd'
""")>]

I would expect the mvids to be the same. Would that be a correct assumption or is there still something that ends up in the signature when adding a new private binding?

Thanks for spotting this, commiting it as a (failing) test case and will investigate.
I had a very similar case for private function added, which passes - there must be some subtle difference between the two (most likely the binding is exposed as internal and not private, or something like that)

@charlesroddie
Copy link
Contributor

Fantastic to see progress on this.

Re: private/internal functions, in principle shouldn't both private and internal functions be excluded?

@T-Gro
Copy link
Member Author

T-Gro commented Jun 12, 2023

Fantastic to see progress on this.

Re: private/internal functions, in principle shouldn't both private and internal functions be excluded?

If the assembly does not have a single [<InternalsVisibleToAttribute >] https://learn.microsoft.com/en-us/dotnet/api/system.runtime.compilerservices.internalsvisibletoattribute?view=net-7.0 , impact of internals can be removed too.

But if there is at least one project to which internals are made visible, all internals have to be counted for.

@vzarytovskii
Copy link
Member

@T-Gro mvid calculation should take into account optimization data, otherwise inlining might work incorrectly (not rebuilding ref assembly, when inlined code is changed, but not type) when building against reference assemblies.

@KevinRansom
Copy link
Contributor

I also wonder, will the signature files continue to contain the additional unnecessary private data?

@T-Gro
Copy link
Member Author

T-Gro commented Jun 27, 2023

@KevinRansom ; @vzarytovskii :

Please have a look one more time guys.
Now it does include optimization data and there is a case with an inline value to prove that hash changes if it's impl changes.

However, the test suite now relies on the --nooptimizationdata flag, which only keeps MustInline Vals for optimization, but not the Optional. It turns out, typical plain-old-nothing-special bindings are almost always ValInline.Optional, which would kind of break purpose of the ref assembly improvements for F#.

Therefore, I will raise a sibling PR with a new approach we discussed with Vlad:

When building in DEBUG && when ref assemblies are present, I propose to build the project with --nooptimizationdata . This practically means that only things marked as inline really get inlined (including SRTPs), but all the ValInline.Optional Vals are not.

WDYT?

@T-Gro
Copy link
Member Author

T-Gro commented Jun 27, 2023

Take a look at the DeterminicTests.fs, now it is all passing.
If I remove the --nooptimizationdata flag, I would start to get failures - even when changing body of a private binding which is not marked as "inline". (because ValInline.Optional means that the compiler/optimizer is free to decide whether to inline or not)

@T-Gro T-Gro requested a review from vzarytovskii July 4, 2023 08:24
KevinRansom
KevinRansom previously approved these changes Jul 4, 2023
Copy link
Contributor

@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.

A minor factoring quibble. Otherwise it looks okay.

@KevinRansom KevinRansom dismissed their stale review July 5, 2023 16:16

want to have another look

@T-Gro T-Gro merged commit c08591a into dotnet:main Jul 13, 2023
@T-Gro T-Gro deleted the mvid-refassembly-stability branch July 13, 2023 16:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

6 participants