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

syscall: memoize convT2I from creating errors in WSARecv, WSASend, etc? #16988

Closed
diogin opened this issue Sep 4, 2016 · 11 comments
Closed

syscall: memoize convT2I from creating errors in WSARecv, WSASend, etc? #16988

diogin opened this issue Sep 4, 2016 · 11 comments

Comments

@diogin
Copy link
Contributor

@diogin diogin commented Sep 4, 2016

Please answer these questions before submitting your issue. Thanks!

What version of Go are you using (go version)?

go version go1.7 windows/amd64

What operating system and processor architecture are you using (go env)?

Windows 7 SP1 Ultimate X64, Intel i7-3770K + Z77 chipset, go env prints:
set GOARCH=amd64
set GOBIN=
set GOEXE=.exe
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOOS=windows
set GOPATH=
set GORACE=
set GOROOT=D:\port\go
set GOTOOLDIR=D:\port\go\pkg\tool\windows_amd64
set CC=gcc
set GOGCCFLAGS=-m64 -mthreads -fmessage-length=0 -fdebug-prefix-map=C:\Users\diogin\AppData\Local\Temp\go-build701008446=/tmp/go
-build -gno-record-gcc-switches
set CXX=g++
set CGO_ENABLED=1

What did you do?

A runnable server.go is at https://play.golang.org/p/Jg7xSHU3B4
A runnable client.go is at https://play.golang.org/p/8u0y0j9IHh

First, build and run server.go, and then client.go.
After the server prints EOF, run:

go tool pprof -alloc_objects server.exe heap.pprof

Then, run top in pprof:

(pprof) top
327685 of 327685 total ( 100%)
Dropped 3 nodes (cum <= 1638)
flat flat% sum% cum cum%
327685 100% 100% 327685 100% syscall.WSARecv
0 0% 100% 327685 100% main.main
0 0% 100% 327685 100% net.(_conn).Read
0 0% 100% 327685 100% net.(_ioSrv).ExecIO
0 0% 100% 327685 100% net.(_netFD).Read
0 0% 100% 327685 100% net.(_netFD).Read.func1
0 0% 100% 327685 100% runtime.goexit
0 0% 100% 327685 100% runtime.main

We see syscall.WSARecv allocates a lot of memory. Then list WS:

(pprof) list WS
Total: 327685
ROUTINE ======================== syscall.WSARecv in D:/port/go/src/syscall/zsyscall_windows.go
327685 327685 (flat, cum) 100% of Total
. . 1518:
. . 1519:func WSARecv(s Handle, bufs *WSABuf, bufcnt uint32, recvd *uint32, flags *uint32, overlapped *Overl
apped, croutine *byte) (err error) {
. . 1520: r1, _, e1 := Syscall9(procWSARecv.Addr(), 7, uintptr(s), uintptr(unsafe.Pointer(bufs)), uintptr(
bufcnt), uintptr(unsafe.Pointer(recvd)), uintptr(unsafe.Pointer(flags)), uintptr(unsafe.Pointer(overlapped)), uintptr(unsafe.Poi
nter(croutine)), 0, 0)
. . 1521: if r1 == socket_error {
. . 1522: if e1 != 0 {
327685 327685 1523: err = error(e1)
. . 1524: } else {
. . 1525: err = EINVAL
. . 1526: }
. . 1527: }
. . 1528: return

And disasm WS:

327685     327685     48e33b: CALL runtime.convT2I(SB)

So line 1523 allocates a lot of memory, only to make a useless error, which means "overlapped i/o operation is in progress".

What did you expect to see?

The memory consumption don't increase.

What did you see instead?

It allocates a lot of memory.

@bradfitz bradfitz changed the title WSARecv and WSASend cause a lot of memory consumption syscall: memoize convT2I from creating errors in WSARecv, WSASend, etc? Sep 4, 2016
@bradfitz bradfitz added the OS-Windows label Sep 4, 2016
@diogin
Copy link
Contributor Author

@diogin diogin commented Sep 4, 2016

Hi brad,

Thanks for your changing of the title. Yes, the conversion of error code e1 to error should not be repeated.
MSDN page of WSARecv ( https://msdn.microsoft.com/en-us/library/ms740668(v=vs.85).aspx#WSAEINPROGRESS ) said:

"A blocking operation is currently executing. Windows Sockets only allows a single blocking operation—per- task or thread—to be outstanding, and if any other function call is made (whether or not it references that or any other socket) the function fails with the WSAEINPROGRESS error."

This might be the cause.

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Sep 4, 2016

These come from mksyscall_windows.go,

func (r *Rets) useLongHandleErrorCode(retvar string) string {  
        const code = `if %s {                                                                                                       
                if e1 != 0 {                                                                                                        
                        err = error(e1)                                                                                             
                } else {                                                                                                            
                        err = %sEINVAL                                                                                              
                }                                                                                                                   
        }`  
        cond := retvar + " == 0"  
        if r.FailCond != "" {  
                cond = strings.Replace(r.FailCond, "failretval", retvar, 1)  
        }  
        return fmt.Sprintf(code, cond, syscalldot())  
}  

Maybe we should change error(r1) to makeOrGetError(r1) and have all the popular ones pre-created? Whatever the popular ones are.

/cc @josharian @alexbrainman

@josharian
Copy link
Contributor

@josharian josharian commented Sep 4, 2016

SGTM. IIRC we did something similar on the *nix side to remove allocations for common, static errors.

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Sep 4, 2016

Yeah, in 6bf0007 and dde5b56

@gopherbot
Copy link

@gopherbot gopherbot commented Sep 4, 2016

CL https://golang.org/cl/28484 mentions this issue.

@quentinmit quentinmit added this to the Go1.8Maybe milestone Sep 6, 2016
@gopherbot gopherbot closed this in ca264cd Sep 7, 2016
@diogin
Copy link
Contributor Author

@diogin diogin commented Sep 7, 2016

Hi Brad,

Sorry to have misled you, but the correct error code here is not WSAEINPROGRESS, it is WSA_IO_PENDING (code 997), see: https://msdn.microsoft.com/en-us/library/ms740668(v=vs.85).aspx#WSA_IO_PENDING

I apologize for the misleading again!

@bradfitz bradfitz reopened this Sep 7, 2016
@alexbrainman
Copy link
Member

@alexbrainman alexbrainman commented Sep 7, 2016

I will look into this.

Alex

@alexbrainman
Copy link
Member

@alexbrainman alexbrainman commented Sep 8, 2016

@bradfitz

// TODO: add more here, after collecting data on the common
// error values see on Windows.

What is "common" here? Can we add all that run of all.bat reports? Would adding them all slow things down in errnoErr?

(perhaps when running
// all.bat?)

How would I do that? Write some code in runtime to dump all encountered in errnoErr errors into a global file? Then sort and uniq?

Alex

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Sep 8, 2016

What is "common" here? Can we add all that run of all.bat reports?

Or the top N frequently occurring. Use some judgement. I don't know.

Would adding them all slow things down in errnoErr?

It shouldn't. Let that be the compiler's problem. Switching on ints is on the TODO list to optimize more anyway.

Plus: it's an error path. How fast does it have to be?

How would I do that? Write some code in runtime to dump all encountered in errnoErr errors into a global file? Then sort and uniq?

Sounds good.

@alexbrainman
Copy link
Member

@alexbrainman alexbrainman commented Sep 8, 2016

Sounds good.

I was hoping you would come up with some inventive way. Like me using video recorders or something. :-(

Alex

@gopherbot
Copy link

@gopherbot gopherbot commented Sep 10, 2016

CL https://golang.org/cl/28990 mentions this issue.

@gopherbot gopherbot closed this in fca3dd3 Sep 11, 2016
@golang golang locked and limited conversation to collaborators Sep 11, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.