Skip to content
This repository has been archived by the owner on Nov 20, 2018. It is now read-only.

Use System.Buffers #550

Closed
wants to merge 15 commits into from
Closed

Use System.Buffers #550

wants to merge 15 commits into from

Conversation

benaadams
Copy link
Contributor

Resolves #545

Added System.Buffer

  • OwinWebSocketAdapter.CloseAsync
  • Microsoft.AspNetCore.WebUtilities.BufferedReadStream.ctor
  • Microsoft.AspNetCore.WebUtilities.FormReader.ctor (variable initialized)
  • Microsoft.AspNetCore.WebUtilities.HttpRequestStreamReader.ctor
  • Microsoft.AspNetCore.WebUtilities.HttpResponseStreamWriter.ctor
  • Microsoft.AspNetCore.WebUtilities.MultipartReaderStream.Read
  • Microsoft.AspNetCore.WebUtilities.MultipartReaderStream.ReadAsync
  • Microsoft.AspNetCore.WebUtilities.StreamHelperExtensions.DrainAsync
  • Microsoft.AspNetCore.WebUtilities.WebEncoders.Base64UrlDecode
  • Microsoft.AspNetCore.WebUtilities.WebEncoders.Base64UrlEncode
  • Microsoft.Net.Http.Headers.ContentDispositionHeaderValue.TryDecode5987

Extra

  • Microsoft.AspNetCore.WebUtilities.FileBufferingReadStream

Now also resolves #548

...except when content is buffered to disk; which needs this issue resolving in corefx https://github.com/dotnet/corefx/issues/5598
Due to FileStream's internal buffer is not being exposed/settable.

@davidfowl
Copy link
Member

Seeing all of these hardcoded statics worry me a bit. I think we should pass the arraypool in the ctor and have overloads that use the shared one by default.

@benaadams
Copy link
Contributor Author

Changed BufferPool so it can be overridden from Shared for everything except
OwinWebSocketAdapter.CloseAsync - different api than standard owin
ContentDispositionHeaderValue.TryDecode5987 - messy

@benaadams
Copy link
Contributor Author

Alas doesn't address #548 as FileBufferingReadStream is more messy and if you give MemoryStream a starting buffer its fixed size; so would need to do an extra step in the progressive upgrade
(e.g. 1MB byte[] -> MemoryStream -> File)

@benaadams
Copy link
Contributor Author

Actually....

@benaadams
Copy link
Contributor Author

Travis fail is a "too many open files" issue on OSX

@muratg muratg added this to the 1.0.0-rc2 milestone Feb 3, 2016
@Tratcher Tratcher added the Perf label Feb 3, 2016
@benaadams
Copy link
Contributor Author

Raised issue for decoding https://github.com/dotnet/corefx/issues/5888

@@ -51,6 +74,14 @@ public class FileBufferingReadStream : Stream
// TODO: allow for an optional buffer size limit to prevent filling hard disks. 1gb?
public FileBufferingReadStream(Stream inner, int memoryThreshold, string tempFileDirectory)
Copy link
Member

Choose a reason for hiding this comment

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

This ctor isn't chaining.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

eep!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

is now


if ((ch & 0xFF80) == 0)
{
helper.AddByte((byte)ch); // 7 bit have to go as bytes because of Unicode
Copy link
Member

Choose a reason for hiding this comment

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

Brain explodes... I liked it better when I didn't have to know how the decoder worked.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Kept it private so can push it back into corefx at somepoint

@benaadams
Copy link
Contributor Author

Made feedback changes

@Tratcher
Copy link
Member

Tratcher commented Feb 5, 2016

@rynowak How do you want to verify this?

@benaadams
Copy link
Contributor Author

Superseded by #556 ?

@benaadams
Copy link
Contributor Author

Pre-for 3,000 requests 44MB on MemoryStream

pre-alloc

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

allocs

@benaadams benaadams closed this Feb 11, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants