tcp: enable large socket (rfc1323) data transport by gireeshpunathil · Pull Request #1279 · libuv/libuv

gireeshpunathil · 2017-03-29T12:17:45Z

For TCP based full duplex streams, leverage the TCP window scaling, if it is supported by the underlying system. The socket buffer size is aligned to the system configuration values, and the internal I/O buffers are enlarged to enable transport of large amount of data.

Summary of changes:

Introduces an API uv_enable_largesocket(uv_stream_t *handle) to customize socket buffers
Customized the stream chunk size from adjusted socket buffers
Added a test case to validate the API as well as for negative validation.
Changed the documentation to reflect the new API.
Currently supported only for Linux, AIX and MAC OS X.

Custom streaming test cases (with node.js) with large data transfer (5 - 200 MB) and with reasonable processing on the data (map-reduce) in an LRTT network showed performance improvements on the lines of:

12% (Linux)
10% (AIX)
16% (MAC)
15%(WIndows - tested with a PoC, not fully developed code)

Please refer to #1217 for background and further details.

santigimeno · 2017-03-29T12:55:27Z

+.. c:function:: int uv_enable_largesocket(uv_stream_t* handle)
+
+    For TCP based full duplex streams, leverages the TCP window scaling,
+    if it is supported by underlying system. The socket buffer size is aligned


by the underlying system?

@santigimeno - thanks for the review and the suggestions - made this change.

santigimeno · 2017-03-29T12:59:05Z

+int uv_enable_largesocket(uv_stream_t *stream) {
+#if defined(_AIX) || defined(__linux__) || defined(__APPLE__)
+  int ret = uv__enable_largesocket(stream);
+  return ret == 0 ? 0 : UV_ENOSYS;


why don't return uv__enable_largesocket(stream); directly?

done, makes sense as we have only two values to return: 0 or UV_ENOSYS

santigimeno · 2017-03-29T13:02:41Z

+  int rmem = 0;
+  int wmem = 0;
+  int rr = -1;
+  int rw = -1;


Style: separate declaration from assignment. Here and in the rest of the PR.

done - here and in other files as well.

santigimeno · 2017-03-29T13:05:19Z

+  if (ret <= 0)
+    return UV_ENOSYS;
+
+  close(fd);


use uv__close(). Close the descriptor also in case of error.

santigimeno · 2017-03-29T13:06:17Z

+    return UV_ENOSYS;
+
+  close(fd);
+  strcpy(buf2, buf);


You could use uv_strdup and avoid one of the previous malloc calls

santigimeno · 2017-03-29T13:06:32Z

+        token2 = strtok(NULL, "=");
+        if (token2 != NULL) {
+        wmem = (int) strtol(token2, (char **)NULL, 10);
+        ws = 1;


indent this correctly

santigimeno · 2017-03-29T13:07:07Z

+      }
+    }
+  }
+


if ws != 1 we can return early. Also if wmem <= (64 * 1024)

santigimeno · 2017-03-29T13:09:15Z

