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

Revert "Make solution options global" #59537

Merged
merged 1 commit into from
Feb 15, 2022

Conversation

RikkiGibson
Copy link
Contributor

@RikkiGibson RikkiGibson commented Feb 14, 2022

#59168 is suspected of introducing RPS regressions.

Last known good: https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/379782
First known bad: https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/379787
Revert validation: https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/379865

Regressed counters:

  • BuildInfoVS64.Test - RegressedBeyondThreshold / 0001.BuildInfo / Build_Ngen_InvalidAssemblyCount / Regressed: 3.00 Count (50.00%)
  • WebToolsVS64.Debugging - RegressedBeyondThreshold / 0300.Stop Debugging / VM_AdjustedImagesInMemory_Total_devenv / Regressed: 3.80 Count (0.75%)
  • WebToolsVS64.SolutionManagement - RegressedBeyondThreshold / 0100.Open Solution / IO_AdjustedFileReads_devenv / Regressed: 35.40 Count (9.41%)
  • WebToolsVS64.SolutionManagement - RegressedBeyondThreshold / 0100.Open Solution / RefSet_Image_Delta_devenv / Regressed: 5296128.00 Bytes (3.81%)
  • WebToolsVS64.SolutionManagement - RegressedBeyondThreshold / 0300.Close Solution / VM_AdjustedImagesInMemory_Total_devenv / Regressed: 4.00 Count (1.01%)
  • WebToolsVS64.TypingInRazorView - RegressedBeyondThreshold / 0100.Typing in Razor View / VM_AdjustedImagesInMemory_Total_devenv / Regressed: 4.60 Count (1.09%)

Please advise whether the failures are plausibly connected with this PR, and if so, whether we should revert, or take a follow-up PR to fix, or seek an exception. @genlu @tmat @dibarbet.

@genlu
Copy link
Member

genlu commented Feb 14, 2022

BuildInfoVS64.Test - RegressedBeyondThreshold / 0001.BuildInfo / Build_Ngen_InvalidAssemblyCount / Regressed: 3.00 Count (50.00%)

The ngen error is definitely caused by this change:

02/14/2022 10:44:44.177 [4060]: 1>Warning: System.TypeLoadException: Could not load type 'Microsoft.CodeAnalysis.ExternalAccess.Razor.IRazorDocumentExcerptService' from assembly 'Microsoft.CodeAnalysis.ExternalAccess.Razor, Version=4.2.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35'. while resolving 0x200000d - Microsoft.CodeAnalysis.Razor.DocumentExcerptServiceBase.

@genlu
Copy link
Member

genlu commented Feb 14, 2022

Those assembly load regression seem to be caused by ProjectSystem reporting erros. For example:

Name
 Image Load wer.dll (C:\Windows\System32\)
