Can you also add a test which uses an offset other than -1?

Hi all, sorry for the late response, I indeed think this PR should have landed some time ago.
I did have some emailthread going on with @bnoordhuis but this was rather going slow. Anyway I think continuing the discussion here is much more meaningful, so here it goes.

Regarding my own pull request I have the following remarks:
First of all, for my own implementation (the original reason for #260) it is enough to have public access to 'uv__getiovmax()' With a publicly available uv__getiovmax() a user could just fill the right (maximum) amount of buffers and issue a write.
Secondly, POSIX states that writev should be atomic as @ronkorving also already noted.

For the first issue I propose a publicly available uv__getiovmax defined as:

int uv__getiovmax() {
#if defined(_SC_IOV_MAX)
 static int iovmax=-1;
 if(iovmax<0) sysconf(_SC_IOV_MAX);
 return iovmax;
#elif defined(IOV_MAX)
 return IOV_MAX;
#else
 return 1024;
#endif
}

Swapping the _SC_IOV_MAX and IOV_MAX ifdefs
And reintroducing the limits.h include with __need_IOV_MAX

With this solution any users needing to send more than getiovmax() buffers could do this in a loop.
So staying under this limit or choosing to go over 'knowing' it will most certainly hurt performance is up to the users... (@ronkorving: Would this be a viable solution for node.js, meaning node.js will provide the loop to stay under the limit?)

To resolve the second issue, I think my solution should be refactored into a glibc/writev solution:

• calculate total combined size of all buffers
• allocate memory
• copy all buffers to alloc'd memory
• call write with alloc'd memory

I am willing to provide a solution for this, but my time is really limited, so if anyone wants to beat me to it, you're most welcome!

hey @bwijen glad to see you return to this work! this and/or your original PR is really important, whichever one eventually lands! 👍

@bwijen Welcome back, and thanks for your feedback. Is this joined feedback from @bnoordhuis and yourself based on your e-mail correspondence?

I understand that leaving it to the user to loop may seem advantageous, although in the case of Node.js I think it will make no difference at all (as far as I can see, it won't be able to leverage its potential advantage given the current state of writable streams.. but I may be wrong and input from someone like @bnoordhuis would be most appreciated).

As for the suggested implementation detail of moving the loop to Node-land, why no doubt possible, it would be a massive pain to do it from C++, because of the asynchronous chaining it would cause (multiple req objects that need to be sequentially executed one after the other). So the only alternative really becomes to do this from JavaScript. Since the current implementation is 100% in C++ land, this will most certainly cost us in performance, as well as add complexity (unless someone else sees another way out?). I do assume that one of the design goals of libuv is to take care of some level of complexity, so its users don't suffer that complexity.

You mention "To resolve the second issue". Are you talking about atomicity? Your suggestion sounds like a very painful one (in terms of memory used and copied around). I'm also not sure if you're suggesting to actually land this in glibc (I doubt that's realistic, but that's how it reads).

In any case. How about a best-of-both-worlds approach? Expose IOV_MAX to user-land, so they have the freedom to use its advice, but they may choose to ignore it and leave it to libuv to write all the available buffers in one single C++ loop (as your PR does).

With this solution any users needing to send more than getiovmax() buffers could do this in a loop.

I don't really like this so much. What's the real benefit? We already have a precedent of doing it internally (see streams), so I don't see why we shouldn't do the same for fs operations.

To resolve the second issue, I think my solution should be refactored into a glibc/writev solution:

calculate total combined size of all buffers
allocate memory
copy all buffers to alloc'd memory
call write with alloc'd memory

There shouldn't be a need for that. The contract is that as long as the completion callback hasn't been called, libuv holds on to the buffers. We should never copy the content of those buffers within libuv.

Quick note: "this or that makes things easier for Node" is not a good justification, this is libuv, separate project. If there is something we need to change / expose, we will, but it needs merits of its own.

Unless there are issues I haven't seen, this PR sounds like the right thing to do, as we already do for uv_stream_t.

I agree that Node is not the goal. However, it can be used to present real life use cases (just like Node is a real life use case for V8, and it hurts when the V8 team makes breaking changes that have a lot of collateral cost for the Node team).

In any case, it seems you (@saghul) do think this PR is the right approach, so we are on the same page. I think it's libuv's job (node or no node) to present a friendly interface to the user, and hide the complicated parts and OS-specific limitations of doing I/O. And indeed, it's not like streams use a different approach.

I will move forward by addressing the issues exposed in this PR by @saghul. If in the future anyone decides that this PR is philosophically incorrect, I encourage improvement, but there's imho nothing wrong with iteration. I have to admit that I'm new to libuv, so I'm not really familiar with the who's-who and who inevitably is likely to decide on whether to merge this (which right now makes me feel a bit like between a rock and hard place when receiving contradicting feedback).

I agree that Node is not the goal. However, it can be used to present real life use cases (just like Node is a real life use case for V8, and it hurts when the V8 team makes breaking changes that have a lot of collateral cost for the Node team).

My note was just a hint not to derail the discussion, of course I care about not breaking Node :-)

I have to admit that I'm new to libuv, so I'm not really familiar with the who's-who and who inevitably is likely to decide on whether to merge this (which right now makes me feel a bit like between a rock and hard place when receiving contradicting feedback).

We encourage open collaboration, so everyone is more than welcome to contribute to the discussion. As for merging the actual PR or making final design / API decisions, both @bnoordhuis and I are libuv Core Janitors (there are more, check the contributing guide, but he and I are the ones involved in this discussion at the moment).

Thank you for the feedback 👍

Thinking about this again (and reading the feedback) I agree that this is indeed the way to go.
I still think my first issue holds, I would really appreciate a public api to uv__getiovmax()
For the second issue I have to admit, that libuv has no need to be atomic. (@bnoordhuis: had already pointed this out, but I wasn't convinced at the time.)

@ronkorving: So what's next? We have now have both your #448 and my #269 I have no problem with withdrawing mine so we can all focus on yours...

I would really appreciate a public api to uv__getiovmax()

Lets discuss that in another issue / PR (feel free to open one), it doesn't need to be tied to this one.

@bwijen I have time to work on this and get it into shape. You deserve all the credit for the fix, but I really don't mind taking it to the finish line. It's a great learning experience for me too. But it's your baby and thus your call. All I would ask for is: sooner rather than later :)

@ronkorving I have closed #269, so lets focus on this one as of now...

Understood 👍

@saghul speaking of tests.. any idea why make check passes, while the test fs_write_alotof_bufs seems should break?

@ronkorving why should it break? This PR should fix it, shouldn't it?

I mean it doesn't even break on the v1.x branch (I'm on OSX btw).

So, you tested the test without the rest of the patch? No idea, cannot try it right now, sorry.

When I checkout v1.x, the test (that was already in place passes). From the looks of it, it shouldn't pass at all. Maybe the test is broken.

I think you have mixed up your branches. There is no such a test in the v1.x branch. This very PR adds it.

Sigh. It must be Wednesday. I never could get the hang of Wednesdays.

Looking good! Can you please add another test which uses some initial offset please? Thanks!

@ronkorving nope, it succeeds! We don't quite have a green build, but your test is passing AFAIS. Good job! Can you please squash all your commits into one and rebase if necessary?

Oh really? Hurray! About to catch to some sleep, but will do that in about 11 hours. Thanks for everything @saghul!

@saghul squashed the lot.

@ronkorving I still see 5 commits. Please make it one and adjust the commit log to our guidelines in CONTRIBUTING.md. Thanks!

Oh, I thought you wanted to keep @bwijen's the way they were, as you said "squash all your commits" (emphasis added by me). I guess I took that a bit too literal. Give me a moment, and I'll try to squash the whole bunch.

That's it, everything in a single commit.

I messed up, give me a moment.

All done.

CI: https://jenkins-iojs.nodesource.com/view/libuv/job/libuv+any-pr+multi/126/

Are we there yet? :) CI looks promising.

We are. LGTM, landing now.

Landed in ✨ 2bf7827 ✨ Thanks a lot @ronkorving and @bwijen!

\o/ Thanks guys!
Looking forward to the next release :)