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

proposal: net, os, syscall, internal/poll: Add ControlWith, ReadWith, WriteWith to RawConn interface #51170

Closed
diogin opened this issue Feb 13, 2022 · 11 comments

Comments

@diogin
Copy link
Contributor

diogin commented Feb 13, 2022

The RawConn interface enables us controlling, reading, and writing net.Conn and os.File with direct syscalls. Currently it is defined as:

type RawConn interface {
	// Control invokes f on the underlying connection's file
	// descriptor or handle.
	// The file descriptor fd is guaranteed to remain valid while
	// f executes but not after f returns.
	Control(f func(fd uintptr)) error

	// Read invokes f on the underlying connection's file
	// descriptor or handle; f is expected to try to read from the
	// file descriptor.
	// If f returns true, Read returns. Otherwise Read blocks
	// waiting for the connection to be ready for reading and
	// tries again repeatedly.
	// The file descriptor is guaranteed to remain valid while f
	// executes but not after f returns.
	Read(f func(fd uintptr) (done bool)) error

	// Write is like Read but for writing.
	Write(f func(fd uintptr) (done bool)) error
}

This definition has some inconvenience:

  1. Cannot pass more arguments to f, which may be needed by f. We have to pass a function literal as f, which binds context parameters inside it,
  2. Any error occurred in f cannot be captured directly, we have to store the error to outer variable err through a function literal (closure),
  3. Function literals escape to heap if they refer to outer variables.

To overcome these inconvenience, and keep backward compatible, I propose adding these 3 methods to RawConn interface:

ControlWith(f func(fd uintptr, arg unsafe.Pointer) error, arg unsafe.Pointer) error

ReadWith(f func(fd uintptr, arg unsafe.Pointer) (done bool, err error), arg unsafe.Pointer) error

WriteWith(f func(fd uintptr, arg unsafe.Pointer) (done bool, err error), arg unsafe.Pointer) error

Here, we add another argument, named arg, to f, so we can pass more info needed by f. We also add an error to the returned value of f to capture its errors, which is returned directly by these 3 methods as error, so no function literals are needed. Meanwhile, the real argument is passed to these 3 methods in addition to f, which is in turn passed to f when calling f in the implementation of these 3 methods.

By doing this, we eliminate all of the inconvenience, and pprof shows the function literals don't escape to heap while it can still get what it wants, improving the performance.

This proposal affects package net, os, syscall, and internal/poll, but the effect is small and limited. The main implementation is in internal/poll. Taking WriteWith (in package internal/poll, it will be called RawWriteWith) as an example:

func (fd *FD) RawWriteWith(f func(uintptr, unsafe.Pointer) (bool, error), arg unsafe.Pointer) error {
	if err := fd.writeLock(); err != nil {
		return err
	}
	defer fd.writeUnlock()
	if err := fd.pd.prepareWrite(fd.isFile); err != nil {
		return err
	}
	for {
		if done, err := f(uintptr(fd.Sysfd), arg); done {
			return err
		}
		if err := fd.pd.waitWrite(fd.isFile); err != nil {
			return err
		}
	}
}

The semantics of these methods are consistent with existing Control, Read, and Write. The design is also consistent.

Any thoughts?

@gopherbot gopherbot added this to the Proposal milestone Feb 13, 2022
@ianlancetaylor
Copy link
Contributor

The current usage doesn't seem like a big problem in practice. Can you point to code examples that would clearly benefit from this approach?

If we do this, then I don't think we should use unsafe.Pointer. We should use any.

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) Feb 13, 2022
@diogin
Copy link
Contributor Author

diogin commented Feb 14, 2022

The main benefit is that the function literal does not escape to heap. An example of code is in net/sendfile_linux.go:

var werr error
err = sc.Read(func(fd uintptr) bool {
	written, werr = poll.SendFile(&c.pfd, int(fd), remain)
	return true
})
if err == nil {
	err = werr
}

Here, in the function literal, 4 outer variables causes the function to escape: written, werr, &c.pfd, and remain.
If written, &c.pfd, and remain are combined into a struct and passed into f, and werr is returned directly, then this function literal won't escape:

// for demonstration only. in bigger programs, argType may be part of the containing type
type argType {
	written *int64
	pfd     *poll.FD
	remain  int64
}

// for demonstration only. in bigger programs, arg may be a parameter of the containing type
arg := &argType{&written, &c.pfd, remain}

err = sc.ReadWith(func(fd uintptr, arg unsafe.Pointer) (bool, error) {
	a := (*argType)(arg)
	*a.written, werr := poll.SendFile(a.pfd, int(fd), a.remain)
	return true, werr
}, unsafe.Pointer(arg))

Yes, in practice, this is not a big problem, but if we need to eliminate the unnecessary memory allocation caused by the escape of the function literal, at least this provides a way, so is meaningful. With previous APIs, we can't eliminate the memory allocation, as long as we refer something outside of the function literal, it always escape.

For the type of arg, any should be a better option to unsafe.Pointer.

@ianlancetaylor
Copy link
Contributor

I wonder why escape analysis isn't good enough here?

@diogin
Copy link
Contributor Author

diogin commented Feb 15, 2022

I have no idea. The calling path is sendFile -> Read -> RawRead:

func sendFile(c *netFD, r io.Reader) (written int64, err error, handled bool) {
	var remain int64 = 1 << 62 // by default, copy until EOF

	lr, ok := r.(*io.LimitedReader)
	if ok {
		remain, r = lr.N, lr.R
		if remain <= 0 {
			return 0, nil, true
		}
	}
	f, ok := r.(*os.File)
	if !ok {
		return 0, nil, false
	}

	sc, err := f.SyscallConn()
	if err != nil {
		return 0, nil, false
	}

	var werr error
	err = sc.Read(func(fd uintptr) bool {
		written, werr = poll.SendFile(&c.pfd, int(fd), remain)
		return true
	})
	if err == nil {
		err = werr
	}

	if lr != nil {
		lr.N = remain - written
	}
	return written, wrapSyscallError("sendfile", err), written > 0
}

func (c *rawConn) Read(f func(uintptr) bool) error {
	if err := c.file.checkValid("SyscallConn.Read"); err != nil {
		return err
	}
	err := c.file.pfd.RawRead(f)
	runtime.KeepAlive(c.file)
	return err
}

// RawRead invokes the user-defined function f for a read operation.
func (fd *FD) RawRead(f func(uintptr) bool) error {
	if err := fd.readLock(); err != nil {
		return err
	}
	defer fd.readUnlock()
	if err := fd.pd.prepareRead(fd.isFile); err != nil {
		return err
	}
	for {
		if f(uintptr(fd.Sysfd)) {
			return nil
		}
		if err := fd.pd.waitRead(fd.isFile); err != nil {
			return err
		}
	}
}

Pprof shows these 3 lines cause memory allocation, all of which are in function sendFile:

func sendFile(c *netFD, r io.Reader) (written int64, err error, handled bool) {
var werr error
err = sc.Read(func(fd uintptr) bool {

These allocations cause noticeable performance reduction on benchmarking.
Of course, if the escape analysis is good enough in this situation, then we don't need to add more methods to RawConn.

@rsc
Copy link
Contributor

rsc commented Feb 16, 2022

If the problem is escape analysis, it sounds like we should fix that for the code snippets above.
The compiler knows the underlying type of the result of f.SyscallConn().
A smarter compiler is always better than new API.

@rsc
Copy link
Contributor

rsc commented Feb 16, 2022

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 (old) Feb 16, 2022
@diogin
Copy link
Contributor Author

diogin commented Feb 17, 2022

Thanks! If a change to compiler is feasible in this case, it should be a better choice to solve this issue.

@rsc
Copy link
Contributor

rsc commented Feb 23, 2022

Filed #51334 for the compiler change. This seems like a likely decline.

@rsc rsc moved this from Active to Likely Decline in Proposals (old) Feb 23, 2022
@rsc
Copy link
Contributor

rsc commented Feb 23, 2022

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

@diogin
Copy link
Contributor Author

diogin commented Feb 24, 2022

OK, I'll follow the compiler change, please close this issue.
Thanks!

@rsc rsc moved this from Likely Decline to Declined in Proposals (old) Mar 2, 2022
@rsc
Copy link
Contributor

rsc commented Mar 2, 2022

No change in consensus, so declined.
— rsc for the proposal review group

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Declined
Development

No branches or pull requests

4 participants