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

Don't allocate for ResponseCookiesFeature #699

Closed
wants to merge 1 commit into from
Closed

Don't allocate for ResponseCookiesFeature #699

wants to merge 1 commit into from

Conversation

@benaadams
Copy link
Contributor

@benaadams benaadams commented Sep 2, 2016 •
edited
Loading

  • Don't allocate ResponseCookiesFeature when not used
  • Don't allocate StringBuilder pool
  • Don't allocate StringBuilders
  • Don't allocate extra strings when constructing cookie string

Resolves #676

/cc @davidfowl @muratg @Tratcher @pakrym

@benaadams benaadams force-pushed the no-alloc-ResponseCookiesFeature- branch from d3c7782 to 668f574 Compare September 2, 2016 18:34
@benaadams benaadams mentioned this pull request Sep 2, 2016
Closed
Tratcher
Tratcher reviewed Sep 2, 2016
View reviewed changes
src/Microsoft.AspNetCore.Http/Features/ResponseCookiesFeature.cs
/// <see cref="IResponseCookiesFeature"/> and the <see cref="IHttpResponseFeature"/>.
/// </param>
/// <param name="builderPool">The <see cref="ObjectPool{T}"/>, if available.</param>
public ResponseCookiesFeature(IFeatureCollection features, ObjectPool<StringBuilder> builderPool)
Copy link
Member

@Tratcher Tratcher Sep 2, 2016

Choose a reason for hiding this comment

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

Side note: we can't remove public API (even if we change the behavior).

Copy link
Contributor Author

@benaadams benaadams Sep 2, 2016

Choose a reason for hiding this comment

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

Restored. Do I need to add back AppendToStringBuilder on SetCookieHeaderValue?

Copy link
Member

@Tratcher Tratcher Sep 2, 2016

Choose a reason for hiding this comment

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

Yes, it was public in v1.0

Copy link
Contributor Author

@benaadams benaadams Sep 2, 2016

Choose a reason for hiding this comment

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

Restored

@benaadams benaadams force-pushed the no-alloc-ResponseCookiesFeature- branch 2 times, most recently from 7d5188b to e8e4f7c Compare September 2, 2016 19:41
@benaadams
Copy link
Contributor Author

benaadams commented Sep 4, 2016

Could also use string.Concat with a single string[] which would only be an allocation for the array as its quite efficient memory-wise.

Continually appending to the string is obv problematic, but the cookie is both known size and a known number of strings.

@muratg muratg added this to the 1.1.0 milestone Sep 7, 2016
@Tratcher
Copy link
Member

Tratcher commented Sep 30, 2016

The string.concat option sounds like it's worth a try.

@pakrym
Copy link
Contributor

pakrym commented Sep 30, 2016 •
edited
Loading

Would be 2 allocations, 1 for array another for string, I'm thinking about extracting inplace string formatting into a struct, it would look like

var isf = new InplaceStringFormatter();
isf.AppendLength(s1);
isf.AppendLength(s2);
isf.AppendLength(s3);
isf.AppendValue(s1);
isf.AppendValue(s2);
isf.AppendValue(s3);
var s= isf.ToString();

Could reuse it in #716 too.

This was referenced Sep 30, 2016
Closed
Merged
@pakrym
Copy link
Contributor

pakrym commented Oct 3, 2016

@benaadams how do you feel about moving this PR to use dotnet/extensions#157 ?

@benaadams
Copy link
Contributor Author

benaadams commented Oct 4, 2016

Yep

@benaadams
Copy link
Contributor Author

benaadams commented Oct 4, 2016

Need to wait till stuff builds locally :-/

@pakrym
Copy link
Contributor

pakrym commented Oct 6, 2016

@benaadams ping

@benaadams
Copy link
Contributor Author

benaadams commented Oct 7, 2016

Still can't build

Errors in I:\Work\GitHub\HttpAbstractions\test\Microsoft.Net.Http.Headers.Tests\project.json
    Package Microsoft.Extensions.Testing.Abstractions 1.0.0-preview2-003121 is not compatible with netcoreapp1.0 (.NETCoreApp,Version=v1.0). Package Microsoft.Extensions.Testing.Abstractions 1.0.0-preview2-003121 supports:
      - net451 (.NETFramework,Version=v4.5.1)
      - netstandard1.6 (.NETStandard,Version=v1.6)
    One or more packages are incompatible with .NETCoreApp,Version=v1.0.

Etc

@benaadams
Copy link
Contributor Author

benaadams commented Oct 7, 2016

Hmm, does that when I use ./build is ok in VS but can't run tests

@benaadams benaadams force-pushed the no-alloc-ResponseCookiesFeature- branch 2 times, most recently from babb29a to debeab2 Compare October 7, 2016 05:13
@benaadams
Copy link
Contributor Author

benaadams commented Oct 7, 2016

@pakrym updated

@pakrym
Copy link
Contributor

pakrym commented Oct 7, 2016

You have old version of dotnet sdk somewhere, check what dotnet.exe gets used because

    Package Microsoft.Extensions.Testing.Abstractions 1.0.0-preview2-003121 is not compatible with netcoreapp1.0 (.NETCoreApp,Version=v1.0). Package Microsoft.Extensions.Testing.Abstractions 1.0.0-preview2-003121 supports:
      - net451 (.NETFramework,Version=v4.5.1)
      - netstandard1.6 (.NETStandard,Version=v1.6)

was fixed long long time ago.

@pakrym
Copy link
Contributor

pakrym commented Oct 7, 2016

4569653

@pakrym pakrym closed this Oct 7, 2016
@khellang
Copy link
Contributor

khellang commented Dec 19, 2016

Is there a reason why the (Pooled)HttpContextFactory ctors taking an ObjectPoolProvider wasn't obsoleted with this PR?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

@pakrym pakrym

Labels

None yet

Projects

None yet

Milestone

1.1.0-preview1

Development

Successfully merging this pull request may close these issues.

6 participants

@benaadams @Tratcher @pakrym @khellang @muratg @dnfclas