Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Put FD's on sockets, or remove fs.sendfile #3854

Closed
isaacs opened this Issue Aug 11, 2012 · 17 comments

Comments

Projects
None yet
5 participants

isaacs commented Aug 11, 2012

sendfile(2) can only send data from a file fd to a socket fd.

However, since we don't expose the fd on socket handles, so having fs.sendfile is confusing and worthless.

Owner

bnoordhuis commented Aug 11, 2012

fs.sendfile() needs to be replaced by something better. What we need is a smarter approach for sending data from one stream to another.

isaacs commented Aug 11, 2012

I agree, of course, and it should probably be added as an implementation detail in the pipe() method of the streams involved, so that users just use readable.pipe(writable) and we do all the magic for them behind the scenes.

But in the meantime, why even have fs.sendfile, if you can't actually do anything with it? Maybe we could let it take a libuv handle instead of a socket fd?

Owner

bnoordhuis commented Aug 12, 2012

Maybe we could let it take a libuv handle instead of a socket fd?

Reasonable but not trivial to implement unless you resort to platform-specific #ifdef hackery.

uv_fs_sendfile() takes an int so you'd have to extract the relevant fd from the libuv handle on Unices. Not pretty but possible and reasonably straightforward.

It gets gnarlier on Windows - uv-win handles aren't backed by file descriptors.

As a stop-gap measure we could implement the above and say it's broken on Windows.

isaacs commented Aug 12, 2012

Is uv_fs_sendfile even implemented on windows in any kind of system-specific way? It looks like fs__sendfile just reads from one and writes to the other. There's probably no reason why we couldn't do that with a handle.

Owner

bnoordhuis commented Aug 12, 2012

@piscisaureus You should chime in here as well.

Member

piscisaureus commented Aug 12, 2012

@bnoordhuis Tomorrow :-)

Member

piscisaureus commented Aug 13, 2012

We could make it work on all platforms (it's not even very difficult), but it's pointless. Using sendfile() has virtually no benefits in the current setup.

Owner

bnoordhuis commented Aug 13, 2012

Okay, so let's remove it?

Member

piscisaureus commented Aug 13, 2012

drop it!

isaacs commented Aug 14, 2012

@piscisaureus It has virtually no benefits on Windows, but on Unix, it prevents a copy out of kernel space, doesn't it?

Of course, fs.sendfile it hasn't really ever worked properly without ridiculous hoop-jumping, and has been completely broken for two stable branches now, and life seems to be going on. I'd be ok with challenging any feature-requesters to provide some benchmarks to justify the performance benefits.

Member

piscisaureus commented Aug 14, 2012

@isaacs On windows you can easily implement sendfile using the TransmitFile api. The problem however is that you have to block the thread pool until the file has been written, or go back to the thread pool often. We need a good story for this - but naive sendfile isn't that.

isaacs commented Dec 20, 2012

Ok, no one is using this, and making it work will require a rewrite. Let's remove it for 0.10.

Owner

bnoordhuis commented Dec 28, 2012

Axed in 910e24b.

c4milo commented Mar 7, 2013

FWIW I ran across this: https://www.myricom.com/software/myri10ge/440-why-don-t-you-report-tcp-sendfile-performance-for-macosx-on-your-myri10ge-performance-webpage.html

Unfortunately, MacOSX does not have this optimization, so TCP_SENDFILE is actually more expensive on MacOSX. This is because the sendfile() system call on MacOSX copies data from the page cache into special network buffers. Hence, there is no point in benchmarking TCP_SENDFILE when the sender is running MacOSX, as the results will be the same as, or slightly worse than, TCP_STREAM. Hence their omission from our results for MacOSX.

c4milo commented Mar 7, 2013

it also looks like sendfile() could be tweaked using TCP_CORK for network transfers, to get the best out of it. See: http://baus.net/on-tcp_cork

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment