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.

Sign up for GitHub

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

Jump to bottom

strings: IndexAny performance regression vs 1.9 #22750

Closed
TocarIP opened this issue Nov 15, 2017 · 5 comments
Closed

strings: IndexAny performance regression vs 1.9 #22750

TocarIP opened this issue Nov 15, 2017 · 5 comments
Labels
FrozenDueToAge Performance

Comments

@TocarIP
Copy link
Contributor

TocarIP commented Nov 15, 2017

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

go version devel +ef0e2af Mon Nov 6 15:55:31 2017 +0000 linux/amd64

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

GOARCH="amd64"
GOBIN=""
GOCACHE="/nfs/site/home/itocar/.cache/go-build"
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/localdisk/itocar/gopath/"
GORACE=""
GOROOT="/localdisk/itocar/golang"
GOTMPDIR=""
GOTOOLDIR="/localdisk/itocar/golang/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build447443283=/tmp/go-build -gno-record-gcc-switches"

/proc/cpuinfo:
model name : Intel(R) Xeon(R) CPU E5-2609 v3 @ 1.90GHz

What did you do?
Compared strings/IndexAnyASCII/1 performance to 1.9 and found regression.
It is most prominent on ndexAnyASCII/1:4 subbenchmark, but others also show this regression.

src/strings/IndexAnyASCII/1:4 15.0ns ± 0% 17.1ns ± 0% +14.00% (p=0.000 n=8+8)

Bisect points to c82ee79 and b78b54f

b78b54f looks mostly unrelated, but
c82ee79 changes generated code of IndexAny.
Removing useless check makes sense to me, but it also caused changes inside main loop.
Main problem that I see is an extra LoadReg in a loop.

jmpq   df                                                                                                                                                                    
mov    0xb8(%rsp),%r8        // this was done outside of the loop, before                                                                                                                                                                                                                             
cmp    %r8,%rdi                                                                                                                                                              
jge    dc                                                                                                                                                                    mov    0xb0(%rsp),%r9                                                                                                                                                        
movzbl (%r9,%rdi,1),%r10d                                                                                                                                                    
cmp    $0x80,%r10d

It is placed there by regalloc pass.
@TocarIP
Copy link
Contributor Author

TocarIP commented Nov 15, 2017

cc @randall77

@tklauser
Copy link
Member

https://go-review.googlesource.com/78112 (2a166c9) might be of interest regarding c82ee79

@randall77
Copy link
Contributor

I'm confused.
I don't see any such code difference in the code between 1.9.2 and tip. strings.IndexAny has identical assembly, except tip has an extra "return -1" section at the end.

There is a load in the inner loop that could be lifted, but that's there in 1.9.2 also.

@TocarIP
Copy link
Contributor Author

TocarIP commented Nov 16, 2017 •
edited

Turns out I was used slightly outdated version of master (ef0e2af), before 2a166c9. As @tklauser has mentioned 2a166c9 reverts c82ee79 which causes code to also revert to 1.9 state. Also I've tested suggestion from #22234 (I've commented loop.containsCall part) and it removes both loads from the loop, even for pre 2a166c9 version of IndexAny

@randall77
Copy link
Contributor

Ok, I'll close this issue then.

@golang golang locked and limited conversation to collaborators Nov 16, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge Performance
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@tklauser @mvdan @randall77 @gopherbot @TocarIP