Use ValueTask<T> on allocation hot paths by sharwell · Pull Request #23557 · dotnet/roslyn

sharwell · 2017-12-04T15:07:07Z

AnalyzerRunner indicated that these asynchronous paths typically complete synchronously. Measured savings are ~2.5% (~2GiB).

Ask Mode template not completed

Customer scenario

What does the customer do to get into this situation, and why do we think this
is common enough to address for this release. (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)

Test documentation updated?

If this is a new non-compiler feature or a significant improvement to an existing feature, update https://github.com/dotnet/roslyn/wiki/Manual-Testing once you know which release it is targeting.

jasonmalinowski

Might need more changes and confirmation about RPS.

jasonmalinowski · 2017-12-04T18:27:28Z

src/Compilers/Core/Portable/CodeAnalysis.csproj

Does this need updates to the VisualStudioSetup.Dependencies package? Will it ship a new façade that we need to be ready for in RPS?

(VS integration test failures imply yes, this needs more façade updates.)

... and binding redirects.

sharwell · 2017-12-04T18:30:08Z

@jasonmalinowski I meant to mark this one as a work in progress, to be revisited after merging the other allocation-reducing changes that don't require changes to dependencies.

jasonmalinowski · 2017-12-04T18:31:24Z

@sharwell Makes sense. No problem with this going in assuming the paperwork is done though. It'll be good to start getting ValueTask in here.

CyrusNajmabadi · 2017-12-04T21:08:11Z

How are you figuring out which paths to ValueTaskisfy? It seems like thsi would be useful across a lot of the LS. But it's unclear how to drive that, and my concern would be regressing some path that actually needs real async.

sharwell · 2017-12-04T21:10:08Z

How are you figuring out which paths to ValueTaskisfy?

In this case, I'm using AnalyzerRunner and looking for Task<T> allocations where the async state machine method SetResult is called by Start.

sharwell · 2017-12-08T03:39:54Z

@jasonmalinowski My original evaluation of allocation savings understated the value of this pull request by a factor of at least 4.

jaredpar · 2017-12-08T21:28:03Z

We need to make sure that we discuss the RPS implications of this with @MattGertz. This is a new library reference and very likely to be loading in our measured scenarios.

sharwell · 2017-12-08T21:42:21Z

@jaredpar Yes. I plan to update this pull request as issues come to light until it gets to a point that we can claim a justifiable win. However, I'm having all sorts of problems just getting the tests to pass with it.

Adds System.Threading.Tasks.Extensions as a dependency.

…mplete synchronously

sharwell · 2018-03-01T14:30:13Z

Based on the size of this added library (26KiB), I think we have reached a point where the wins may justify the inclusion.

/cc @jinujoseph @jasonmalinowski @jaredpar

jaredpar

Need to understand the binding redirect problem CPS hit better before we can merge this.

jaredpar · 2018-03-01T17:39:40Z

@tmat the other component to updating dependencies is getting permission from DevDiv. In this case @sharwell is trying to trade off memory savings for a new image reference. Need to clear that permission hurdle for Microsoft.DiaSymReader as well.

tmat · 2018-03-01T17:45:34Z

@jaredpar I understand. Just want to use this opportunity to reduce setup authoring work, if possible.

Pilchie · 2018-03-01T18:38:54Z

I sent some email offline.

jasonmalinowski

My signoff remains modulo whatever CLR problems exist which I'm not looped in on.

Do make sure when this merges that we've synced to be ready for the coming insertion -- these insertions often require extra steps; the last time we got surprised by one we caused a lot of chaos with delayed divisional branch snaps.

jasonmalinowski · 2018-03-03T01:32:36Z

src/Workspaces/Core/Portable/Workspace/Solution/Document.cs


            // do it async for real.
-            return DocumentState.GetSyntaxTreeAsync(cancellationToken);
+            return DocumentState.GetSyntaxTreeAsync(cancellationToken).AsTask();


You should probably update src/NuGet/Microsoft.CodeAnalysis.Workspaces.Common.nuspec to state the dependency too. It's "fine" for now in that workspaces depends on common, and it depends on extensions, but if ever somebody "fixes" the core one to not depend on it, this is now broken.

sharwell · 2018-03-03T01:37:39Z

@jasonmalinowski The problems appeared to involve ValueTuple, not ValueTask. We are already using ValueTuple without problems so whatever happened with the other project doesn't seem to be impacting us.

sharwell · 2018-04-19T13:28:00Z

@jaredpar Any remaining issues from your side?

jaredpar · 2018-04-19T16:21:45Z

@sharwell sorry forgot to remove my hold once we realized the issue was ValueTuple not ValueTask. I'm fine with this assuming that we get RPS buy off here as this will add a DLL load to some scenarios.

I'm less concerned about the binding redirects / dependency flow as well. Now that we use our own NuGet packages for bootstrapping you pretty much have to get that all right to pass Jenkins 😄

sharwell · 2018-04-20T11:20:17Z

@jaredpar I can't figure this one out:
https://ci.dot.net/job/dotnet_roslyn/job/master/job/microbuild_prtest/13086/

D:\j\workspace\microbuild_pr---d013602c\Binaries\Tools\dotnet\sdk\2.1.300-preview2-008324\Sdks\NuGet.Build.Tasks.Pack\build\NuGet.Build.Tasks.Pack.targets(198,5): error : '' is not a valid version string. [D:\j\workspace\microbuild_pr---d013602c\src\Nuget\NuGetProjectPackUtil.csproj]
20:11:53 Command failed to execute: D:\j\workspace\microbuild_pr---d013602c\Binaries\Tools\dotnet\dotnet.exe pack -nologo --no-build D:\j\workspace\microbuild_pr---d013602c\src\Nuget\NuGetProjectPackUtil.csproj /p:EmptyDir=D:\j\workspace\microbuild_pr---d013602c\Binaries\Temp\v1hq0dcn.gp5 /p:NugetPackageKind=PreRelease /p:NuspecFile=D:\j\workspace\microbuild_pr---d013602c\src\Nuget\Microsoft.CodeAnalysis.Common.nuspec /p:NuspecBasePath=D:\j\workspace\microbuild_pr---d013602c\Binaries\Release -o D:\j\workspace\microbuild_pr---d013602c\Binaries\Release\NuGet\PreRelease
20:11:53 System.Management.Automation.RuntimeException: Command failed to execute: D:\j\workspace\microbuild_pr---d013602c\Binaries\Tools\dotnet\dotnet.exe pack -nologo --no-build D:\j\workspace\microbuild_pr---d013602c\src\Nuget\NuGetProjectPackUtil.csproj /p:EmptyDir=D:\j\workspace\microbuild_pr---d013602c\Binaries\Temp\v1hq0dcn.gp5 /p:NugetPackageKind=PreRelease /p:NuspecFile=D:\j\workspace\microbuild_pr---d013602c\src\Nuget\Microsoft.CodeAnalysis.Common.nuspec /p:NuspecBasePath=D:\j\workspace\microbuild_pr---d013602c\Binaries\Release -o D:\j\workspace\microbuild_pr---d013602c\Binaries\Release\NuGet\PreRelease

src/Compilers/Core/Portable/CodeAnalysis.csproj

    <PackageReference Include="System.Collections.Immutable" Version="$(SystemCollectionsImmutableVersion)" />
    <PackageReference Include="System.Reflection.Metadata" Version="$(SystemReflectionMetadataVersion)" />
    <PackageReference Include="System.Text.Encoding.CodePages" Version="$(SystemTextEncodingCodePagesVersion)" />
+    <PackageReference Include="System.Threading.Tasks.Extensions" Version="$(SystemThreadingTasksExtensionsVersion)" />


Now that System.Threading.Tasks.Extensions is referenced, the documentation can contain direct links to ValueTask<TResult>.

jaredpar · 2018-04-20T18:04:45Z

@sharwell need to add the version string for your new package here

https://github.com/dotnet/roslyn/blob/master/src/NuGet/NuGetProjectPackUtil.csproj#L93

The error message is definitely not helpful. I ran into it frequently when working with dotnet pack. I've filed the following issue on NuGet to make this clearer. For instance at least tell me the line number where the version string was wrong.

NuGet/Home#6806

sharwell · 2018-04-20T20:45:26Z

@Pilchie for approval. This change trades a one-time 28KiB assembly load for the savings described at the top, and allows us to make similar changes moving forward.

Pilchie · 2018-04-20T21:09:49Z

I think this is probably worthwhile, but it will likely still need an exception. Let's wait for Monday, and loop in @MattGertz about the right time to take it (since merging now might block insertions of other changes to master).

sharwell requested review from a team as code owners December 4, 2017 15:07

jasonmalinowski requested changes Dec 4, 2017

View reviewed changes

sharwell changed the title ~~Use ValueTask<T> on allocation hot paths~~ [WIP] Use ValueTask<T> on allocation hot paths Dec 4, 2017

sharwell added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Dec 4, 2017

sharwell mentioned this pull request Dec 4, 2017

Use TryDequeue before falling back to DequeueAsync #23553

Merged

sharwell force-pushed the use-valuetask branch from 6535a16 to 5ee05ef Compare December 5, 2017 13:47

sharwell mentioned this pull request Dec 5, 2017

Reduce async overhead in CodeAnalysis #23544

Merged

sharwell force-pushed the use-valuetask branch from 5ee05ef to 1aa0c44 Compare December 6, 2017 13:36

