Add ReaderFrom/WriterTo for Linux (splice) #29

cpuguy83 · 2020-11-21T06:16:24Z

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.

dmcgowan · 2020-11-24T20:09:51Z

raw_linux.go

+					// splice not supported on kernel
+					atomic.StoreInt32(&spliceSupported, 0)
+					return true
+				case syscall.EINVAL, syscall.EOPNOTSUPP, syscall.EPERM:


Would syscall.EOPNOTSUPP here indicate the process is unable to perform the syscall regardless of whether the kernel supports it? If this doesn't depend on input would it make sense to treat it like ENOSYS?

I think EOPNOTSUP is more about the transport than the actual system.
For instance, I'd expect EOPNOTSUP if the read side is a unix socket which does not support splicing... but I have not tested this... it may actually be worth adding tests for this.

Although the specific case may actually be EINVAL...

That's fair, its not super clear when these standard return values would be returned in this case and better not to set a global value

raw_linux.go

raw_linux_test.go

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. Signed-off-by: Brian Goff <[email protected]>

cpuguy83 · 2021-07-30T17:33:16Z

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

EINTR handling was breaking out of the loop instead of just retrying - fixed
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 · 2021-07-30T22:12:51Z

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

cpuguy83 · 2021-08-14T19:21:20Z

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 · 2021-08-14T19:38:20Z

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 · 2021-08-14T22:30:36Z

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

thaJeztah · 2022-01-27T10:43:23Z

@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 #32 (comment)), so was looking if we wanted to include this PR as well if we would be doing a new release.

kzys · 2022-02-19T17:47:43Z

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

samuelkarp

A few questions and nits, but otherwise LGTM.

samuelkarp · 2022-09-29T01:32:08Z

raw_linux.go

+	select {
+	case <-f.opened:
+		return f.readFrom(r)
+	default:
+	}
+	select {
+	case <-f.opened:
+		return f.readFrom(r)


What's the purpose of trying to read from <-f.opened twice here?

raw_linux.go

samuelkarp · 2022-09-29T01:40:42Z

raw_linux.go

+	if !ok {
+		return copyBuffer(f.file, r)
+	}


Does this error case also need to handle lr != nil?

Suggested change

if !ok {

return copyBuffer(f.file, r)

}

if !ok {

if lr != nil {

r = lr

}

return copyBuffer(f.file, r)

}

samuelkarp · 2022-09-29T01:42:01Z

raw_linux.go

+		remain = spliceMax
+	}
+
+	// Hear the RawConn Read/Write methods allow us to utilize the go runtime


nit

Suggested change

// Hear the RawConn Read/Write methods allow us to utilize the go runtime

// Here the RawConn Read/Write methods allow us to utilize the go runtime

samuelkarp · 2022-09-29T01:49:35Z

raw_linux.go

+				case nil:
+					handled = true
+					if n == 0 {
+						// At EOF
+						return true
+					}
+				case unix.EINTR:
+					continue


nit: I think it'd be slightly more readable to have an explicit continue in here.

Suggested change

case nil:

handled = true

if n == 0 {

// At EOF

return true

}

case unix.EINTR:

continue

case nil:

handled = true

if n == 0 {

// At EOF

return true

}

continue

case unix.EINTR:

continue

Also, what do you think about reordering the switch to order successful cases before errors and have all the errors grouped together? Something like nil, unix.EINTR, unix.EAGAIN, unix.ENOSYS, syscall.EINVAL, syscall.EOPNOTSUPP, syscall.EPERM, default?

samuelkarp · 2022-09-29T01:50:35Z

raw_linux.go

+	select {
+	case <-f.opened:
+		return f.writeTo(w)
+	default:
+	}
+
+	select {
+	case <-f.opened:
+		return f.writeTo(w)


Same question here about the repeated read from f.opened.

samuelkarp · 2022-09-29T01:51:30Z

raw_linux_test.go

+	data := strings.Repeat("This is a test, this is only a test.", 1000)
+
+	// For these test cases we only call ReadFrom and validate there is no error and the
+	// amouont of data it copied is what we put into it.


nit

Suggested change

// amouont of data it copied is what we put into it.

// amount of data it copied is what we put into it.

cpuguy83 requested review from AkihiroSuda, dmcgowan and tonistiigi November 21, 2020 06:16

cpuguy83 changed the title ~~Add ReaderFrom/WriterTo for Linux~~ Add ReaderFrom/WriterTo for Linux (splice) Nov 21, 2020

cpuguy83 force-pushed the add_readerfrom_writerto branch 5 times, most recently from 0a81acb to abf9d52 Compare November 21, 2020 06:32

dmcgowan reviewed Nov 24, 2020

View reviewed changes

thaJeztah reviewed Jan 29, 2021

View reviewed changes

raw_linux.go Outdated Show resolved Hide resolved

kzys reviewed Jul 26, 2021

View reviewed changes

raw_linux.go Outdated Show resolved Hide resolved

raw_linux_test.go Outdated Show resolved Hide resolved

cpuguy83 force-pushed the add_readerfrom_writerto branch 2 times, most recently from 6e8e675 to 2e799de Compare July 30, 2021 00:39

Add ReaderFrom/WriterTo for Linux

19b9833

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. Signed-off-by: Brian Goff <[email protected]>

cpuguy83 force-pushed the add_readerfrom_writerto branch from 2e799de to 19b9833 Compare July 30, 2021 17:21

samuelkarp approved these changes Sep 29, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ReaderFrom/WriterTo for Linux (splice) #29

Add ReaderFrom/WriterTo for Linux (splice) #29

cpuguy83 commented Nov 21, 2020

dmcgowan Nov 24, 2020

cpuguy83 Nov 24, 2020

cpuguy83 Nov 24, 2020

dmcgowan Nov 25, 2020

cpuguy83 commented Jul 30, 2021

cpuguy83 commented Jul 30, 2021

cpuguy83 commented Aug 14, 2021

thaJeztah commented Aug 14, 2021

cpuguy83 commented Aug 14, 2021

thaJeztah commented Jan 27, 2022

kzys commented Feb 19, 2022

samuelkarp left a comment

samuelkarp Sep 29, 2022

samuelkarp Sep 29, 2022

samuelkarp Sep 29, 2022

samuelkarp Sep 29, 2022

samuelkarp Sep 29, 2022

samuelkarp Sep 29, 2022

	// Hear the RawConn Read/Write methods allow us to utilize the go runtime
	// Here the RawConn Read/Write methods allow us to utilize the go runtime

	// amouont of data it copied is what we put into it.
	// amount of data it copied is what we put into it.

Add ReaderFrom/WriterTo for Linux (splice) #29

Are you sure you want to change the base?

Add ReaderFrom/WriterTo for Linux (splice) #29

Conversation

cpuguy83 commented Nov 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpuguy83 commented Jul 30, 2021

cpuguy83 commented Jul 30, 2021

cpuguy83 commented Aug 14, 2021

thaJeztah commented Aug 14, 2021

cpuguy83 commented Aug 14, 2021

thaJeztah commented Jan 27, 2022

kzys commented Feb 19, 2022

samuelkarp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment