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

Add binding redirects for System.ValueTuple and System.Xml.ReaderWriter #21832

Merged
merged 2 commits into from
Aug 31, 2017

Conversation

jcouv
Copy link
Member

@jcouv jcouv commented Aug 30, 2017

This is a mitigation for https://devdiv.visualstudio.com/DevDiv/_workitems/edit/467584

The issue is that when you install 4.7.1 after installing VS, the binaries that depend on "any facade that we have either introduced or that we have changed in 4.7.1" should be ngen'ed again, but they are not.
The impact is that the compiler (VBCSCompiler.exe, csc.exe, etc) is JIT'ed on every run, resulting is significant performance degradation (50%) which RPS caught.

This PR for 15.3 preview 2 adds binding redirects (without changing the version of the assemblies selected) as a mitigation. We verified those manually.

The binding redirects are shown below. They get added to 5 core binaries (csi, vbi, csc, vbc, VBCSCompiler) and also InteractiveHost.

    <assemblyBinding xmlns="urn:schemas-microsoft-com:asm.v1">
      <dependentAssembly>
        <assemblyIdentity name="System.ValueTuple" publicKeyToken="cc7b13ffcd2ddd51" culture="neutral" />
        <bindingRedirect oldVersion="0.0.0.0-4.0.1.0" newVersion="4.0.1.0" />
      </dependentAssembly>
    </assemblyBinding>
    <assemblyBinding xmlns="urn:schemas-microsoft-com:asm.v1">
      <dependentAssembly>
        <assemblyIdentity name="System.Xml.ReaderWriter" publicKeyToken="b03f5f7f11d50a3a" culture="neutral" />
        <bindingRedirect oldVersion="0.0.0.0-4.1.0.0" newVersion="4.1.0.0" />
      </dependentAssembly>
    </assemblyBinding>

@jcouv jcouv added this to the 15.3 milestone Aug 30, 2017
@jcouv jcouv self-assigned this Aug 30, 2017
@jcouv jcouv changed the base branch from master to dev15.4.x August 30, 2017 23:09
@jcouv
Copy link
Member Author

jcouv commented Aug 30, 2017

@jaredpar @tmat @dotnet/roslyn-compiler for review.
FYI @AlexGhiondea @natidea

@tmat
Copy link
Member

tmat commented Aug 30, 2017

👍

@jcouv
Copy link
Member Author

jcouv commented Aug 31, 2017

Test windows_debug_vs-integration_prtest please

@jcouv
Copy link
Member Author

jcouv commented Aug 31, 2017

@agocke Can you also take a look?

Copy link
Member

@agocke agocke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jcouv
Copy link
Member Author

jcouv commented Aug 31, 2017

Merging per thread with @natidea
Thanks

@jcouv jcouv merged commit eb315c9 into dotnet:dev15.4.x Aug 31, 2017
@jcouv jcouv deleted the binding-redirects branch August 31, 2017 20:20
@jcouv
Copy link
Member Author

jcouv commented Sep 18, 2017

Got some update from AlexG. It turns out the root cause is different than we thought. It was not an ngen problem per-se, but a problem with the config produced by VS which drives the ngen'ing process. That config differed from that of the compiler, so the compiler was not running the ngen'ed image.
The fix is still correct though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants