-
Notifications
You must be signed in to change notification settings - Fork 17.9k
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
cmd/compile: better escape analysis for net.SyscallConn #51334
Comments
We already have a devirtualization pass, but it can only handle pretty simple cases. The case relevant here is that we can devirtualize, but only if
The If you mark All that said, this issue may be fixable just by rejiggering Not that it wouldn't be helpful to make the compiler better, but FYI in case you're looking for something nearer term. |
@randall77 note that
However, everything still escapes
|
When I add sc, err := f.SyscallConn()
if err != nil {
return 0, nil, false
}
rc := sc.(*os.RawConn)
var werr error
err = rc.Read(func(fd uintptr) bool {
written, werr = poll.SendFile(&c.pfd, int(fd), remain)
return true
}) |
In OS, |
@randall77 func newRawConn(file *File) (*rawConn, error) {
return &rawConn{file: file}, nil
} But even manually manual inlining the allocation does not work: func (f *File) SyscallConn() (syscall.RawConn, error) {
return &rawConn{file: f}, nil
} |
Maybe it is the 2 results. The analyzer can only handle single results. |
@randall77 yep! That's it. |
Change https://go.dev/cl/567898 mentions this issue: |
Programs using net.SyscallConn look like:
Although
sc
is an interface value of typenet.SyscallConn
, the compiler could know the underlying type, because there's only one that net.TCPConn.SyscallConn returns. It should therefore be able to see that sc.Read does not escape its argument, so that the closure can be stack-allocated. But today the closure is heap-allocated. It would be good to make the compiler smart enough to avoid the allocation.See also #51170, which proposed an API change, but a smarter compiler with no new API would be better.
The text was updated successfully, but these errors were encountered: