roslyn: .Net 4.6 optimizations breaking on 4.5.2 servers

We hit a break when rolling out .Net 4.6 at Stack Overflow which I did a sanity check on here. This is the Array<T> optimization in play:

screen shot 2015-08-30 at 6 27 52 am

The optimization itself isn’t as important as the class it uses: Array.Empty<T>, which doesn’t exist in 4.5.2. This creates an overlapping set of issues of builds and runtime that resulted in a break for us. I first upgraded our canary servers, but not 100% correctly - the .Net 4.5.2 reference assemblies were not present. This resulted in the following layout:

Build server: .Net 4.6 (reference assemblies up through 4.5.1, but not 4.5.2) Project target: .Net 4.5.2 Runtime server: .Net 4.5.2

So what happened is that the build server did not find .Net 4.5.2 reference assemblies and happily resolved .Net 4.6 assemblies instead. The only hint of this is a build warning, not an error. (Aside: You could easily, IMO, argue that .Net 4.5.1 is a much safer fallback resolution here. Separately, you could argue the commonplace of these build warnings makes them weaker justification in breaks.) Since the compiled-against assembly is what Roslyn checks against in GetWellKnownTypeMember(), this mean that regardless of the targeted framework, a .Net 4.6+ optimization was used in the IL.

I think there’s another discussion to be had on forward vs. backwards resolution on a target failure (or having the default actually be an error, not a warning), but let’s scope this issue more to this specific problem. In discussion on the other issue, @FransBouma made what I think is an excellent point:
Why isn’t this a JIT optimization?

It makes much more sense here, at least on the surface, that a more well-informed JIT can make a better decision here given the types actually available at runtime. What was the reasoning for this being in roslyn instead of a runtime optimization? The net result currently is a runtime-only break as a result of this optimization, seen here:

System.MissingMethodException: Method not found: '!!0[] System.Array.Empty()'.
   at Calculon.Web.Scheduled.Tasks.ReportRedisInfoToBosunTask.<Execute>d__5.MoveNext()
   at System.Runtime.CompilerServices.AsyncMethodBuilderCore.Start[TStateMachine](TStateMachine& stateMachine)
   at Calculon.Web.Scheduled.Tasks.ReportRedisInfoToBosunTask.Execute(DateTime started)
   at Calculon.Web.Scheduled.ScheduledTasks.<RunScheduledTaskAsync>d__7.MoveNext()

To me, the runtime-only nature of the break makes it quite a bit more serious for people upgrading to .Net 4.6 and (quite literally) changing nothing else. What was the thinking here? Does JIT not make sense for other reasons that aren’t so apparent? If this was reasoned through and consciously decided, I’d love to see and read the discussions. I simply can’t find them, though I have searched.

About this issue

  • Original URL
  • State: closed
  • Created 9 years ago
  • Comments: 23 (15 by maintainers)

Commits related to this issue

Most upvoted comments

@mihailik

It looks like the Microsoft itself made the same mistake, and built Microsoft.Net.Compilers nuget package using wrong set of tools/on wrong server.

We made a different mistake here. The binaries contained in that NuGet package (csc.exe, vbc.exe, etc …) deliberately changed their minimum required runtime from 4.5.2 to 4.6 in the 2.0 release timeframe. There were a number of reasons for this, in particular we needed access to some threading primitives to help with performance issues in the workspace / IDE layers. The minimum runtime change though was deliberate.

The mistake we made was failing to note the new minimum runtime requirement in our app.config file. Hence when the binaries are run on 4.5.2 it ends up with errors like missing Array.Empty instead of printing a more helpful message about minimum runtime.

The primary tracking issue for this is #17908 and the PR to add the entry is #18018.

Oh and I suspect the reason why a JIT optimization isn’t a good idea is that you can’t just replace new T[0] with Array.Empty<T>(), code may rely on getting a new instance and might fail if it gets the cached one, see https://github.com/dotnet/corefx/issues/2363 for some more discussion.