+ ntoskrnl!?
 + ntdll!NtMapViewOfSection
  + ntdll!LdrpMinimalMapModule
   + ntdll!LdrpMapDllWithSectionHandle
    + ntdll!LdrpMapDllNtFileName
     + ntdll!LdrpMapDllSearchPath
      + ntdll!LdrpProcessWork
       + ntdll!LdrpLoadDllInternal
        + ntdll!LdrpLoadDll
         + ntdll!LdrLoadDll
          + kernelbase!LoadLibraryExW
           + clr!CLRLoadLibraryExWorker
            + clr!CLRLoadLibraryEx
             + clr!LocalLoadLibraryHelper
              + clr!??NDirect::LoadLibraryModule
               + clr!NDirect::NDirectLink
                + clr!NDirect::GetStubForILStub
                 + clr!GetStubForInteropMethod
                  + clr!??MethodDesc::DoPrestub
                   + clr!PreStubWorker
                    + clr!ThePreStub
                     + microsoft.visualstudio.projectsystem.implementation.ni!HResult.Verify
                      + microsoft.visualstudio.projectsystem.implementation.ni!WatsonErrorReport.Prepare
                       + microsoft.visualstudio.projectsystem.implementation.ni!WatsonErrorReport.CreatePreparedNonFatalReport
                        + microsoft.visualstudio.projectsystem.implementation.ni!ProjectErrorReporting.SubmitErrorReport
                         + microsoft.visualstudio.projectsystem.implementation.ni!Microsoft.VisualStudio.ProjectSystem.ProjectErrorReporting+<>c__DisplayClass10_0.<SubmitErrorReport>b__1(System.Exception)
                          + guardmethodassembly!GuardMethodClass.GuardMethod(class System.Func`1,class System.Func`2,class System.Func`2)
                           + clr!ExceptionTracker::CallHandler
                            + clr!ExceptionTracker::CallFilterFunclet
                             + clr!ExceptionTracker::ProcessManagedCallFrame
                              + clr!ExceptionTracker::ProcessOSExceptionNotification
                               + clr!ProcessCLRException
                                + ntdll!RtlpExecuteHandlerForException
                                 + ntdll!RtlDispatchException
                                  + ntdll!RtlRaiseException
                                   + kernelbase!RaiseException
                                    + clr!RaiseTheExceptionInternalOnly
                                     + clr!IL_Throw
                                      + mscorlib.ni!ExceptionDispatchInfo.Throw
                                       + microsoft.visualstudio.projectsystem.ni!CommonProjectSystemTools.Rethrow
                                        + microsoft.visualstudio.projectsystem.implementation.ni!Microsoft.VisualStudio.ProjectSystem.ExceptionFilter+<>c__DisplayClass2_0.<Guard>g__Action|0()
                                         + guardmethodassembly!GuardMethodClass.GuardMethod(class System.Func`1,class System.Func`2,class System.Func`2)
                                          + microsoft.visualstudio.projectsystem.implementation.ni!Microsoft.VisualStudio.ProjectSystem.Designers.ProjectFaultHandlerService+<HandleErrorAsync>d__45.MoveNext()
                                           + microsoft.visualstudio.projectsystem.implementation.ni!System.Runtime.CompilerServices.AsyncTaskMethodBuilder.Start[Microsoft.VisualStudio.ProjectSystem.Designers.ProjectFaultHandlerService+<HandleErrorAsync>d__45](<HandleErrorAsync>d__45 ByRef)
                                            + microsoft.visualstudio.projectsystem.implementation.ni!ProjectFaultHandlerService.HandleErrorAsync
                                             + microsoft.visualstudio.projectsystem.managed.ni!FaultExtensions.ReportFaultAsync
                                              + microsoft.visualstudio.projectsystem.managed.ni!Microsoft.VisualStudio.ProjectSystem.FaultExtensions+<>c__DisplayClass6_0.<RegisterFaultHandlerAsync>b__0(System.Threading.Tasks.Task)

And by checking the exceptions thrown in the trace around the time of the call stack above, it's also caused by the razor ExternalAccess change:

image

@tmat
Copy link
Member

tmat commented Feb 14, 2022

We should instead insert new Razor build. I thought it's already inserted but apparently not.

Copy link
Member

@tmat tmat left a comment

Choose a reason for hiding this comment

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

Let's make sure Razor is inserted instead of reverting this.

@tmat
Copy link
Member

tmat commented Feb 14, 2022

Razor insertion is now in progress.

@RikkiGibson
Copy link
Contributor Author

Could you please include a link to it?

@dibarbet
Copy link
Member

Could you please include a link to it?

@RikkiGibson this is the release that is generating the insertion, unfortunately it doesn't have your improvements and it has to clone VS so it may take a few :)
https://dev.azure.com/dnceng/internal/_releaseProgress?releaseId=40624&_a=release-environment-logs&environmentId=121442

@dibarbet
Copy link
Member

@RikkiGibson
Copy link
Contributor Author

We're now dual inserting in https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/379897

@RikkiGibson
Copy link
Contributor Author

RikkiGibson commented Feb 15, 2022

We're now dual inserting in https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/379897

RPS results are in for this. Unfortunately, we're still regressing a counter:

  • BuildInfoVS64.Test - RegressedBeyondThreshold / 0001.BuildInfo / Build_Ngen_InvalidAssemblyCount / Regressed: 1.00 Count (16.67%)

It appears the same counter is regressed in the insertion containing only aspnetcore changes. https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/379899

cc @genlu @tmat

