Skip to content
This repository

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

Closed
isaacs opened this Issue · 17 comments

5 participants

Isaac Z. Schlueter Ben Noordhuis Bert Belder James M. Greene Camilo Aguilar
Isaac Z. Schlueter
Collaborator

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.

Ben Noordhuis

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

Isaac Z. Schlueter
Collaborator

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?

Ben Noordhuis

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.

Isaac Z. Schlueter
Collaborator

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.

Ben Noordhuis

@piscisaureus You should chime in here as well.

Bert Belder
Collaborator

@bnoordhuis Tomorrow :-)

Bert Belder
Collaborator

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.

Ben Noordhuis

Okay, so let's remove it?

Bert Belder
Collaborator

drop it!

Isaac Z. Schlueter
Collaborator

@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.

Bert Belder
Collaborator

@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.

Isaac Z. Schlueter
Collaborator

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

Ben Noordhuis bnoordhuis closed this issue from a commit
Ben Noordhuis bnoordhuis fs: remove fs.sendfile()
Said function has been broken (and useless) since v0.6.0. Remove it altogether.

Fixes #3854.
910e24b
Ben Noordhuis

Axed in 910e24b.

Camilo Aguilar

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.

Camilo Aguilar

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
Something went wrong with that request. Please try again.