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

Specifying the initialization type of static constructors #4448

Closed
GSPP opened this issue Aug 8, 2015 · 24 comments
Closed

Specifying the initialization type of static constructors #4448

GSPP opened this issue Aug 8, 2015 · 24 comments
Labels
Area-Compilers fabric-bot-test Testing the impact of changes to the fabric bot Need More Info The issue needs more information to proceed.
Milestone

Comments

@GSPP
Copy link

GSPP commented Aug 8, 2015

The CLR supports two initialization modes for static constructors depending on whether the beforefieldinit flag is present. See http://csharpindepth.com/Articles/General/Beforefieldinit.aspx for discussion.

It is unpredictable to most developers which of the two modes will be in effect. In fact in my estimation most developers do not have a good understanding of when static constructors even run. This is a long-standing thorny issue.

In my mind it has been a mistake not to make all static constructors beforefieldinit. I cannot think of any case in which that might be a bad idea from a language design perspective. The idea of static constructors is to initialize static fields. As long as any static field access is preceded by static constructor execution all should be fine. There is no reason to force initialization before any other member is being used. Also, the precise moment when initialization happens should not matter. beforefieldinit implements just that.

That said for compatibility reasons this change probably cannot be done now. Maybe it can be done guarded by some compatibility switch?!

I propose:

  1. Static initializers and constructors should behave exactly the same way regarding the beforefieldinit flag. Whether the flag is set or not is of secondary importance.
  2. There should be an attribute applicable to any class that specifies the desired beforefieldinit behavior (Auto, Lazy, Unconstrained).
  3. Optionally, beforefieldinit should always be set. Possibly guarded by a compatibility switch (maybe the semantics of the BinaryCompatibility class, an assembly-level attribute or a compiler-level behavior?).

That should clean up the mess.

@svick
Copy link
Contributor

svick commented Aug 9, 2015

I'm not quite following your argument. You say that it shouldn't matter when a static constructor is run, but you also say that whether beforefieldinit is set is important enough to warrant a compatibility switch (which is something C# seems to be trying very hard to avoid). Those two seem to be contradictory to me.

Personally, I think an attribute would be useful for the rare cases where having beforefieldinit or not does matter. But I disagree about your other proposals, especially the one which would change code that is currently deterministic into non-deterministic (by setting beforefieldinit on types with a static constructor), because I would consider that a breaking change.

@GSPP
Copy link
Author

GSPP commented Aug 9, 2015

My preference for beforefieldinit is for mainly performance reasons. I think sane code should not depend in any way on when a cctor is run. I acknowledge that bad code might exist, though, that relies on the specific initialization order that a certain runtime has given it.

I can live with any choice here, either always make it beforefieldinit or never (with an attribute possibly overriding the choice).

@svick Do you also disagree with proposal (1)? That seemed quite uncontroversial to me. It seems you only disagree with (3), correct?

@svick
Copy link
Contributor

svick commented Aug 9, 2015

I disagree with (1). As far as I can see, the only gain there is consistency. And while consistency is good, I don't think it outweighs the disadvantages here:

If you consistently set beforefieldinit, you lose determinism for types with static constructors.
If you consistently don't set beforefieldinit, you lose performance for types without static constructors.

The current situation is confusing, but I think there is code out there that depends on both determinism in one case and performance in the other case, and that new version of the compiler shouldn't break that.

@mburbea
Copy link

mburbea commented Aug 10, 2015

I'm not sure of the determinism of beforefieldinit differing, but I do find it disappointing that performance suffers greatly when you do add a static constructor to a generic class.

I personally would agree to making it so that I may explicitly declare my intent to declare something beforefieldinit, so I no longer have to use something ugly like this:

static class Foo<T>
{
        private static Bar<T> b;
        private static Baz<T> bz;
        private static Type type = typeof(T);
        private static string Boo = SomeMethod(out b, out bz);
}

@gafter gafter added this to the Unknown milestone Aug 14, 2015
@gafter
Copy link
Member

gafter commented Aug 14, 2015

Can you please provide a program that demonstrates how the change would be visible to user programs?

@GSPP
Copy link
Author

GSPP commented Aug 14, 2015

Here's a program:

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace ConsoleApplication10
{
    class Program
    {
        static void Main(string[] args)
        {
            //if (Environment.TickCount == 0)
            C1.F();
            //if (Environment.TickCount == 0)
            C2.F();
        }
    }

    class C1
    {
        public static readonly int X = Util.GenerateSideEffect("cctor");

        public static void F()
        {
            Console.WriteLine("Static method");
        }
    }

    class C2
    {
        public static readonly int X = Util.GenerateSideEffect("cctor");

        static C2()
        {
            //Does nothing.
        }

        public static void F()
        {
            Console.WriteLine("Static method");
        }
    }

    static class Util
    {
        public static int GenerateSideEffect(string msg)
        {
            Console.WriteLine(msg);
            return 0;
        }
    }
}

This outputs:

Static method
cctor
Static method

The two types show different initialization behavior. This is on .NET 4.5.2 Release x64 without debugger.

As you can see it is contrived. For practical programs the difference in behavior should almost never matter. It is insane to rely on this behavior. This request mostly about performance and about simplifying the language.

@gafter
Copy link
Member

gafter commented Aug 15, 2015

You've found a pretty serious bug in the Roslyn C# compiler. This is a regression from the previous version. The C# language specification requires that the static initializers be executed before the class is "used". That is not occurring in C1.

For clarity, I've modified your test case slightly. It can be seen to behave differently using the previous versus current versions of the C# compiler.

using System;

namespace ConsoleApplication10
{
    class Program
    {
        static void Main(string[] args)
        {
            C1.F();
            C2.F();
        }
    }

    class C1
    {
        public static readonly int X = Util.GenerateSideEffect("C1.cctor");

        public static void F()
        {
            Console.WriteLine("C1.F");
        }
    }

    class C2
    {
        public static readonly int X = Util.GenerateSideEffect("C2.cctor");

        static C2()
        {
            //Does nothing.
        }

        public static void F()
        {
            Console.WriteLine("C2.F");
        }
    }

    static class Util
    {
        public static int GenerateSideEffect(string msg)
        {
            Console.WriteLine(msg);
            return 0;
        }
    }
}

@gafter gafter added the Bug label Aug 15, 2015
@gafter gafter modified the milestones: 1.1, Unknown Aug 15, 2015
@gafter gafter added the Tenet-Compatibility Violation of forwards/backwards compatibility in a design-time piece. label Aug 15, 2015
@GSPP
Copy link
Author

GSPP commented Aug 15, 2015

Are you sure? According to Jon Skeets article (http://csharpindepth.com/Articles/General/Beforefieldinit.aspx) there always was a variety of behaviors possible.

http://tryroslyn.azurewebsites.net/#f:r/K4Zwlgdg5gBAygTxAFwKYFsDcAobECG6qIADvgMaowDCA9hCLQDaoCCJJTY5+yY9ARgAM2AN7YYkmOSb4QIGAAUATrSjLCEqeKm6YKXtxgA3WmAAmMALL5IAChTLIUANoBdGPmVQQASi16MDqBetQCAHQAYna+OCGhAExRMXEhAL4BMBmZMnIKYZnBgSTAAEZc5PrIhpXKqPjm9EwIMJDIMAAaMAC8MACqfEzhAOKoEKgaaHAWqACiAGbzqOTIdgBEYeHkK7TKa7G48SXlRgZ8laYWMNH+8UXxNPSMLOEA6k5oADKQqOubkftUoEMrpsrpcvIaAlCpldMcKlUajA6g0mi02p0ev1BiMxhNeKhpuY5otlqsNkltshdoDDiEzkZqAkYrDtKzAgB6DkAEVoxBgEFoyAAFs5wuysnTimUEQyLmZLDcJfd4nQGMxUG8PqhvuM/kkAQd4iCpGCpHLpLJIQMwEwYUcZadqudWhB2qNxpNCTMFksVg5kE5oDB0CAoLcQiqQmrnpr3mAvj87KHw0CHnVkMBlBAYEI06DMhk0kAA==

The decompiled output shows that one type is "beforefieldinit" and the other one is not. This matches my expectations. I admit I have not read the C# spec and relied on memory.

In fact I had to play a little with the code to get it to do different things for the two types because this also depends on the runtime. Different runtimes have different laziness behaviors.

Even if this turns out to be a bug I request a language change for the next version of C#. This really is an unpredictable spot of the language at the moment.

@gafter
Copy link
Member

gafter commented Aug 15, 2015

Reading the spec more carefully, I see

10.5.5.1 Static field initialization

The static field variable initializers of a class correspond to a sequence of assignments that are executed in the textual order in which they appear in the class declaration. If a static constructor (§10.12) exists in the class, execution of the static field initializers occurs immediately prior to executing that static constructor. Otherwise, the static field initializers are executed at an implementation-dependent time prior to the first use of a static field of that class. ...

and in 10.12

The static constructor for a closed class type executes at most once in a given application domain. The execution of a static constructor is triggered by the first of the following events to occur within an application domain:

  • An instance of the class type is created.
  • Any of the static members of the class type are referenced.

(my emphasis)

It appears the observed behavior may be permitted after all.

@gafter gafter added Bug and removed Bug labels Aug 25, 2015
@AlekseyTs
Copy link
Contributor

Using code from @gafter 's comment above, I do not see any difference with respect to presence of beforefieldinit flag in resulting metadata between Roslyn and previous version of the compiler. Runtime behavior, however, is slightly different unless /optimize+ is used with Roslyn compiler. It looks like /optimize- (which is the default) in Roslyn affects binary in some other way, which allows CLR to run the code in a less optimal way.

@AlekseyTs
Copy link
Contributor

I confirmed that the difference in behavior is due to the presence of

[assembly: DebuggableAttribute(DebuggableAttribute.DebuggingModes.EnableEditAndContinue | 
                               DebuggableAttribute.DebuggingModes.IgnoreSymbolStoreSequencePoints | 
                               DebuggableAttribute.DebuggingModes.DisableOptimizations | 
                               DebuggableAttribute.DebuggingModes.Default ) ]

in the binary produced by Roslyn compiler with /optimize-.

Previous version of the compiler emits it only when /debug+ is specified, the binary has the same runtime behavior then.

I believe that the change in when the attribute is emitted and with what values is an intentional change ( @tmat please confirm ), therefore, there is no bug in Roslyn's behavior.

@AlekseyTs
Copy link
Contributor

Moving this issue out of Milestone 1.1 since there is no bug to fix.

@AlekseyTs AlekseyTs removed Bug Tenet-Compatibility Violation of forwards/backwards compatibility in a design-time piece. labels Sep 17, 2015
@AlekseyTs AlekseyTs modified the milestones: Unknown, 1.1 Sep 17, 2015
@AlekseyTs AlekseyTs removed their assignment Sep 17, 2015
@tmat
Copy link
Member

tmat commented Sep 18, 2015

@AlekseyTs Yes, the debuggable attribute change was intentional.

@tmat
Copy link
Member

tmat commented Sep 18, 2015

In VS the default configurations translate to command line options as follows:

Release
/optimize+ /debug:pdbonly
Debug
/optimize- /debug:full

The code above behaves the same between native compiler and Roslyn in these configurations.

@jaredpar
Copy link
Member

jaredpar commented Dec 7, 2015

@tmat sounds like the behavior is the same between native and Roslyn compiler here. So no regression.

Hence back to the original question: should we change the behavior. My instinct here is no. Even though user code ideally should not depend on the order of static constructors, in practice it definitely depends on the order of static constructors. There are many examples of this we've seen in the past. Hence changing this = breaking a lot of customers.

Breaking customers has a very high bar to meet. I don't see the changes of this behavior meeting that bar.

Maybe it can be done guarded by some compatibility switch?!

Every switch is another level in our test matrix. Even switches that guard preferred behavior are costly due to the test impact.

Thoughts?

@jaredpar jaredpar added the Need More Info The issue needs more information to proceed. label Dec 7, 2015
@MgSam
Copy link

MgSam commented Dec 7, 2015

The better proposal is an attribute that allows you opt-in to the desired behavior. That way you don't break anyone, but if your code requires a certain behavior you can specify it explicitly. That's much better than implementation-dependent behavior.

@GSPP
Copy link
Author

GSPP commented Dec 7, 2015

Teams could apply the attribute across the board driven by automated tools such as Resharper or Roslyn analyzers. There could be a team-wide policy to apply an attribute a certain way. For example Roslyn surely would apply the the option that results in the best possible performance.

@jaredpar
Copy link
Member

jaredpar commented Dec 7, 2015

The attribute though is little different from a compiler switch when it comes to our testing matrix. It's actually harder to test given the level of granularity on which it operates.

For example Roslyn surely would apply the the option that results in the best possible performance.

I disagree. It's unclear that static constructor is a performance issue for Roslyn. We profile extensively internally and items like symbol allocation, metadata cracking and GC dominate our profiles. I doubt any changes to static ctors would show up at all in these profiles.

@GSPP
Copy link
Author

GSPP commented Dec 7, 2015

The cctors will not show up because they are tiny but all static member accesses might become slower because they need to runtime check whether the cctor already ran. That is the benefit of allowing the JIT to eagerly call the cctor.

So the cost is spread across very many lines of code, like thousands.

@jaredpar
Copy link
Member

jaredpar commented Dec 7, 2015

Agree it's small and spread out. The term I generally use for this is "peanut butter cost" (gets spread out everywhere). But I still go back to my earlier point, this cost is dwarfed by other perf concern. Using it in Roslyn would not be a motivator for adding this feature.

@GSPP
Copy link
Author

GSPP commented Dec 7, 2015

True.

@GSPP
Copy link
Author

GSPP commented Dec 20, 2015

Joe Duffy reports from the Midori project (a managed OS): http://joeduffyblog.com/2015/12/19/safe-native-code/

I was astonished the day I learned that 10% of our code size was spent on static initialization checks.

It's in the section "Statics". They introduced a [BeforeFieldInit] that was "liberally applied by developers".

@GSPP
Copy link
Author

GSPP commented Feb 14, 2016

Stephen Toub just committed to corefx to mechanically convert many cctors to field initializers in order to avoid the performance penalties involved with missing beforefieldinit.

dotnet/corefx#6016

This might serve as a motivating example for why control over beforefieldinit is useful.

@ghost ghost added the fabric-bot-test Testing the impact of changes to the fabric bot label Aug 10, 2021
@ghost ghost closed this as completed Sep 16, 2021
@ghost
Copy link

ghost commented Sep 16, 2021

Closing this issue as we've seen no reply to the request for more information. If you are able to get the requested information, please add it to the issue and we will retriage it.

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers fabric-bot-test Testing the impact of changes to the fabric bot Need More Info The issue needs more information to proceed.
Projects
None yet
Development

No branches or pull requests

9 participants