-
Notifications
You must be signed in to change notification settings - Fork 527
RequestBody draining still allocates per-request buffers #334
Comments
/cc @halter73 @benaadams |
This is only on fresh connections, not keep alive already open connections? Might be worth pooling it; since its just a reusable sink. |
Resolves aspnet#334
Raised PR #335 "Use pooled array" |
Resolves aspnet#334
Resolves aspnet#334
Resolves aspnet#334
I guess I didn't look at the PR that added |
All of these solutions involve copy bytes from one location to another in memory. Wouldn't it be better to override |
@rynowak That's definitely an option. We were considering a change to make CopyToAsync bufferless in all cases, but that seemed to be a little too complex for a last minute rc1 fix. It might make more sense just to add a new method to FrameRequestStream that efficiently consumes the rest of the request rather than overriding CopyToAsync and checking if the argument is Stream.Null only to do the same thing. |
⬆️ either of those would work. |
Because you'll have multiple threads writing to it on different cores and cause cpu cache thrashing. |
👍 Could even just call it directly in Frame; it constructs |
Something that fasts forwards to the end rather than doing the copy to a buffer would be better; as should be less work? (Less work for CPU rather than; for dev) |
Resolves aspnet#334
Resolves aspnet#334
Resolves aspnet#334
Resolves aspnet#334
Resolves aspnet#334
Resolves aspnet#334
Resolves aspnet#334
Resolves aspnet#334
Resolves aspnet#334
Resolves aspnet#334
This change: 4bba074
Fixed a serious allocation issue, but also added a 4kb buffer to each frame, which is more per-request allocation. Simply just making this
static
as well may not do what we want, as this might lead to a lot of contention for this cache line.I think we need to revisit the original proposal which was to implement
CopyToStreamAsync
in a smarter way.Allocation data for 3000 requests:
In my sample app, this is responsible for 12/82mb allocated
The text was updated successfully, but these errors were encountered: