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

x/text: avoid double execution of lookup in lookupAndFormat() #29136

Open
fgm opened this issue Dec 7, 2018 · 2 comments
Open

x/text: avoid double execution of lookup in lookupAndFormat() #29136

fgm opened this issue Dec 7, 2018 · 2 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Performance
Milestone

Comments

@fgm
Copy link

fgm commented Dec 7, 2018

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

$ go version
go version go1.11.2 linux/amd64

Does this issue reproduce with the latest release?

  • This is the latest release.
  • code is identical in the master branch

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

go env Output
$ go env
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/fgm/.cache/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/fgm/go"
GOPROXY=""
GORACE=""
GOROOT="/usr/local/go"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
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-build766242374=/tmp/go-build -gno-record-gcc-switches"

What did you do?

  • Prepare a simple app with exported strings using //go:generate gotext -srclang=en update -out=catalog/catalog.go -lang=fr
  • Invoke p.Printf("Hello, world") and debug what happens, until stepping into message.lookupAndFormat() until this fragment:
	if p.catContext.Execute(id) == catalog.ErrNotFound {
		if p.catContext.Execute(msg) == catalog.ErrNotFound {
			p.Render(msg)
			return
		}

What did you expect to see?

  • since in most cases id == msg, skip the second execution of p.catContext(msg) if the first execution returned catalog.ErrNotFound

What did you see instead?

  • if p.catContext.Execute(id) returned catalog.ErrNotFound, code performs the same work a second time as p.catContext.Execute(msg)
  • which could be avoided by checking whether id == msg, maybe like:
	if p.catContext.Execute(id) == catalog.ErrNotFound {
		if id == msg || p.catContext.Execute(msg) == catalog.ErrNotFound {
			p.Render(msg)
			return
		}
	}
@gopherbot gopherbot added this to the Unreleased milestone Dec 7, 2018
@bcmills
Copy link
Contributor

bcmills commented Dec 10, 2018

CC: @mpvl

@fgm
Copy link
Author

fgm commented Sep 11, 2019

Still present in v0.3.2 (12/2018) and current master HEAD. I could prepare a PR if you so wish.

@seankhliao seankhliao added Performance NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Aug 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Performance
Projects
None yet
Development

No branches or pull requests

4 participants