Use System.Buffers + ValueTask in FormReading #556

benaadams · 2016-02-07T09:44:26Z

Built on #550
Addressing Task allocations in #553 (last commit in this PR)

ValueTask reduces Task allocation by FormReader to 1%
Post for 30,000 requests - byte[] allocations are as follows 0.5MB on rent vs 440MB on MemoryStream originally

/cc @rynowak @davidfowl @Tratcher @stephentoub

stephentoub · 2016-02-07T14:26:57Z

This is because ValueTask isn't supported in net451

Why? The main point of putting it in a new assembly was so it would be usable there:
https://github.com/dotnet/corefx/blob/master/src/System.Threading.Tasks.Extensions/src/System.Threading.Tasks.Extensions.csproj#L9

benaadams · 2016-02-07T19:18:30Z

This is because ValueTask isn't supported in net451

Why? The main point of putting it in a new assembly was so it would be usable there:

Entirely my fault as I couldn't follow instructions; when it said it needed "System.Threading.Tasks": "4.0.0.0" it literally meant exactly that, not "System.Threading.Tasks": "4.0.0-*"

benaadams · 2016-02-11T11:18:46Z

Work done

Current

Buffers + ValueTask

benaadams · 2016-02-11T12:00:24Z

ValueTask reduces Task allocation by FormReader to 1% which continues to be amortised over time downwards #553 (comment)

Revisiting the original allocation issue from 30,000 requests (x10 on original)

ValueTask dramatically reduces the Task allocations to around 1% of the current (7MB vs 750MB)

benaadams · 2016-02-11T12:24:30Z

Pre-for 3,000 requests 44MB on MemoryStream

Post for 30,000 requests - byte[] allocations are as follows 0.5MB on rent vs 440MB on MemoryStream originally

rynowak · 2016-02-11T15:38:51Z

Nice 👍

benaadams · 2016-03-02T07:28:26Z

Rebased; removed WebEncoders changes as its changed somewhat.

sajayantony · 2016-03-02T08:15:07Z

Sweet 👍 /cc @DavidObando @mnltejaswini

DavidObando · 2016-03-02T18:19:02Z

Love this.

rynowak · 2016-03-02T20:06:54Z

src/Microsoft.AspNetCore.WebUtilities/FileBufferingReadStream.cs

@@ -43,6 +56,17 @@ public class FileBufferingReadStream : Stream
                throw new ArgumentNullException(nameof(tempFileDirectoryAccessor));
            }

+            _bytePool = bytePool;
+            if (memoryThreshold < 1024 * 1024)


Would be nice to put this in a constant.

benaadams · 2016-03-07T22:13:24Z

Rebased, somewhat squashed, feedback changes made

benaadams · 2016-03-07T22:21:45Z

src/Microsoft.AspNetCore.WebUtilities/FormReader.cs