sharwell removed the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Dec 7, 2017

sharwell requested review from a team December 8, 2017 17:01

sharwell force-pushed the use-valuetask branch from 1aa0c44 to 255b601 Compare December 8, 2017 18:38

jasonmalinowski approved these changes Dec 8, 2017

View reviewed changes

jaredpar approved these changes Dec 8, 2017

View reviewed changes

sharwell force-pushed the use-valuetask branch 2 times, most recently from c07e78a to 7f4106c Compare December 11, 2017 16:10

sharwell added 2 commits December 11, 2017 11:27

Support using ValueTask<T> in the compiler

3a852e0

Adds System.Threading.Tasks.Extensions as a dependency.

Use ValueTask<T> on allocation hot paths where operations normally co…

fce2c4a

…mplete synchronously

sharwell force-pushed the use-valuetask branch from 7f4106c to fce2c4a Compare December 11, 2017 17:27

sharwell changed the title ~~[WIP] Use ValueTask<T> on allocation hot paths~~ Use ValueTask<T> on allocation hot paths Dec 11, 2017

jinujoseph added this to the 15.7 milestone Jan 30, 2018

jaredpar requested changes Mar 1, 2018

View reviewed changes

jasonmalinowski approved these changes Mar 3, 2018

View reviewed changes

marksmeltzer mentioned this pull request Mar 3, 2018

Use a cached delegate for method group conversion #5835

Closed

sharwell mentioned this pull request Mar 30, 2018

use ValueTask in Roslyn #25843

Closed

jinujoseph modified the milestones: 15.7, Unknown Apr 15, 2018

jaredpar approved these changes Apr 19, 2018

View reviewed changes

jcouv added the Area-IDE label Apr 19, 2018

jinujoseph modified the milestones: Unknown, 15.8 Apr 19, 2018

sharwell changed the base branch from dev15.7.x to master April 19, 2018 18:44

Merge remote-tracking branch 'dotnet/master' into use-valuetask

7db8cea

khellang reviewed Apr 20, 2018

View reviewed changes

Update PerformanceSensitiveAttribute documentation

247bf58

Now that System.Threading.Tasks.Extensions is referenced, the documentation can contain direct links to ValueTask<TResult>.

Merge remote-tracking branch 'dotnet/master' into use-valuetask

bec2262

jinujoseph added the Approved to merge label Apr 26, 2018

sharwell merged commit 7786ba4 into dotnet:master Apr 26, 2018

sharwell deleted the use-valuetask branch April 26, 2018 21:17

This was referenced May 8, 2018

Need to update our Tasks dependency #26709

Closed

Fix our Compiler NuGet dependency list #26762

Merged

Conversation

sharwell commented Dec 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Customer scenario

Bugs this fixes

Workarounds, if any

Risk

Performance impact

Is this a regression from a previous update?

Root cause analysis

How was the bug found?

Test documentation updated?

Uh oh!

jasonmalinowski left a comment

Choose a reason for hiding this comment

Uh oh!

jasonmalinowski Dec 4, 2017

Choose a reason for hiding this comment

Uh oh!

jasonmalinowski Dec 4, 2017

Choose a reason for hiding this comment

Uh oh!

tmat Dec 11, 2017

Choose a reason for hiding this comment

Uh oh!

tmat Dec 11, 2017

Choose a reason for hiding this comment

Uh oh!

sharwell commented Dec 4, 2017

Uh oh!

jasonmalinowski commented Dec 4, 2017

Uh oh!

CyrusNajmabadi commented Dec 4, 2017

Uh oh!

sharwell commented Dec 4, 2017

Uh oh!

sharwell commented Dec 8, 2017

Uh oh!

jaredpar commented Dec 8, 2017

Uh oh!

sharwell commented Dec 8, 2017

Uh oh!

sharwell commented Mar 1, 2018

Uh oh!

jaredpar left a comment

Choose a reason for hiding this comment

Uh oh!

jaredpar commented Mar 1, 2018

Uh oh!

tmat commented Mar 1, 2018

Uh oh!

Pilchie commented Mar 1, 2018

Uh oh!

jasonmalinowski left a comment

Choose a reason for hiding this comment

Uh oh!

jasonmalinowski Mar 3, 2018

Choose a reason for hiding this comment

Uh oh!

sharwell commented Mar 3, 2018

Uh oh!

sharwell commented Apr 19, 2018

Uh oh!

jaredpar commented Apr 19, 2018

Uh oh!

sharwell commented Apr 20, 2018

Uh oh!

This comment was marked as resolved.

Uh oh!

jaredpar commented Apr 20, 2018

Uh oh!

sharwell commented Apr 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Pilchie commented Apr 20, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

sharwell commented Dec 4, 2017 •

edited

Loading

sharwell commented Apr 20, 2018 •

edited

Loading