+  if (rs == 1 && ws == 1) {
+    if (rmem > (64 * 1024) && wmem > (64 * 1024)) {
+      rr = uv_recv_buffer_size((uv_handle_t*) stream, &rmem);
+      rw = uv_send_buffer_size((uv_handle_t*) stream, &wmem);


return early if rr != 0

done - here, and in all other similar places.

santigimeno · 2017-03-29T13:10:46Z

+  if (fscanf(fp, "%d", &rmem) == 1)
+    rs = 1;
+  fclose(fp);
+


if rs != 1 and && rmem <= (64 * 1024) we can return early

done - I guess you meant to return in either case - so using OR instead of AND.
rs != 1 || rmem <= (64 * 1024)

Yes, sorry.

santigimeno · 2017-03-29T13:11:42Z

+
+  oreq.getnext = 1;
+  while (ioctl(fd, SIOCGNETOPT1, (caddr_t)&oreq) != -1) {
+    if (strcmp(oreq.name, "tcp_sendspace") == 0) {


maybe use strncmp here?

done, changed to strncmp

gireeshpunathil · 2017-03-29T18:35:29Z

@santigimeno - thanks, I guess I addressed all your change requests, please let me know if I missed out anything.

txdv · 2017-03-30T07:30:08Z

Maybe put 64 * 1024 in a define? That magic number gets repeated all over the place.

santigimeno · 2017-03-30T08:25:55Z


    .. versionchanged:: 1.4.0 UNIX implementation added.

+.. c:function:: int uv_enable_largesocket(uv_stream_t* handle)


A couple of things:

Why a uv_stream_t instead of a uv_tcp_t as this addition is for TCP only?

What about renaming it to something like uv_tcp_enable_largesocket?

done, this is moved to tcp structure, renamed the API to reflect changes being applicable to tcp only, and the document changes moved from stream.rst to tcp.rst.

santigimeno · 2017-03-30T08:27:12Z

  int delayed_error;                                                          \
  int accepted_fd;                                                            \
  void* queued_fds;                                                           \
+  int chunk_read_size;                                                        \


I'll mark this for v2 because of this interface change

the interface change is not intentional - the requirement is to have a mechanism to associate the modified chunk size with a specific socket / stream upon API invocation, and use that value while reading in uv__write (write is inherently capable of writing as much as it can without blocking, so no issues). Do you have any thoughts on achieving this without introducing an interface change - either to stream / tcp structures?

santigimeno · 2017-03-30T08:28:23Z

+  rs = 0;
+  ws = 0;
+  rmem = 0;
+  wmem = 0;


I don't think initializing rmem and wmem is necessary.

done, removed initialization of rmem and wmem

santigimeno · 2017-03-30T08:29:02Z

+  if (rr != 0)
+    return UV_ENOSYS;
+
+  rw = uv_send_buffer_size((uv_handle_t*) stream, &wmem);


one of rr and rw variables is now innecessary, you can reuse one of them

done, retained only one (renamed to r , following conventions on return values)

santigimeno · 2017-03-30T08:30:24Z

+    return UV_ENOSYS;
+  }
+  buf = uv__malloc(statistics.st_size + 1);
+


still, we need to check that uv__malloc does not fail

done, covered the failure case

santigimeno · 2017-03-30T08:30:53Z

+  }
+
+  uv__close(fd);
+  buf2 = uv__strdup(buf);


check that uv__strdup does not fail

done, covered the failure case.

santigimeno · 2017-03-30T08:40:34Z

+  int rw;
+  char *buf, *buf2, *pattern, *token, *token2;
+  struct stat statistics;
+  int fd = open("/etc/sysctl.conf", O_RDONLY);


Instead of reading the file looking for net.inet.tcp.sendspace and net.inet.tcp.recvspace can't you just make sysctl() calls?

I had thought about this, but did not find net.inet* probes / properties under the head CTL_NET in sys/socket.h.

Excerpts from man sysctl:

The top level names are defined with a CTL_ prefix in <sys/sysctl.h>, and are as follows. The next and subsequent levels down are found in the include files listed here, and described in separate sections below. Name Next level names Description CTL_DEBUG sys/sysctl.h Debugging CTL_VFS sys/mount.h File system CTL_HW sys/sysctl.h Generic CPU, I/O CTL_KERN sys/sysctl.h High kernel limits CTL_MACHDEP sys/sysctl.h Machine dependent CTL_NET sys/socket.h Networking CTL_USER sys/sysctl.h User-level CTL_VM sys/resources.h Virtual memory (struct loadavg)

bash$ find /usr/include | xargs grep -E "sendspace|recvspace" 2> /dev/null

yielded nothing.

Have you tried using sysctlbyname()?

that works, thanks! will make use of this in src/unix/darwin.c

#cat /etc/sysctl.conf kern.ipc.maxsockbuf=16777216 net.inet.tcp.sendspace=1048576 net.inet.tcp.recvspace=1048576 #cat sysctl.c #include <sys/types.h> #include <sys/sysctl.h> #include <stdlib.h> #include <stdio.h> int main() { int sendspace; int recvspace; size_t size = 4; sysctlbyname("net.inet.tcp.sendspace", &sendspace, &size, NULL, 0); sysctlbyname("net.inet.tcp.recvspace", &recvspace, &size, NULL, 0); fprintf(stderr, "%d %d\n", sendspace, recvspace); } #./a.out 1048576 1048576 #

done, the darwin code is now pretty simple and straight forward.

santigimeno · 2017-03-30T08:41:05Z

+  if (rr != 0)
+    return UV_ENOSYS;
+
+  rw = uv_send_buffer_size((uv_handle_t*) stream, &wmem);


only need one of rr and rw.

done, retaining only one.

santigimeno · 2017-03-30T08:44:28Z

+  wmem = 0;
+  if (fscanf(fp, "%d", &rmem) == 1)
+    rs = 1;
+  fclose(fp);


I don't think rs is needed (or ws), you can return with error is fscanf() != 1 and then just check rmem(or wmem) value.

santigimeno · 2017-03-30T08:44:56Z

+  if (rr != 0)
+    return UV_ENOSYS;
+
+  rw = uv_send_buffer_size(stream, &wmem);


Same as above with regards to rr and rw.

saghul · 2017-04-02T11:29:16Z

I haven't done a full review, but note that we plan on removing alloc_cb for v2 (there is even an open PR for it). So IMHO we should make this API explicit, so users can use it, instead of saving it in the TCP handle to pass it to alloc_cb.

santigimeno

I'm not sure if this is what @saghul has in mind, but wouldn't it make sense to have an API that retrieves the max buffer size from the OS so that later the user can decide what size wants to use?

santigimeno · 2017-04-02T15:25:28Z

    The callback is made when the connection has been established or when a
    connection error happened.

+.. c:function:: int uv_tcp_enable_largesocket(uv_tcp_t *handle)


style: we prefer uv_tcp_t* handle over uv_tcp_t *handle

done, thanks.

santigimeno · 2017-04-02T15:25:54Z

                             uv_tcp_t* handle,
                             const struct sockaddr* addr,
                             uv_connect_cb cb);
+UV_EXTERN int uv_tcp_enable_largesocket(uv_tcp_t *tcp);


Style uv_tcp_t* tcp

santigimeno · 2017-04-02T15:26:36Z

 #endif

 static int uv__run_pending(uv_loop_t* loop);
+int uv__tcp_enable_largesocket(uv_tcp_t *tcp);


santigimeno · 2017-04-02T15:28:35Z

+      rmem <= STDTCPWINDOW)
+    return UV_ENOSYS;
+
+  if (sysctlbyname("net.inet.tcp.sendspace", &wmem, &size, NULL, 0) ||


I think this implementation could probably work on the BSD's (FreeBSD, OpenBSD, etc). If that's the case this could be moved to a separate file so could be used by every platform.

I have no complete idea about all the possible BSDs (open|free|net...) and no system to test the changes!

There's a FreeBSD bot in the CI that can be used to test

@santigimeno - is it possible to open up access for me to this CI system where I can do the testing?

@gireeshpunathil yes I think that's possible. I think you should ask for access opening an issue in the https://github.com/nodejs/build repo.

@santigimeno - the darwin specific code is now ported onto BSD. Few points though:

The CI bot is freebasd only, so I wasn't able to test for other BSD variants. But looking at the documentation for the sysctl API as well as another documentation which compares all the BSD variants, I see that the code applies to at least 5 platforms which derive their base system capabilities from 4.4BSD: freebsd, netbsd, openbsd, macos, ios.

There is a mention about dragonfly BSD variant in the documentation, and usage of it in libuv code. I wasn't sure about its relevance in this context, so omitted it for the time. What do you suggest?

The CI bot is freebasd only, so I wasn't able to test for other BSD variants.

I have a OpenBSD VM somewhere, I'll try to run the tests from your branch and report back.

There is a mention about dragonfly BSD variant in the documentation, and usage of it in libuv code. I wasn't sure about its relevance in this context, so omitted it for the time. What do you suggest?

I think DragonFly BSD should work with your changes too, so I would add the support. See: https://leaf.dragonflybsd.org/cgi/web-man?command=tuning&section=7 and https://leaf.dragonflybsd.org/cgi/web-man?command=sysctl&section=3

OK, it doesn't work on OpenBSD as sysctlbyname is not supported. Also, I'm not sure even if net.inet.tcp.recvspace and net.inet.tcp.sendspace exist. Maybe @qbit can shed some light on this.

They do not, there is inet.divert, but I doubt they are the same thing:

from sysctl(3)

divert.recvspace (net.inet.divert.recvspace) Returns the default divert receive buffer size. divert.sendspace (net.inet.divert.sendspace) Returns the default divert send buffer size.

santigimeno · 2017-04-02T15:40:30Z

+static int bytes_read = 0;
+static int largesocket = 0;
+static int large_socket_enabled = 0;
+static int maxread = 0;


No need to initialize static variables to zero

santigimeno · 2017-04-02T15:43:13Z

+  int rmem;
+  int wmem;
+  int r;
+  FILE *fp = uv__open_file("/proc/sys/net/core/rmem_max");


style of the pointer

santigimeno · 2017-04-02T15:49:49Z

+  int r;
+
+  size_t size = sizeof(int);
+  if (sysctlbyname("net.inet.tcp.recvspace", &rmem, &size, NULL, 0) ||


I would drop the size variable and just use sizeof(rmem) or sizeof(wmen)

the 3rd param is an IN/OUT variable which supplies the size of the buffer before the call, and returns the size after the call and it is an address type and I guess it requires an lvalue which sizeof cannot provide?

My bad, you're of course right. Sorry about that.

saghul · 2017-04-03T08:32:24Z

I'm not sure if this is what @saghul has in mind, but wouldn't it make sense to have an API that retrieves the max buffer size from the OS so that later the user can decide what size wants to use?

@santigimeno pretty much, yeah.

gireeshpunathil · 2017-04-03T09:56:11Z

@saghul - thanks for the suggestion. So does that mean that the current proposal of large data reads on a per-socket basis will give way to custom reads, on a per-read basis? Isn't it like providing too low level control to the programmer who may not have that level of clarity of data flow?

For example a user will (rightly) assume that data transport between two end-points are always massive, so they would prefer to enlarge the socket in expectation of all the read / writes between those endpoints are large. But they may not have fine-grained understanding about each read iterations?

Also, what about the writes? The current (v1.x) approach is to write as much as a socket can. The proposed (this PR) approach is to enlarge the socket (on demand) and write as much as the socket can. How your approach will cater to the write scenarios? Will it cap at the buffer size retrieved from OS, or will it ignore the buffer size and write as much as possible?

saghul · 2017-04-03T11:51:38Z

So does that mean that the current proposal of large data reads on a per-socket basis will give way to custom reads, on a per-read basis? Isn't it like providing too low level control to the programmer who may not have that level of clarity of data flow?

I'm not sure I follow. What we are doing is dropping the suggested_size variable from alloc_cb because it's just a hardcoded value, and many get confused by it. With your change, users will be able to pick a value that better works for them, or they could use a fixed value, or use FIONREAD, etc you get the idea.

Also, what about the writes? The current (v1.x) approach is to write as much as a socket can. The proposed (this PR) approach is to enlarge the socket (on demand) and write as much as the socket can. How your approach will cater to the write scenarios? Will it cap at the buffer size retrieved from OS, or will it ignore the buffer size and write as much as possible?

As I said, I haven't done a full review, but a quick glance revealed the reach chunk size stuff which I pointed out. I'll have a better understanding once I do a full review.

Thanks for contributing!

For TCP based full duplex streams, leverage the TCP window scaling, if it is supported by underlying system. The socket buffer size is aligned to the system configuration values, and the internal I/O buffers are enlarged to enable transport of large amount of data.

gireeshpunathil · 2017-04-12T06:49:57Z

@saghul - sorry for responding late. I got your point as: as there was no definite way of deciding I/O chunk sizes, it was hard-coded to be 64KB at the read site, which can be over-ridden by the users in the alloc_cb function - and in most cases the default of 64KB is being used. There is already a v2 proposal to drop this suggestion so that users can define what value they want the I/O to be processed with.

I agree that it makes sense for the sizes to be customisable.

You proposed to make the API explicit so that the users can decide better on how much to read etc. I am inclined to that idea, but would appreciate completion of review of the proposed logic, so that I can address any change proposals.

saghul · 2017-04-12T06:55:25Z

@gireeshpunathil Thanks for understanding.

You proposed to make the API explicit so that the users can decide better on how much to read etc. I am inclined to that idea, but would appreciate completion of review of the proposed logic, so that I can address any change proposals.

Will do! I'm running a bit low in free time these days, I hope I can get to this next week.

sam-github

Would be interesting to have advice in the API docs about the application networking patterns that benefit from using this API, and the ones that don't - assuming there are applications that don't benefit. If all applications benefit from setting large send/recv buffers then it would just be the default for all platforms that support it.

sam-github · 2017-06-01T14:42:50Z

+  int wmem;
+  int r;
+
+  int fd = socket(1, 1, 0);


can we useAF_ and SOCK_ macros instead of "1"?

sam-github · 2017-06-01T14:46:13Z

+
+  int fd = socket(1, 1, 0);
+  if (fd < 0)
+    return UV_ENOSYS;


shouldn't you return the real error? This can also fail with EMFILE and others if you are unlucky, and its when you are unlucky that you want accurate error returns for troubleshooting.

sam-github · 2017-06-01T14:52:07Z

+
+int uv__tcp_enable_largesocket(uv_tcp_t *tcp) {
+  struct optreq1 oreq;
+  int ws;


these could be = 0 here rather than below

sam-github · 2017-06-01T15:01:20Z

+
+    * 0: Indicates system supports TCP window scaling, and socket buffers are
+         increased (in platform specific manner)
+    * EV_ENOSYS: Indicates either no platform support,


I think its a little odd that all errors are coerced to ENOSYS, are there no other reasonable errors? EPERM is impossible, for example?

sam-github · 2017-06-01T15:05:16Z

 #endif

 static int uv__run_pending(uv_loop_t* loop);
+int uv__tcp_enable_largesocket(uv_tcp_t* tcp);


shouldn't this declaration be in src/uv-common.h?

sam-github · 2017-06-01T15:09:32Z

+  return uv__tcp_enable_largesocket(tcp);
+#else
+  /* Implement me. */
+  return UV_ENOSYS;


I'm not sure its better, but the other options is for there be an implementation of uv__tcp_enable_largesocket() for every platform, where some simply return UV_ENOSYS? That would allow the platform list to not have to be maintained in this source file, and the list is present in a couple places, if that's necessary, perhaps there should be a single platform list in uv-common.h that is used to define a UV__HAS_LARGESSOCKET, and the ifdefs in the .c files would use that, so its less likely that the platform lists get out of sync.

sam-github · 2017-06-01T15:10:14Z

+
+int uv__tcp_enable_largesocket(uv_tcp_t *tcp) {
+  int rmem;
+  int wmem;


= 0 here?

sam-github · 2017-06-01T15:12:15Z

+  if (rmem <= STDTCPWINDOW)
+    return UV_ENOSYS;
+
+  fp = uv__open_file("/proc/sys/net/core/wmem_max");


this repeated chunk of code could be refactored into a small file-static helper function

sam-github · 2017-06-01T15:13:51Z

    buf = uv_buf_init(NULL, 0);
+#if defined(_AIX) || defined(__linux__) || defined(__APPLE__)
+    stream->alloc_cb((uv_handle_t*)stream, stream->type == UV_TCP ?
+                    ((uv_tcp_t *) stream)->chunk_read_size : STDTCPWINDOW, &buf);


@saghul this is the part you think could be handled differently to be more forward compatible to uv 2.x?

@sam-github Yes. In v2 there will be no alloc_cb.

sam-github · 2017-06-01T15:15:38Z

    assert(stream->alloc_cb != NULL);

    buf = uv_buf_init(NULL, 0);
+#if defined(_AIX) || defined(__linux__) || defined(__APPLE__)


should this list of platforms be the same as in src/unic/tcp.c?

gireeshpunathil · 2017-06-02T08:03:30Z

re: comment
@sam-github - thanks, I will address your review comments once we have zeroed on the approach.

@saghul - thanks for the pointers, let me try to exaplain my understanding so that we are all on the same page:

Possible consumption models with the API (uv_enable_largesocket)

Call once, and all subsequent TCP streams and all their I/Os will automatically rearm (of course, only as suggested size to the alloc_cb, which is over-ridable) with large buffers => pervasive and resource savvy, and potentially less useful.
Call on a stream, and all subsequent I/Os on the stream will automatically rearm with large buffers => less pervasive, less resource savvy, and more efficient and fine-grained. (current proposal)
Call at an I/O site, on a need basis. (I guess this is what you are proposing in v2?) Only the impending I/O will use large buffer - least pervasive, least resource savvy and finest-grained. But questionable in terms of the level of control / awarenss an application has on specific I/Os, compared to specific streams.

Now in terms of how do we go about in v1.*:

Remove the association of OS-retrievd chunk size with the TCP stream and keep it in a static variable within the API itself.
- With this, model (2) above cannot be achieved, as we lost the association with specific streams.
- With this, model (3) above also not possible, as uv__read and the API are disconnected. (May be I am missing something here?)
- Model (1) is possible.
Turn on a flag (largesocket) if the API is called.
At the I/O site (), use the new size, if the flag is on.

Please suggest. Thanks again!

saghul · 2017-06-02T08:45:31Z

Possible consumption models with the API (uv_enable_largesocket)

Call once, and all subsequent TCP streams and all their I/Os will automatically rearm (of course, only as suggested size to the alloc_cb, which is over-ridable) with large buffers => pervasive and resource savvy, and potentially less useful.

Call on a stream, and all subsequent I/Os on the stream will automatically rearm with large buffers => less pervasive, less resource savvy, and more efficient and fine-grained. (current proposal)

The first two won't work without alloc_cb, so they are not real options for v2.

Call at an I/O site, on a need basis. (I guess this is what you are proposing in v2?) Only the impending I/O will use large buffer - least pervasive, least resource savvy and finest-grained. But questionable in terms of the level of control / awarenss an application has on specific I/Os, compared to specific streams.

This is the approach we need. I don't understand your concern about the level of control / awareness. We would provide a building block, a way for the application to know what a good value is, but ultimately there are other reasons to go with a different one, the use of a circular buffer or other kind of allocator, for example. I don't want libuv to make assumptions abed on a single metric.

Now in terms of how do we go about in v1.*:

Remove the association of OS-retrievd chunk size with the TCP stream and keep it in a static variable within the API itself.
With this, model (2) above cannot be achieved, as we lost the association with specific streams.
With this, model (3) above also not possible, as uv__read and the API are disconnected. (May be I am missing something here?)
Model (1) is possible.
Turn on a flag (largesocket) if the API is called.
At the I/O site (), use the new size, if the flag is on.
Please suggest. Thanks again!

I really don't want this in v1.

gireeshpunathil · 2017-06-02T12:16:22Z

I don't want libuv to make assumptions abed on a single metric.

The metric we collected was through an iterative approach of refining a streaming test case and collecting evidence from multiple platforms (#1217). Do you find gaps in that?

Search in the node.js repo revealed that http parser is the only entity which over-rides the suggested_size - but the over-ride itself is 64 KB - which means the maximum data transferred in one shot in the current libuv is 64KB. I understand that node is not the only consumer of libuv, so I park my point about granular control over read chunks on a per I/O basis.

I really don't want this in v1.

I am sorry to hear that! Do you believe the proposal is not technically feasible in v1? Or do you think it may adversely affect some existing usage patterns that are not covered in our metric? Please provide a rationale.

saghul · 2017-06-02T14:07:44Z

The metric we collected was through an iterative approach of refining a streaming test case and collecting evidence from multiple platforms (#1217). Do you find gaps in that?

Search in the node.js repo revealed that http parser is the only entity which over-rides the suggested_size - but the over-ride itself is 64 KB - which means the maximum data transferred in one shot in the current libuv is 64KB. I understand that node is not the only consumer of libuv, so I park my point about granular control over read chunks on a per I/O basis.

Sorry if I sounded too harsh on first reply, I really appreciate your work here and re-reading my comment I realize I sounded pretty bad, apologies.

What I meant is that (if I understood correctly) while many systems and values were measured, the scenario was just one: HTTP traffic. Is this correct? My additional point is that while the ideal buffer size could be X, there might be other reasons why the user needs to use Y, based on memory allocation constraints, for example.

I am sorry to hear that! Do you believe the proposal is not technically feasible in v1? Or do you think it may adversely affect some existing usage patterns that are not covered in our metric? Please provide a rationale.

Again, sorry for being too terse before. My reasoning is that if we are to remove half of the feature in v2, I don't want it in v1. It's more churn for those who start depending on it.

Now, we could land the v2 API into v1, even if alloc_cb exists, since it just provides a suggested size, so the user would be free to ignore it and use the new API to query the best possible value.

sam-github · 2017-06-02T14:19:24Z

the scenario was just one: HTTP traffic. Is this correct?

As I understand it the primary scenario is actually streaming TCP data not HTTP, for example video apps like netflix.

gireeshpunathil · 2017-06-10T14:52:42Z

@saghul ,
Thanks for the clarification. No, you haven't been terse - I see it as an assertive view that was required to balance a PR between two disparate code bases.

So thinking in terms implementing this as an API in v1 - I reviewed the node code and formed a revised (and a more reasonable I think) proposal - which I guess is aligned to your considerations:

Modify the API from uv_tcp_enable_largesocket() to uv_tcp_rfc1323_size()
The API does not modify socket / stream attributes, instead just retrieves the size
For v1 or v2, the API remains same, and remains to be the only addition to libuv
For v1, interested callers use the API to get system-capable size, and then apply it at an I/O through allocation callback, as applicable.
For v2, interested callers use the API to get system-capable size, and then apply it at an I/O site. We will have more clarity here when the v2 I/O site is materialized, or is it already?

Benefits of this approach are (i) No modifications to this feature in libuv when we go from v1 to v2. (ii) change in consumption of the API between v1 and v2 will be contained by the change introduced due to alloc_cb removal.

Please share your thoughts, thanks.

regarding the workload, yes - the primary scenario is TCP data streaming.

stale · 2019-08-10T01:51:46Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

santigimeno requested changes Mar 29, 2017

View reviewed changes

gireeshpunathil force-pushed the largesocket branch from 6206358 to 7d5a23c Compare March 29, 2017 18:28

santigimeno requested changes Mar 30, 2017

View reviewed changes

santigimeno added the v2 label Mar 30, 2017

gireeshpunathil force-pushed the largesocket branch 3 times, most recently from 7d952a1 to ba850c7 Compare April 2, 2017 08:06

santigimeno requested changes Apr 2, 2017

View reviewed changes

gireeshpunathil force-pushed the largesocket branch from ba850c7 to 6a969da Compare April 3, 2017 09:37

gireeshpunathil mentioned this pull request Apr 7, 2017

Request for temporary access to BSD systems nodejs/build#677

Closed

gireeshpunathil force-pushed the largesocket branch from 6a969da to a7d9b1b Compare April 12, 2017 05:35

sam-github reviewed Jun 1, 2017

View reviewed changes

cjihrig removed the v2 label Apr 10, 2018

stale Bot added the stale label Aug 10, 2019

stale Bot closed this Aug 17, 2019

mmomtchev mentioned this pull request Jun 19, 2021

Expose highWaterMark in http nodejs/node#39092

Closed


		.. versionchanged:: 1.4.0 UNIX implementation added.

		.. c:function:: int uv_enable_largesocket(uv_stream_t* handle)

Uh oh!

Conversation

gireeshpunathil commented Mar 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

santigimeno Mar 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gireeshpunathil commented Mar 29, 2017

Uh oh!

txdv commented Mar 30, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

gireeshpunathil commented Mar 29, 2017 •

edited

Loading

santigimeno Mar 29, 2017 •

edited

Loading