stream_wrap: use uv_try_write where possible
#6902
Conversation
Use `uv_try_write` for string and buffer writes, thus avoiding to do allocations and copying in some of the cases.
| @@ -419,7 +419,6 @@ int uv__close(int fd) { | |||
| int rc; | |||
|
|
|||
| assert(fd > -1); /* Catch uninitialized io_watcher.fd bugs. */ | |||
| assert(fd > STDERR_FILENO); /* Catch stdio close bugs. */ | |||
There was a problem hiding this comment.
merge issue? not sure why there's a change in deps/
|
Will do a more through review. Though I have one general concern that's already an issue with the current streams API. If a user is writing out a bunch of small requests, and their waiting for the Stream API to tell them when to stop writing, there's the possibility of blocking the event loop for too long. If not indefinitely. Where this is a problem is because in JS the callbacks are then called immediately. This can allow issues to occur when writing out large files that cause cause the So, basically, there are issues with the Streams JS API that need to be resolved and this patch would make them more likely to happen. Also, can you briefly explain when it's "immediately" written? Like, that it's just been passed off to the kernel to be written. Sorry, not familiar w/ how the system guts work. Overall I like the idea. Adds complexity, but I see significant potential performance gains in key areas (e.g. intra-datacenter communication). |
|
This patch would not change behavior of the streams, it just makes them skip allocation process if the write could be completed "immediately", and by immediately I mean that |
| size_t data_size; | ||
| uv_buf_t buf; | ||
|
|
||
| bool try_write = storage_size + 15 <= sizeof(stack_storage) && |
There was a problem hiding this comment.
we should be avoiding the use of magic numbers, that is to say -- can we use constant symbols instead? I know the 15 was there before this change.
There was a problem hiding this comment.
Yep, I haven't introduced it. And not going to fix it as part of this PR ;)
|
ping guys! |
|
Throw in this: diff --git a/src/stream_wrap.cc b/src/stream_wrap.cc
index 3ae078c..39c139c 100644
--- a/src/stream_wrap.cc
+++ b/src/stream_wrap.cc
@@ -32,6 +32,7 @@
#include "util.h"
#include "util-inl.h"
+#include <string.h> // memcpy()
#include <stdlib.h> // abort()
#include <limits.h> // INT_MAXto get it to build |
|
Pushed fix. |
| char* storage; | ||
| WriteWrap* req_wrap; | ||
| char* data; | ||
| char stack_storage[16384]; |
There was a problem hiding this comment.
What's this magical 16384 from? Please place a comment why that number is significant. :)
|
@indutny did my thing. ping me back. :) |
|
@trevnorris all fixed, I think? |
|
Needs to be landed after joyent/libuv#1086 |
|
ok. think I'm cool with this. |
|
Ok, I have another fix instead that one. |
|
This one: #6946 |
|
@trevnorris LGTY? |
|
Logic seems good, but posterity be warned that I don't understand the inner workings of @indutny LGTM. |
|
@tjfontaine your turn, please ;) |
| req.cb = cb; | ||
| } else { | ||
| if (!req.async) | ||
| timers._unrefActive(this); |
There was a problem hiding this comment.
Why do we need to set the timer here vs L620?
There was a problem hiding this comment.
It is not required, I guess it is a mistake...
|
I am basically +1 on this, though I would appreciate some comment blocks on the interfaces such that I don't have to think about the semantics in detail every time I approach it |
|
Thank you, landed in 9836a4e |
Use
uv_try_writefor string and buffer writes, thus avoiding to doallocations and copying in some of the cases.
/cc @tjfontaine @trevnorris