Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

net: add tcp WriteTo func to enable splice socket data to file #48530

Open
jim3ma opened this issue Sep 22, 2021 · 8 comments
Open

net: add tcp WriteTo func to enable splice socket data to file #48530

jim3ma opened this issue Sep 22, 2021 · 8 comments

Comments

@jim3ma
Copy link

@jim3ma jim3ma commented Sep 22, 2021

When transfer data from tcp socket to file directly, splice will help us with low time cost in linux. Currently, os.File just uses write to transfer data from tcp socket.

To do this in linux, there are some changes:

  1. add LimitWriter to detect writing limit in internal package, discussed in this issues.
  2. add spliceW function to splice data from tcp socket to file.
  3. add WriteTo function for net.TCPConn.

We get 10% at least cpu time reduce, zero memory copy from kernel to user space.

image

Implement PR: #46149, go review

@gopherbot gopherbot added this to the Proposal milestone Sep 22, 2021
@ianlancetaylor ianlancetaylor added this to Incoming in Proposals Sep 22, 2021
@rsc
Copy link
Contributor

@rsc rsc commented Oct 6, 2021

What are the specific API changes you are proposing?

@rsc
Copy link
Contributor

@rsc rsc commented Oct 6, 2021

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc rsc moved this from Incoming to Active in Proposals Oct 6, 2021
@rsc
Copy link
Contributor

@rsc rsc commented Oct 20, 2021

It looks like there are no direct API changes other than TCPConn implementing io.WriterTo by adding a WriteTo method.
This means that any io.Copy(dst, src) where src is a TCPConn
and dst has a ReadFrom will now use src.WriteTo instead of dst.ReadFrom.
Are there are any cases where that matters?
If dst and src are both networks, presumably we end up at splice either way.
But are there other kinds of dsts that matter?

If splice isn't applicable to this specific TCPConn,
we fall back to io.Copy(dst, noWriteTo{src}),
so we will still get to the dst.ReadFrom.

So the only possible change is when splice is an option but not the best option. Does that case exist?

@rsc
Copy link
Contributor

@rsc rsc commented Oct 27, 2021

If we do find such a case, TCPConn.WriteTo can always not use splice then.

@rsc rsc moved this from Active to Likely Accept in Proposals Oct 27, 2021
@rsc
Copy link
Contributor

@rsc rsc commented Oct 27, 2021

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

@jim3ma
Copy link
Author

@jim3ma jim3ma commented Oct 28, 2021

It looks like there are no direct API changes other than TCPConn implementing io.WriterTo by adding a WriteTo method. This means that any io.Copy(dst, src) where src is a TCPConn and dst has a ReadFrom will now use src.WriteTo instead of dst.ReadFrom. Are there are any cases where that matters? If dst and src are both networks, presumably we end up at splice either way. But are there other kinds of dsts that matter?

If splice isn't applicable to this specific TCPConn, we fall back to io.Copy(dst, noWriteTo{src}), so we will still get to the dst.ReadFrom.

So the only possible change is when splice is an option but not the best option. Does that case exist?

Currently, I just find two cases:

  • From net.TCPConn to os.File
  • From net.TCPConn to net.TCPConn

Two cases has been handled in https://go-review.googlesource.com/c/go/+/319593

@rsc rsc moved this from Likely Accept to Accepted in Proposals Nov 3, 2021
@rsc
Copy link
Contributor

@rsc rsc commented Nov 3, 2021

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

@rsc rsc changed the title proposal: net: add tcp WriteTo func to enable splice socket data to file net: add tcp WriteTo func to enable splice socket data to file Nov 3, 2021
@rsc rsc removed this from the Proposal milestone Nov 3, 2021
@rsc rsc added this to the Backlog milestone Nov 3, 2021
@jim3ma
Copy link
Author

@jim3ma jim3ma commented Nov 28, 2021

Optimized http get code based on enable splice socket data to file: https://github.com/jim3ma/go/tree/dev.http.writeto.1.16-2021-11-28

Typical testing result(download 1 Gigabytes using http.Get):

echo 3 > /proc/sys/vm/drop_caches; time ./httpget-optmized; rm -f test.output;
copied bytes: 1048576000
./httpget-optmized  0.01s user 0.29s system 74% cpu 0.403 total

echo 3 > /proc/sys/vm/drop_caches; time ./httpget-no-optmized; rm -f test.output;
copied bytes: 1048576000
./httpget-no-optmized  0.02s user 0.39s system 94% cpu 0.436 total

testing code:

package main

import (
	"fmt"
	"io"
	"net/http"
	"os"
)

func main() {
	req, err := http.NewRequest(http.MethodGet, "http://127.0.0.1:3000/test.data", nil)
	if err != nil {
		panic(err)
	}
	resp, err := http.DefaultClient.Do(req)
	if err != nil {
		panic(err)
	}
	defer resp.Body.Close()
	f, err := os.OpenFile("test.output", os.O_CREATE|os.O_RDWR, 0644)
	if err != nil {
		panic(err)
	}
	n, err := io.Copy(f, resp.Body)
	if err != nil {
		panic(err)
	}

	fmt.Printf("copied bytes: %d\n", n)
}

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

No branches or pull requests

3 participants