Use a pooled StringBuilder in Dependency.GetID #3129

Pilchie · 2018-01-11T18:24:53Z

Traces showed this taking upward of 1% of allocations of opening a solution.

**Customer scenario**

What does the customer do to get into this situation, and why do we think this
is common enough to address in Escrow. (Granted, sometimes this will be
obvious "Open project, VS crashes" but in general, I need to understand how
common a scenario is)

Bugs this fixes:

(either VSO or GitHub links)

Workarounds, if any

Also, why we think they are insufficient for RC vs. RC2, RC3, or RTW

Risk

This is generally a measure our how central the affected code is to adjacent
scenarios and thus how likely your fix is to destabilize a broader area of code

Performance impact

(with a brief justification for that assessment (e.g. "Low perf impact because no extra allocations/no complexity changes" vs. "Low")

Is this a regression from a previous update?

Root cause analysis:

How did we miss it? What tests are we adding to guard against it in the future?

How was the bug found?

(E.g. customer reported it vs. ad hoc testing)

Pilchie · 2018-01-11T18:25:51Z

Tagging @lifengl for review.

Also @tmat - is there a way to re-use Roslyn's object pooling stuff instead of making my own?

tmat · 2018-01-11T18:30:36Z

Certainly. Add a package reference to Microsoft.CodeAnalysis.PooledObjects.

tmat · 2018-01-11T18:35:08Z

You might also want to add a workaround for NuGet/Home#4856 to the project that will reference the source package:

<ItemGroup>
    <Compile Update="@(Compile)">
      <Link Condition="'%(NuGetPackageId)' != ''">%(NuGetPackageId)\%(Link)</Link>
    </Compile>
</ItemGroup>

Pilchie · 2018-01-11T18:36:31Z

@tmat is it only on myget?

tmat · 2018-01-11T18:36:56Z

Yes.

Pilchie · 2018-01-11T18:57:06Z

Updated to use Microsoft.CodeAnalysis.PooledObjects

jmarolf · 2018-01-11T19:58:26Z

...VisualStudio.ProjectSystem.Managed.VS/Microsoft.VisualStudio.ProjectSystem.Managed.VS.csproj

@@ -20,6 +20,7 @@

    <PackageReference Include="NuGet.SolutionRestoreManager.Interop" Version="$(NuGetSolutionRestoreManagerInteropVersion)" />
    <PackageReference Include="NuGet.VisualStudio" Version="$(NuGetVisualStudioVersion)" />
+    <PackageReference Include="Microsoft.CodeAnalysis.PooledObjects" Version="$(MicrosoftCodeAnalysisPooledObjectsVersion)" />


@tmat does this get pulled into the vsix automatically via repotoolset?

It's a source package - the sources are included into the project. No new binary dependencies are added.

tmat · 2018-01-11T21:38:36Z

build/Versions.props

@@ -67,6 +67,7 @@
    <!-- Roslyn -->
    <MicrosoftVisualStudioLanguageServicesVersion>2.6.0-beta1-62113-02</MicrosoftVisualStudioLanguageServicesVersion>
    <MicrosoftVisualStudioIntegrationTestUtilitiesVersion>2.6.0-beta1-62113-02</MicrosoftVisualStudioIntegrationTestUtilitiesVersion>
+    <MicrosoftCodeAnalysisPooledObjectsVersion>2.6.0-beta1-62113-02</MicrosoftCodeAnalysisPooledObjectsVersion>


Could you use a newer version? This version does not have source link support, which might result in build failures.
2.7.0-beta3-62506-01 should be good (even newer ones require some toolset update that's not ready yet).

I'll have a new RepoToolset built in a moment that supports the latest compiler source packages.

Yeah, I was just trying to match the version of the rest of the Roslyn packages, but I can update this one with a comment about why it is different. So I should pick the latest 2.7 package, right?

Updated, but I'm seeing a new error on my work machine that I wasn't seeing at home:

ProjectSystem\VS\Tree\Dependencies\Snapshot\Dependency.cs(337,23): error CS0122: 'PooledStringBuilder' is inaccessible due to its protection level [C:\Code\project-system\src\Microsoft.VisualStudio.ProjectSystem.Managed.VS\mvsokklc.tmp_proj]

Looks like the xaml tmp_proj isn't getting the files added to it. Any ideas?

Odd. Not sure how much the XAML project differs from regular project. My guess would be that nuget can't determine what language it is from the extension and thus doesn't know what language to pick from the source package content types. Perhaps forcing <Language>C#</Language> in Directory.Build.props file next to the project might work?

@rrelyea or @jaredpar - do you have any advice here?

Pilchie · 2018-01-12T17:14:18Z

Closing and re-opening to get CI to run.

abpiskunov · 2018-01-12T17:43:27Z

FYI, i just ran a small experiment https://gist.github.com/abpiskunov/ba0900a9b38eba27c33e26b6d48d6b85 and string.Join clearly wins vs StringBuilder for small number of strings....

abpiskunov · 2018-01-13T16:20:54Z

Actually never mind, i did not do restart on stopwatch :( and had intervals instead of separate time chunks. StringBuilder is still better than Join, though they are close in case of small sets of strings.

Pilchie · 2018-01-17T00:21:49Z

Bumping to a top level comment. I can't build this because I get:

ProjectSystem\VS\Tree\Dependencies\Snapshot\Dependency.cs(337,23): error CS0122: 'PooledStringBuilder' is inaccessible
due to its protection level [C:\Code\project-system\src\Microsoft.VisualStudio.ProjectSystem.Managed.VS\pzb2kyvz.tmp_pr
oj]

Note the .tmp_proj. Any idea how to fix this? @rrelyea @jaredpar ? @tmat's suggestion of a Directory.Build.props with <Language>C#</Language> didn't help.

Pilchie · 2018-01-17T00:30:46Z

Also @nguerrera and also @onovotny, since I heard Sdk.Extras might have had to deal with this.

clairernovotny · 2018-01-17T00:34:12Z

We have nasty code like this:

https://github.com/onovotny/MSBuildSdkExtras/blob/c8e2a713d6ea0c80273a12900248202c11489159/MSBuild.Sdk.Extras/build/MSBuild.Sdk.Extras.props#L9-L32

clairernovotny · 2018-01-17T00:39:25Z

@Pilchie This may help. There's a workaround in there:
NuGet/Home#5894 (comment)

Pilchie · 2018-01-17T18:29:09Z

Okay, pushed a couple more commits. Please take another look.

jaredpar · 2018-01-17T18:31:59Z

build/Versions.props

@@ -70,6 +70,9 @@
    <RoslynIntegrationVsixVersion>2.6.0.6211302</RoslynIntegrationVsixVersion>
    <MicrosoftNetRoslynDiagnosticsVersion>2.6.0-beta1-62322-01</MicrosoftNetRoslynDiagnosticsVersion>

+    <!-- From roslyn, but source package, so okay for version to mismatch the above -->
+    <MicrosoftCodeAnalysisPooledObjectsVersion>	2.7.0-beta3-62511-04</MicrosoftCodeAnalysisPooledObjectsVersion>


nit: 2 blank spaces here in value.

Fixed. Actually a TAB!!!

jaredpar · 2018-01-17T18:34:04Z

src/Microsoft.VisualStudio.ProjectSystem.Managed/Text/StringBuilderExtensions.cs

+                        break;
+                    }
+
+                    return builder;


This return seems wrong. It unconditionally returns from function the first time a value in trimChars is not the end of the build.r

Yes, those are the semantics of TrimEnd - remove characters from the end of the string that are one of trimChars.

Consider "hello".TrimEnd("zo"). That produces "hell". Your change though will produce "hello" because you unconditionally return after z doesn't match o.

Yep. Thanks, I thought I had that, but clearly not.

Added an explicit check for a match.

Traces showed this taking upward of 1% of allocations of opening a solution. Fixes dotnet#2918.

Pilchie · 2018-01-17T18:59:32Z

And fixed my broken rebase :(

Pilchie · 2018-01-17T20:55:52Z

@jaredpar @tmat - does repo toolset control compiler version?

vbc : error BC2014: the value '15.5' is invalid for option 'langversion' [D:\j\workspace\windows_relea---15bd572b\src\Microsoft.VisualStudio.Editors\vtztzpdf.tmp_proj]

Pilchie · 2018-01-17T20:56:32Z

Or, that being the tmp_proj again, are we not using the toolset compiler for it?

jaredpar · 2018-01-17T20:57:28Z

@Pilchie that error is on the WPF temp project file and you're setting the VB language version in a section for VB only. Guessing it's that issue.

Pilchie · 2018-01-17T21:03:12Z

"that issue" is usually that the project doesn't know what language it is, but in this case, it knew to apply the VB setting, so instead it sounds like it's failing to apply the toolset compiler. Any idea how to workaround? In the meantime, I think I just won't specify langver for VB.

jaredpar · 2018-01-17T21:17:05Z

@Pilchie ah I see. That error does'nt make a lot of sense. Will wait for @tmat to comment on how the compiler toolset is chosen. That is likely the issue here.

Pilchie · 2018-01-17T22:00:51Z

Now I seem to be getting a compiler crash in C# :(

 Unhandled Exception: System.ArgumentException: Illegal characters in path.
    at System.IO.Path.CheckInvalidPathChars(String path, Boolean checkAdditional)
    at System.IO.Path.GetFileName(String path)
    at Microsoft.CodeAnalysis.Compilation.SerializeToPeStream(CommonPEModuleBuilder moduleBeingBuilt, EmitStreamProvider peStreamProvider, EmitStreamProvider metadataPEStreamProvider, EmitStreamProvider pdbStreamProvider, Func`1 testSymWriterFactory, DiagnosticBag diagnostics, Boolean metadataOnly, Boolean includePrivateMembers, Boolean emitTestCoverageData, String pePdbFilePath, CancellationToken cancellationToken)
    at Microsoft.CodeAnalysis.CommonCompiler.RunCore(TextWriter consoleOutput, ErrorLogger errorLogger, CancellationToken cancellationToken)
    at Microsoft.CodeAnalysis.CommonCompiler.Run(TextWriter consoleOutput, CancellationToken cancellationToken)
    at Microsoft.CodeAnalysis.CSharp.CommandLine.Csc.<>c__DisplayClass1_0.<Run>b__0(TextWriter tw)
    at Microsoft.CodeAnalysis.CommandLine.ConsoleUtil.RunWithUtf8Output[T](Func`2 func)
    at Microsoft.CodeAnalysis.CSharp.CommandLine.Csc.Run(String[] args, BuildPaths buildPaths, TextWriter textWriter, IAnalyzerAssemblyLoader analyzerLoader)
    at Microsoft.CodeAnalysis.CommandLine.DesktopBuildClient.RunLocalCompilation(String[] arguments, BuildPaths buildPaths, TextWriter textWriter)
    at Microsoft.CodeAnalysis.CommandLine.BuildClient.RunCompilation(IEnumerable`1 originalArguments, BuildPaths buildPaths, TextWriter textWriter)
    at Microsoft.CodeAnalysis.CommandLine.DesktopBuildClient.Run(IEnumerable`1 arguments, RequestLanguage language, CompileFunc compileFunc, IAnalyzerAssemblyLoader analyzerAssemblyLoader)
    at Microsoft.CodeAnalysis.CSharp.CommandLine.Program.Main(String[] args)

tmat · 2018-01-17T22:24:28Z

Re compiler versions used:
The default version is currently 2.7.0-beta2-62330-02: https://github.com/dotnet/roslyn-tools/blob/master/sdks/RepoToolset/tools/DefaultVersions.props#L29
Repo can override it by specifying the value of <MicrosoftNetCompilersVersion> property in Versions.props file.

As for the compiler crash ... looking into the log now: https://ci.dot.net/job/dotnet_project-system/job/dev15.6.x/job/windows_debug_prtest/12/artifact/artifacts/debug/log/ ...

tmat · 2018-01-17T22:32:39Z

I see no obvious issue :(

Pilchie · 2018-01-17T22:45:13Z

We appear to set MicrosoftNetCompilersVersion to 2.6.0-beta3-62310-01 here.

Maybe worth updating?

tmat · 2018-01-17T22:54:15Z

@Pilchie Definitely. I'd just remove it so that it defaults to the one defined in RepoToolset.

Pilchie · 2018-01-17T23:29:42Z

No joy:

C:\Program Files (x86)\Microsoft Visual Studio\Preview\Enterprise\MSBuild\15.0\Bin\Roslyn\Microsoft.
VisualBasic.Core.targets(87,10): error MSB4064: The "EmbedAllSources" parameter is not supported by
the "Vbc" task. Verify the parameter exists on the task, and it is a settable public instance proper
ty. [C:\Code\project-system\src\Microsoft.VisualStudio.Editors\aaodp2bk.tmp_proj]
C:\Program Files (x86)\Microsoft Visual Studio\Preview\Enterprise\MSBuild\15.0\Bin\Roslyn\Microsoft.
VisualBasic.Core.targets(73,5): error MSB4063: The "Vbc" task could not be initialized with its inpu
t parameters.  [C:\Code\project-system\src\Microsoft.VisualStudio.Editors\aaodp2bk.tmp_proj]

Pilchie · 2018-01-22T23:52:36Z

Reverted to just using a local pool for 15.6. @tmat @jaredpar any objectsions?

jaredpar · 2018-01-22T23:53:06Z

SGTM

Pilchie requested review from davkean, abpiskunov, natidea and a team January 11, 2018 18:24

jmarolf reviewed Jan 11, 2018

View reviewed changes

tmat reviewed Jan 11, 2018

View reviewed changes

Pilchie closed this Jan 12, 2018

Pilchie reopened this Jan 12, 2018

jmarolf approved these changes Jan 17, 2018

View reviewed changes

Pilchie force-pushed the Fix2918-Dependency.GetID branch 2 times, most recently from 41e54db to 2b73f3b Compare January 17, 2018 18:28

jaredpar reviewed Jan 17, 2018

View reviewed changes

Pilchie force-pushed the Fix2918-Dependency.GetID branch from 2b73f3b to d70ecd7 Compare January 17, 2018 18:54

Use a pooled StringBuilder in Dependency.GetID

4666ca2

Traces showed this taking upward of 1% of allocations of opening a solution. Fixes dotnet#2918.

Pilchie force-pushed the Fix2918-Dependency.GetID branch from d70ecd7 to 59961f4 Compare January 17, 2018 18:59

jaredpar approved these changes Jan 17, 2018

View reviewed changes

Pilchie force-pushed the Fix2918-Dependency.GetID branch from 94e4ec1 to 4666ca2 Compare January 22, 2018 23:52

Fix pooling

4587102

Pilchie merged commit 96e1792 into dotnet:dev15.6.x Jan 23, 2018

Pilchie deleted the Fix2918-Dependency.GetID branch January 23, 2018 00:28

Pilchie mentioned this pull request Jan 23, 2018

GC pressure Dependency.GetID #2918

Closed

Use a pooled StringBuilder in Dependency.GetID #3129

Use a pooled StringBuilder in Dependency.GetID #3129

Conversation

Pilchie commented Jan 11, 2018

Pilchie commented Jan 11, 2018

tmat commented Jan 11, 2018

tmat commented Jan 11, 2018

Pilchie commented Jan 11, 2018

tmat commented Jan 11, 2018

Pilchie commented Jan 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Pilchie commented Jan 12, 2018

abpiskunov commented Jan 12, 2018

abpiskunov commented Jan 13, 2018

Pilchie commented Jan 17, 2018

Pilchie commented Jan 17, 2018

clairernovotny commented Jan 17, 2018 • edited Loading

clairernovotny commented Jan 17, 2018

Pilchie commented Jan 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaredpar Jan 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Pilchie commented Jan 17, 2018

Pilchie commented Jan 17, 2018

Pilchie commented Jan 17, 2018

jaredpar commented Jan 17, 2018

Pilchie commented Jan 17, 2018

jaredpar commented Jan 17, 2018

Pilchie commented Jan 17, 2018

tmat commented Jan 17, 2018

tmat commented Jan 17, 2018

Pilchie commented Jan 17, 2018

tmat commented Jan 17, 2018

Pilchie commented Jan 17, 2018

Pilchie commented Jan 22, 2018

jaredpar commented Jan 22, 2018

clairernovotny commented Jan 17, 2018 •

edited

Loading

Pilchie commented Jan 17, 2018 •

edited

Loading

jaredpar Jan 17, 2018 •

edited

Loading