Proposal: Form parsing improvements #413
Conversation
|
Hi @benaadams, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution! The agreement was validated by .NET Foundation and real humans are currently evaluating your PR. TTYL, DNFBOT; |
| public static readonly IReadableStringCollection Empty = new ReadableStringCollection(); | ||
|
|
||
| private static readonly string[] EmptyKeys = new string[0]; | ||
| private static readonly IEnumerable<KeyValuePair<string, StringValues>> EmptyEnumerable = (IEnumerable<KeyValuePair<string, StringValues>>)(new KeyValuePair<string, StringValues>[0]); |
There was a problem hiding this comment.
Not sure where that is? Can't seem to grab a ref.
Can do the #if !DNXCORE50 stuff on EmptyKeys with Array.Empty<string>() though?
Also #if DNXCORE50 for EmptyEnumerable = Array.Empty<KeyValuePair<string, StringValues>>())
Didn't know if it was too messy
Also be explicit about the conversion cost
benaadams
force-pushed
the
form-parsing
branch
from
5a6cc4b to
44fb6c5
Compare
| @@ -119,7 +144,7 @@ public IFormCollection ReadForm() | |||
| var section = await multipartReader.ReadNextSectionAsync(cancellationToken); | |||
| while (section != null) | |||
| { | |||
| var headers = new HeaderDictionary(section.Headers); | |||
| var headers = section.Headers; | |||
| ContentDispositionHeaderValue contentDisposition; | |||
| ContentDispositionHeaderValue.TryParse(headers[HeaderNames.ContentDisposition], out contentDisposition); | |||
There was a problem hiding this comment.
regression: this will throw if Content-Disposition is missing. You could just use section.ContentDisposition instead.
There was a problem hiding this comment.
Which would wrap; so delay wrapping down a few lines for FormFile wouldn't help. Will revert.
There was a problem hiding this comment.
There was a problem hiding this comment.
Changed as suggested
|
Can you share your metrics for these changes? |
Can't actually run it atm; the dnx unstable feeds are a few versions back so need to wait for them to be updated |
|
Resolves #368 ? |
benaadams
force-pushed
the
form-parsing
branch
from
97ecb7e to
eb4ac33
Compare
benaadams
changed the title
|
|
||
| // Some of these code paths use StreamReader which does not support cancellation tokens. | ||
| using (cancellationToken.Register(_request.HttpContext.Abort)) | ||
| using (cancellationToken.Register((Action)_request.HttpContext.Abort)) |
There was a problem hiding this comment.
Why are you casting here? You can actually avoid the delegate allocation per request here as well.
cancellationToken.Register(state => ((HttpContext)state).Abort(), _request.HttpContext)
There was a problem hiding this comment.
Couldn't work out how to avoid alloc; that will do nicely :)
| get { return _form; } | ||
| set | ||
| { | ||
| _parsedFormTask = null; |
There was a problem hiding this comment.
Why not set the cached task here?
There was a problem hiding this comment.
You've got the non-async method; so didn't want to set the task if that's the only path used
|
Will be covered by rebased #411 |
Cached task pattern for avoiding state machine and task allocation for repeated ReadFormAsync
struct KeyValueAccumulator
Delayed allocations; reduced allocs for empty paths