Skip to content

Optimisation failure with structs in referenced projects #3923

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

Open
saul opened this issue Nov 10, 2017 · 2 comments
Open

Optimisation failure with structs in referenced projects #3923

saul opened this issue Nov 10, 2017 · 2 comments
Labels
Area-Compiler-Optimization The F# optimizer, release code gen etc. Bug Impact-Medium (Internal MS Team use only) Describes an issue with moderate impact on existing code.
Milestone

Comments

@saul
Copy link
Contributor

saul commented Nov 10, 2017

Structs referenced from other projects have a significant optimisation failure whereby an instance of the struct cannot be passed to functions - it is always re-instantiated to empty just before the function call.

Repro steps

Imagine you have the following C# project with a single file:

namespace ClassLibrary1
{
    public struct SomeStruct
    {
        private string _value;
        public string Value => _value;

        public void Set(string value)
        {
            _value = value;
        }
    }
}

The above project is referenced from F#:

open ClassLibrary1

let someFunc (s : SomeStruct) =
    printfn "From inner: %A" s.Value

[<EntryPoint>]
let main argv = 
    
    let s = SomeStruct()
    s.Set("there")
    printfn "Pre call:   %A" s.Value
    someFunc s
    printfn "Post call:  %A" s.Value

    0

Expected behavior

Output of the program is:

Pre call:   "there"
From inner: "there"
Post call:  "there"

Actual behavior

With optimisations enabled (e.g. Release mode), the output is:

Pre call:   "there"
From inner: <null>
Post call:  "there"

Known workarounds

No known workaround.

Related information

Severe bug, stopping me from using a C# library that uses structs.

The IL for the main function looks like:

.method public static int32  main(string[] argv) cil managed
{
  .entrypoint
  .custom instance void [FSharp.Core]Microsoft.FSharp.Core.EntryPointAttribute::.ctor() = ( 01 00 00 00 ) 
  // Code size       40 (0x28)
  .maxstack  4
  .locals init ([0] valuetype [ClassLibrary1]ClassLibrary1.SomeStruct s,
           [1] valuetype [ClassLibrary1]ClassLibrary1.SomeStruct V_1)
  IL_0000:  nop
  IL_0001:  ldloca.s   V_1
  IL_0003:  initobj    [ClassLibrary1]ClassLibrary1.SomeStruct
  IL_0009:  ldloc.1
  IL_000a:  stloc.0
  IL_000b:  ldloca.s   s
  IL_000d:  ldstr      "there"
  IL_0012:  call       instance void [ClassLibrary1]ClassLibrary1.SomeStruct::Set(string)
  IL_0017:  ldloca.s   V_1 // WHY IS THIS HERE?
  IL_0019:  initobj    [ClassLibrary1]ClassLibrary1.SomeStruct // WHY IS THIS HERE?
  IL_001f:  ldloc.1
  IL_0020:  call       void Program::someFunc(valuetype [ClassLibrary1]ClassLibrary1.SomeStruct)
  IL_0025:  nop
  IL_0026:  ldc.i4.0
  IL_0027:  ret
} // end of method Program::main

IL_0017 and IL_0019 have been added by the optimiser - in Debug mode these instructions are not here, and the bad behaviour is not present.

Thanks

@dsyme
Copy link
Contributor

dsyme commented Nov 17, 2017

@saul Yes agreed, the behaviour shouldn't change under optimization

However the correct code to mutate the struct is this:

let mutable s = SomeStruct()

My first question is why we are not getting a "struct is being mutated" warning/error.

@dsyme dsyme added Bug Impact-High (Internal MS Team use only) Describes an issue with extreme impact on existing code. labels Nov 17, 2017
@dsyme dsyme changed the title Major optimisation failure with structs in referenced projects Optimisation failure with structs in referenced projects Nov 17, 2017
@dsyme
Copy link
Contributor

dsyme commented Nov 17, 2017

@saul OK, I've looked into this more.

First, if your struct types are mutable then the right way to code with them is like this:

let mutable x = SomeStruct()
x.PropertyAccessOnAStruct
x.MethodOnAStruct() 

etc. That's just the right thing to do.

Now, let's consider

let x = SomeStruct()
x.PropertyAccessOnAStruct

As a value-based language where let x = ... declares an immutable value, F# has special rules time with mutable structs. F# does the following:

  • If SomeStruct is declared in F# then it can be analyzed to determine if it is mutable or immutable (it's nearly always the latter in F# code, being quite hard to declare a mutable struct). In that case, we know that the property access doesn't mutate the struct, and we don't need to take a defensive copy before taking its address.

  • However, if SomeStruct is declared in .NET, we currently always assume it is immutable, or at least that the property access doesn't mutate the struct. (As an aside, this wasn't the case in early versions of F# (1.x) but was the case from F# 2.0 onwards. I can't recall the exact reasoning but I'm assuming the change was made for performance reasons - otherwise too many defensive copies would be taken)

  • A couple of operations such as property setters are inferred to be as "definitely mutates" (DefinitelyMutates) and an outright error will be given if they are used on let x = SomeStruct()

  • There is a level 5 optional warning you can turn on to show when defensive copies are made

Now, that leaves us in an awkward situation where we are falsely assuming your .NET struct type to be immutable. As a result, I propose to make the following adjustments:

  1. If a method call has void/unit return type then we will label it a "likely mutates" and give a warning (on by default) when used on an .NET let-declared struct value. In your code, this would now give the following warning at your call to x.Set(...):

image

This is only a warning since it possible the user has I/O operations such as x.Dump() on a struct type. They would need to turn the warning off. But the case should be rare enough that we are doing the right thing here. For stability reasons we won't change the execution behaviour of "likely mutates" operations

  1. We give an optional warning (off by default) whenever a .NET struct type has been assumed to be immutable. However this is very noisy, as all property getter accessors are assumes to be immutable

This is implemented in #3968

@dsyme dsyme added Impact-Medium (Internal MS Team use only) Describes an issue with moderate impact on existing code. and removed Impact-High (Internal MS Team use only) Describes an issue with extreme impact on existing code. labels Nov 17, 2017
@cartermp cartermp added this to the 16.0 milestone Jul 21, 2018
@cartermp cartermp modified the milestones: 16.0, 16.1 Feb 21, 2019
@cartermp cartermp modified the milestones: 16.1, 16.2 Apr 23, 2019
@cartermp cartermp modified the milestones: 16.2, Backlog Apr 30, 2019
@dsyme dsyme added Area-Compiler-Optimization The F# optimizer, release code gen etc. and removed Area-Compiler labels Mar 31, 2022
@vzarytovskii vzarytovskii moved this to Not Planned in F# Compiler and Tooling Jun 17, 2022
@vzarytovskii vzarytovskii reopened this Jan 4, 2024
@github-project-automation github-project-automation bot moved this from Done to In Progress in F# Compiler and Tooling Jan 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compiler-Optimization The F# optimizer, release code gen etc. Bug Impact-Medium (Internal MS Team use only) Describes an issue with moderate impact on existing code.
Projects
Archived in project
Development

No branches or pull requests

5 participants