The revert validation does have clean RPS results. If I don't hear from you guys tonight with a sure-fire "change X and the assembly count will be fixed, guaranteed", or "we need an exception for the new assembly, guaranteed", I'm going to merge this revert, so that we can hopefully have a healthy insertion in the morning.

It's very important that our release branches stay insertable, because if more PRs get merged that break RPS while this is also going on, it will be even more difficult to triage the problems and get un-stuck. Please ensure that if your change depends on a recent change in another component, that you verify that the other component is up to date before merging to Roslyn.

@tmat
Copy link
Member

tmat commented Feb 15, 2022

@NTaylorMullen There seems to be version mismatch with Razor assemblies. Missing binding redirect?

02/14/2022 23:51:57.434 [5464]: Failed to load dependency Microsoft.AspNetCore.Razor.Language of assembly Microsoft.WebTools.Languages.Razor.Core, Version=17.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a because of the following error : The located assembly's manifest definition does not match the assembly reference. (Exception from HRESULT: 0x80131040)
02/14/2022 23:51:57.637 [5464]: 1>    Compiling assembly C:\VisualStudio\Common7\IDE\Extensions\Microsoft\Web Tools\Languages\Razor\v4.0\Microsoft.WebTools.Languages.Razor.Core.dll (CLR v4.0.30319) ...
02/14/2022 23:51:57.934 [5464]: 3>    Compiling assembly C:\VisualStudio\Common7\IDE\Extensions\ukdqr4lk.ss4\MSTestv2IntelliTestExtension.dll (CLR v4.0.30319) ...
02/14/2022 23:51:58.028 [5464]: 1>Warning: System.IO.FileLoadException: Could not load file or assembly 'Microsoft.AspNetCore.Razor.Language, Version=6.0.2.0, Culture=neutral, PublicKeyToken=adb9793829ddae60' or one of its dependencies. The located assembly's manifest definition does not match the assembly reference. (Exception from HRESULT: 0x80131040) ---> System.IO.FileLoadException: Could not load file or assembly 'Microsoft.AspNetCore.Razor.Language, Version=6.0.0.0, Culture=neutral, PublicKeyToken=adb9793829ddae60' or one of its dependencies. The located assembly's manifest definition does not match the assembly reference. (Exception from HRESULT: 0x80131040)
   --- End of inner exception stack trace ---02/14/2022 23:51:58.028 [5464]: 1> while resolving 0x1000031 - Microsoft.AspNetCore.Razor.Language.RazorCodeDocument.

@tmat
Copy link
Member

tmat commented Feb 15, 2022

We should insert Razor separately and make sure it's good.

@RikkiGibson RikkiGibson merged commit 4de93ea into main Feb 15, 2022
@ghost ghost added this to the Next milestone Feb 15, 2022
@RikkiGibson RikkiGibson deleted the revert-59168-MakeSolutionOptionsGlobal branch February 15, 2022 05:20
333fred added a commit that referenced this pull request Feb 15, 2022
…ures/required-members

* upstream/main: (1291 commits)
  Revert "Make solution options global (#59168)" (#59537)
  Update src/VisualStudio/CSharp/Test/F1Help/F1HelpTests.cs
  Update src/VisualStudio/CSharp/Impl/LanguageService/CSharpHelpContextService.cs
  Update Language Feature Status.md (#59546)
  Catch more exceptions (#59526)
  Fix Peek Definition for Razor files (#59528)
  Update OneOffInsertion.ps1 path in 17.0 branch (#59541)
  Use GetBestTypeByMetadataName in 'use System.HashCode' fixer
  Pass exception into internal error diagnostic (#59443)
  Provide f1 help for discards
  Update src/VisualStudio/Core/Def/SymbolSearch/AbstractDelayStartedService.cs
  Fix formatting
  move line
  Simplify
  Simplify
  Fixup
  Explicitly perform some mef loads in teh background
  Bump Microsoft.DiaSymReader version for release
  Remove IBoundSwitchStatement (#59516)
  Make solution options global (#59168)
  ...
tmat added a commit that referenced this pull request Feb 15, 2022
tmat added a commit that referenced this pull request Feb 15, 2022
@RikkiGibson RikkiGibson modified the milestones: Next, 17.2.P2 Mar 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants