-
Notifications
You must be signed in to change notification settings - Fork 523
[Wip] Differentiate write and write2 + simplify #526
Conversation
/// <summary> | ||
/// Summary description for UvWriteRequest2 | ||
/// </summary> | ||
public class UvWrite2Req : UvRequest |
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.
The class name and file name are inconsistent. I personally prefer UvWriteReq2
.
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.
Renamed
|
||
public unsafe void Write2( | ||
UvStreamHandle handle, | ||
ArraySegment<byte> buf, |
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.
I wonder if it would be better to put the dummy buffer in this class and use a static pBuffers
object. Better encapsulation probably.
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.
Better yet. Create the dummy buffer in this class using CreateMemory
. This way the only GCHandle
we have to bother with is _pinUvWrite2Req
.
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.
Made change for this but can't test due to dotnet/aspnetcore#1207
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.
@benaadams Do you still have this change?
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.
Yes, though will have to add it to PR after Saturday (Playtest this weekend so focused on that)
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.
Sounds good to me. Good luck with the playtest!
Still needs a change as above |
@benaadams Still working on this? |
Not at the moment; would be a slight breaking change though would be weird if anyone but Kestrel took a dependency on them |
Going to close this and raise as an issue |
UvWriteReq only uses pooled blocks
UvWrite2Req only has one buffer
List<GCHandle>
isn't neededFrom #519