search

containerd / fifo

fifo pkg for Go
https://containerd.io
Apache License 2.0
87 stars 30 forks source link

Add ReaderFrom/WriterTo for Linux (splice) #29

Open cpuguy83 opened 3 years ago

cpuguy83 commented 3 years ago

On Linux, we can use splice to optimize copies to happen in kernel when copying to other files. Since this is already a pipe, this should always work when copying to/from another file.

cpuguy83 commented 3 years ago

Updated this, added some new test cases and improved the implementation slightly:

  1. EINTR handling was breaking out of the loop instead of just retrying - fixed
  2. Before the max amount that could be copied before the new methods would return was 1<<62... which is a lot... but is not expected since we want to copy until EOF (splice copies 0 bytes), so the internal copy takes a special value, -1, to mean copy to EOF. This is important because we have special handling for when the reader passed in to ReadFrom is an *io.LimitedReader and this was originally mixing "amount to copy" with "amount remaining to copy".
cpuguy83 commented 3 years ago

I'm going to do some more testing on this as well.

cpuguy83 commented 3 years ago

FYI, ended up making a new repo to play around with this a bit more, benchmarks, etc: https://github.com/cpuguy83/pipes Somewhat suprisingly I'm seeing an approx 2x speedup using splice.

I've made some changes to the implementation that make it a bit cleaner which I'll move here as well.

thaJeztah commented 3 years ago

Somewhat suprisingly I'm seeing an approx 2x speedup using splice.

Silly question; the benchmark in the readme doesn't show a delta; is that because some option wasn't set?

(Performance improvement sounds great though!)

cpuguy83 commented 3 years ago

Hmm I'm not sure. I hadn't used benchstat before... benchcmp shows the delta.

thaJeztah commented 2 years ago

@cpuguy83 @kzys @dmcgowan what's the status on this one? Do we want to have this merged and included in a release?

I went looking what changes are in main that are not yet released (following https://github.com/containerd/fifo/pull/32#issuecomment-1023065555), so was looking if we wanted to include this PR as well if we would be doing a new release.

kzys commented 2 years ago

Sorry. I have missed the ping. Let me take a look next week.