udp/stream: use MaybeStackBuffers to manage buffer memory management #8626
Conversation
instead of creating own buffer, use MaybeStackBuffer on DoSend to take advantage of its destructor to free up memory, so buffer never leaks memory - even if code in DoSend throws.
Use MaybeStackBuffer in Writev to take advantage of destructor on MaybeStackBuffer to clear itself up, rather than Writev managing resources itself.
nodejs-github-bot
added
the
c++
| @@ -102,8 +102,7 @@ int StreamBase::Writev(const FunctionCallbackInfo<Value>& args) { | |||
|
|
|||
| size_t count = chunks->Length() >> 1; | |||
|
|
|||
| uv_buf_t bufs_[16]; | |||
| uv_buf_t* bufs = bufs_; | |||
| MaybeStackBuffer<uv_buf_t> bufs(count); | |||
There was a problem hiding this comment.
Can you use MaybeStackBuffer<uv_buf_t, 16>? That should keep the current default size, which seems reasonable to me
There was a problem hiding this comment.
sure, but still pass in count when creating it as I'm doing?
Or should I reinstate l135-137 to create the bigger buffer if required?
There was a problem hiding this comment.
MaybeStackBuffer does exactly that – it creates a bigger buffer of count entries when the static buffer is not large enough. :) So, yes, both a default of 16 as the second template parameter, and count as the second parameter.
There was a problem hiding this comment.
Excellent. I suppose the same change should be made for for udp_wrap.cc too a345322?diff=split#diff-b62464f44488c6247346b82a87cbd20aL278
There was a problem hiding this comment.
CI looks good, and a +4 −22 diff is always good news!
It would be really great if you could squash the commits, reformat the commit message so that it starts with src: (we usually do that for cleanup changes to src/) and the title line is no longer than 50 characters.
That can also be done when landing the commit, if you prefer.
|
Thanks @addaleax. I'm on leave without access to git but back early next week so will pick it up then:) thanks for your help. |
|
@pkiddie That would mean it’s probably easiest to clean up the commit message when merging the commit, unless you’d prefer to do it yourself? |
|
@addaleax Happy for it to be merged and the message cleaned up, rather than wait on me |
instead of creating own buffer, use MaybeStackBuffer on DoSend to take advantage of its destructor to free up memory, so buffer never leaks memory - even if code in DoSend throws. Use MaybeStackBuffer in Writev to take advantage of destructor on MaybeStackBuffer to clear itself up, rather than Writev managing resources itself. PR-URL: #8626 Reviewed-By: Anna Henningsen <[email protected]>
addaleax
added
the
lib / src
|
Landed in fffb9a3 with squashed commit message, thank you for doing this! 🎉 |
instead of creating own buffer, use MaybeStackBuffer on DoSend to take advantage of its destructor to free up memory, so buffer never leaks memory - even if code in DoSend throws. Use MaybeStackBuffer in Writev to take advantage of destructor on MaybeStackBuffer to clear itself up, rather than Writev managing resources itself. PR-URL: #8626 Reviewed-By: Anna Henningsen <[email protected]>
Checklist
make -j4 test(UNIX), orvcbuild test nosign(Windows) passesAffected core subsystem(s)
udpstreamDescription of change
The
uv_buf_t bufs_[16]lines could beMaybeStackBuffers; as nodejs/code-and-learn#56