-
Notifications
You must be signed in to change notification settings - Fork 4.9k
Remove a closure allocation from WinHttpResponseStream #7785
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hold on, don't merge this just yet; it looks like a closure for token is also being implicitly allocated here. Will patch in a moment.
|
test innerloop osx debug build and test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe in the future TcsQueryDataAvailable could be changed to a subclass of TaskCompletionSource holding the relevant data to avoid the tuple allocation (similar to what was done in #7788), but for now I'm leaving this PR as-is.
|
@jamesqo I am thinking about doing some potential refactoring of this class to address some cancellation issues when reading response streams. I want to hold this PR for now until that issue is worked out. Thanks. |
|
@davidsh Sure, no problem. |
|
@davidsh , are you still working on the refactorings that you mentioned? What are the next steps on this PR? |
|
@jamesqo Thank you for your work on this PR. We are still involved in reworking parts of WinHttpResponseStream especially dealing with existing memory leaks etc. So, it would be best to close out this PR at this time. |
Pass in the current
WinHttpResponseStreamas a state parameter to the continuation, so we avoid a closure. Also unindented the code.cc @davidsh @stephentoub
edit: Just updated it to pass in the method parameters as state, as well.