dotnet / fsharp

The F# compiler, F# core library, F# language service, and F# tooling integration for Visual Studio
https://dotnet.microsoft.com/languages/fsharp
MIT License
3.9k stars 783 forks source link

Unnecessary static "init" field checks for some simple cases #6454

Open dsyme opened 5 years ago

dsyme commented 5 years ago

The F# compiler emits "init" field checks to prevent access of bindings before initialization is complete. This is for soundness.

However, this impacts performance for a normal implementation of some immutable types where these safety checks are not needed.

Thanks to @auduchinok for reporting this.

Repro steps

Consider this code:

[<Struct>]
type S<'T>(x: 'T[]) = 
    static let empty = S<'T>(null)
    static member Empty = empty

The compiled form of this code has an inserted static init field which is checked on each access to Empty, see below. The same applies for non-generic and non-struct types.

The F# compiler does this for good reasons - without it it is possible to read uninitialized bindings too easily and without any warning, e.g. consider the following small variation:

[<Struct>]
type S2<'T>(x: 'T[]) = 
    static let empty = S2<'T>.M()
    static member M() = empty

Ideally, the F# compiler would automatically detect the most important cases where safety checks are not required.

Here is the ILDASM for the original:

.method private specialname rtspecialname static 
        void  .cctor() cil managed
{
  // Code size       20 (0x14)
  .maxstack  8
  IL_0000:  ldnull
  IL_0001:  newobj     instance void valuetype A/S`1<!T>::.ctor(!0[])
  IL_0006:  stsfld     valuetype A/S`1<!0> valuetype A/S`1<!T>::empty
  IL_000b:  ldc.i4.1
  IL_000c:  volatile.
  IL_000e:  stsfld     int32 valuetype A/S`1<!T>::init@4
  IL_0013:  ret
} // end of method S`1::.cctor

and

.method public specialname static valuetype A/S`1<!T> 
        get_Empty() cil managed
{
  // Code size       23 (0x17)
  .maxstack  8
  IL_0000:  volatile.
  IL_0002:  ldsfld     int32 valuetype A/S`1<!T>::init@4
  IL_0007:  ldc.i4.1
  IL_0008:  bge.s      IL_0011
  IL_000a:  call       void [FSharp.Core]Microsoft.FSharp.Core.LanguagePrimitives/IntrinsicFunctions::FailStaticInit()
  IL_000f:  br.s       IL_0011
  IL_0011:  ldsfld     valuetype A/S`1<!0> valuetype A/S`1<!T>::empty
  IL_0016:  ret
} // end of method S`1::get_Empty
auduchinok commented 5 years ago

A possible way to overcome this could be adding a special attribute effectively disabling the check, as it's been discussed. It would allow doing some nasty stuff but probably is a good compromise for cases it's needed in unless there's no other way that isn't too complex to implement.

dsyme commented 5 years ago

I'm not sure how to fix this through automatic analysis. Detecting the lack of recursive access is annoyingly hard, and has to be done at "just the right stage" in the process of checking classes and converting them to their TAST form.

As @auduchinok says, the alternative would be to have an attribute disabling initialization safety checks for a module, or type, or the like. This would make sense for a number of micro-perf reasons for code the programmer is happy to declare safe, akin to DefaultValue and so on. Given it would be opt-in this would be ok.