+            using (reader)
+            {
+                var pair = await pairTask;
+                while (pair.HasValue)


how does this work? Am I missing something that way over my head?

@rynowak do you mean the splitting of the async version from the non-async version (until it becomes async) or the end of read being indicated by a null?

I just hadn't read these two lines super well.

I imagine a golden time in the future when the compiler automatically does these patterns for you...

davidfowl · 2016-03-14T15:52:54Z

@benaadams Can you rebase?

benaadams · 2016-03-14T17:00:55Z

Rebased; note: I had to bump src/Microsoft.Net.Http.Headers/project.json from "netstandard1.0" to "netstandard1.1"

davidfowl · 2016-03-14T17:06:57Z

src/Microsoft.AspNetCore.WebUtilities/project.json

  },
  "frameworks": {
    "net451": {
      "frameworkAssemblies": {
-        "System.Runtime": { "type": "build" }
+        "System.Runtime": { "type": "build" },
+        "System.Threading.Tasks": "4.0.0.0"


This needs to be type build like the other facades

muratg · 2016-03-17T21:28:35Z

src/Microsoft.AspNetCore.Owin/WebSockets/OwinWebSocketAdapter.cs

+                    // Drain until close received
+                    await ReceiveAsync(new ArraySegment<byte>(buffer), cancellationToken);
+                }
+            }


Would this ever actually throw?

Potentially, could throw if socket already disconnected or cancellation token cancelled.

Tratcher · 2016-03-22T19:25:39Z

This ValueTask pattern causes substantial code duplication and maintenance issues. I don't think it's piratical. Can we split this and just take the buffer pooling?

benaadams · 2016-03-22T19:38:13Z

This ValueTask pattern causes substantial code duplication and maintenance issues. I don't think it's piratical.

The ValueTask does cut the allocations on 30k requests from 750MB to 7MB though

/cc @rynowak

Tratcher · 2016-03-22T19:49:30Z

The buffer pooling is ready to merge and unrelated to the ValueTask work. Let's get that merged.

We need to come up with a simpler pattern if we're going to use ValueTasks.

benaadams · 2016-03-22T19:51:17Z

K, will split them later today

benaadams · 2016-03-23T13:11:29Z

We need to come up with a simpler pattern if we're going to use ValueTasks.

Know what you mean, there are a bunch of higher performance patterns with async that do make the code more confusing as you need to split the code into multiple fragments.

I wonder if there is scope to push this into the compiler instead? As it already does that for general async.

Would rosyln be the correct repo to raise this? /cc @stephentoub

stephentoub · 2016-03-23T13:13:04Z

Would roslyn be the correct repo to raise this?

dotnet/roslyn#7169

Tratcher · 2016-03-30T22:14:45Z

I recommend we close this pending the roslyn discussion. @benaadams @rynowak?

rynowak · 2016-03-30T23:02:32Z

That seems fine, I don't think we're going to take this PR in it's current form

dnfclas added the cla-already-signed label Feb 7, 2016

benaadams force-pushed the bufferpool-valuetask branch from 8ff6275 to 28b6cd9 Compare February 7, 2016 09:53

benaadams force-pushed the bufferpool-valuetask branch from 28b6cd9 to 25c7ac3 Compare February 7, 2016 19:16

benaadams mentioned this pull request Feb 7, 2016

Allocations in FormReader #553

Closed

benaadams force-pushed the bufferpool-valuetask branch 2 times, most recently from 8d79967 to 88ec9fa Compare February 11, 2016 10:18

benaadams mentioned this pull request Feb 11, 2016

Use System.Buffers #550

Closed

12 tasks

benaadams mentioned this pull request Feb 11, 2016

Allocations in FileBufferingReadStream #548

Closed

benaadams force-pushed the bufferpool-valuetask branch from 88ec9fa to 1db60b9 Compare March 2, 2016 07:27

rynowak reviewed Mar 2, 2016
View reviewed changes

benaadams force-pushed the bufferpool-valuetask branch 2 times, most recently from 0a5c6b9 to f270c45 Compare March 7, 2016 22:08

benaadams reviewed Mar 7, 2016
View reviewed changes

benaadams changed the title ~~Use ValueTask in FormReading~~ Use ValueTask in FormReading + System.Buffers Mar 8, 2016

benaadams changed the title ~~Use ValueTask in FormReading + System.Buffers~~ Use System.Buffers + ValueTask in FormReading Mar 8, 2016

benaadams added 3 commits March 14, 2016 12:23

Use System.Buffers for temporary arrays

70a8b61

Use ValueTask in FormReading

3307eeb

Introduce constants for buffer sizes

ff075e9

benaadams force-pushed the bufferpool-valuetask branch from f270c45 to ff075e9 Compare March 14, 2016 16:28

davidfowl reviewed Mar 14, 2016
View reviewed changes

System.Threading.Tasks -> build

7cb98f0

muratg reviewed Mar 17, 2016
View reviewed changes

Tratcher self-assigned this Mar 22, 2016

benaadams mentioned this pull request Mar 22, 2016

Use System.Buffers for temporary arrays #595

Closed

Tratcher closed this Mar 30, 2016

ljw1004 mentioned this pull request May 19, 2016

Discussion thread for arbitrary async returns dotnet/roslyn#10902

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use System.Buffers + ValueTask in FormReading #556

Use System.Buffers + ValueTask in FormReading #556

benaadams commented Feb 7, 2016

stephentoub commented Feb 7, 2016

benaadams commented Feb 7, 2016

benaadams commented Feb 11, 2016

benaadams commented Feb 11, 2016 •

edited

benaadams commented Feb 11, 2016

rynowak commented Feb 11, 2016

benaadams commented Mar 2, 2016

sajayantony commented Mar 2, 2016

DavidObando commented Mar 2, 2016

rynowak Mar 2, 2016

benaadams commented Mar 7, 2016

benaadams Mar 7, 2016

rynowak Mar 14, 2016

benaadams Mar 14, 2016

davidfowl commented Mar 14, 2016

benaadams commented Mar 14, 2016

davidfowl Mar 14, 2016

benaadams Mar 14, 2016

muratg Mar 17, 2016

benaadams Mar 18, 2016

Tratcher commented Mar 22, 2016

benaadams commented Mar 22, 2016

Tratcher commented Mar 22, 2016

benaadams commented Mar 22, 2016

benaadams commented Mar 23, 2016

stephentoub commented Mar 23, 2016

Tratcher commented Mar 30, 2016

rynowak commented Mar 30, 2016

Use System.Buffers + ValueTask in FormReading #556

Use System.Buffers + ValueTask in FormReading #556

Conversation

benaadams commented Feb 7, 2016

stephentoub commented Feb 7, 2016

benaadams commented Feb 7, 2016

benaadams commented Feb 11, 2016

benaadams commented Feb 11, 2016 • edited

benaadams commented Feb 11, 2016

rynowak commented Feb 11, 2016

benaadams commented Mar 2, 2016

sajayantony commented Mar 2, 2016

DavidObando commented Mar 2, 2016

Choose a reason for hiding this comment

benaadams commented Mar 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidfowl commented Mar 14, 2016

benaadams commented Mar 14, 2016

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

Tratcher commented Mar 22, 2016

benaadams commented Mar 22, 2016

Tratcher commented Mar 22, 2016

benaadams commented Mar 22, 2016

benaadams commented Mar 23, 2016

stephentoub commented Mar 23, 2016

Tratcher commented Mar 30, 2016

rynowak commented Mar 30, 2016

benaadams commented Feb 11, 2016 •

edited