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

strconv: ParseInt, ParseFloat use two loops instead of one #33330

Closed
sarnold64 opened this issue Jul 28, 2019 · 1 comment
Closed

strconv: ParseInt, ParseFloat use two loops instead of one #33330

sarnold64 opened this issue Jul 28, 2019 · 1 comment
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Performance
Milestone
Go1.14

Comments

@sarnold64
Copy link
Contributor

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

$ go version
go version devel +919594830f Tue Jul 23 23:42:43 2019 +0000 darwin/amd64

Does this issue reproduce with the latest release?

Yes, see version

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

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/sarnold64/Library/Caches/go-build"
GOENV="/Users/sarnold64/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/sarnold64/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/Users/sarnold64/golang/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/Users/sarnold64/golang/go/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/sarnold64/golang/go/src/go.mod"
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 -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/5s/zkkc95rs08731f9t6df3fvsc0000gr/T/go-build178433439=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

I noticed that strconv takes more time than it needs to for ParseInt and ParseFloat because it loops over the input twice, once in underscoreOK and once to do the parsing.

What did you expect to see?

One loop

What did you see instead?

Two loops

I was playing around with pprof after the gophercon talks and saw this looking suspicious.

Some performance results on my laptop after fixing:

name                   old time/op  new time/op  delta
ParseInt/Pos/7bit-12   19.0ns ± 3%  12.1ns ± 0%  -36.58%  (p=0.000 n=6+5)
ParseInt/Pos/26bit-12  28.5ns ± 0%  18.4ns ± 2%  -35.58%  (p=0.002 n=6+6)
ParseInt/Pos/31bit-12  32.4ns ± 1%  20.9ns ± 1%  -35.71%  (p=0.002 n=6+6)
ParseInt/Pos/56bit-12  47.0ns ± 2%  29.3ns ± 1%  -37.70%  (p=0.002 n=6+6)
ParseInt/Pos/63bit-12  50.9ns ± 2%  32.0ns ± 1%  -37.24%  (p=0.002 n=6+6)
ParseInt/Neg/7bit-12   18.3ns ± 6%  12.2ns ± 1%  -33.18%  (p=0.002 n=6+6)
ParseInt/Neg/26bit-12  28.2ns ± 3%  18.2ns ± 2%  -35.38%  (p=0.002 n=6+6)
ParseInt/Neg/31bit-12  31.4ns ± 1%  20.7ns ± 1%  -34.04%  (p=0.004 n=6+5)
ParseInt/Neg/56bit-12  45.4ns ± 0%  29.4ns ± 1%  -35.17%  (p=0.004 n=5+6)
ParseInt/Neg/63bit-12  49.4ns ± 1%  32.0ns ± 1%  -35.30%  (p=0.002 n=6+6)

name                   old time/op  new time/op  delta
Atof64Decimal-12       30.1ns ± 1%  25.4ns ± 1%  -15.75%  (p=0.002 n=6+6)
Atof64Float-12         38.3ns ± 1%  31.5ns ± 0%  -17.84%  (p=0.000 n=6+5)
Atof64FloatExp-12      63.1ns ± 0%  53.9ns ± 0%  -14.53%  (p=0.002 n=6+6)
Atof64Big-12            113ns ± 0%    96ns ± 2%  -14.85%  (p=0.010 n=4+6)
Atof64RandomBits-12     180ns ±10%   160ns ±16%  -11.36%  (p=0.028 n=6+6)
Atof64RandomFloats-12   123ns ± 2%   102ns ± 1%  -17.03%  (p=0.002 n=6+6)
Atof32Decimal-12       31.4ns ± 0%  25.4ns ± 1%  -18.86%  (p=0.002 n=6+6)
Atof32Float-12         36.5ns ± 0%  29.4ns ± 0%  -19.40%  (p=0.008 n=5+5)
Atof32FloatExp-12      67.0ns ± 0%  56.6ns ± 0%  -15.58%  (p=0.008 n=5+5)
Atof32Random-12        90.6ns ± 1%  75.1ns ± 0%  -17.15%  (p=0.004 n=6+5)
@ALTree ALTree added this to the Go1.14 milestone Jul 28, 2019
@ALTree ALTree added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jul 28, 2019
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/187957 mentions this issue: strconv: Speed improvement to number parsing

@golang golang deleted a comment from zhaoyuyang-x Jul 29, 2019
tomocy pushed a commit to tomocy/go that referenced this issue Sep 1, 2019
@sarnold64 @tomocy
strconv: Speed improvement to number parsing
2fb309b 
Run underscore validation only if we have seen underscores.

Some performance results on my laptop:
name                   old time/op  new time/op  delta
Atof64Decimal-12       30.5ns ± 0%  23.8ns ± 0%  -22.02%  (p=0.016 n=5+4)
Atof64Float-12         39.0ns ± 0%  28.7ns ± 0%  -26.39%  (p=0.002 n=6+6)
Atof64FloatExp-12      64.4ns ± 1%  54.4ns ± 1%  -15.65%  (p=0.002 n=6+6)
Atof64Big-12            115ns ± 1%    87ns ± 1%  -24.45%  (p=0.002 n=6+6)
Atof64RandomBits-12     187ns ±14%   156ns ±19%  -16.46%  (p=0.032 n=6+6)
Atof64RandomFloats-12   126ns ± 0%   105ns ± 1%  -16.65%  (p=0.000 n=6+5)
Atof32Decimal-12       32.0ns ± 1%  24.0ns ± 1%  -24.97%  (p=0.002 n=6+6)
Atof32Float-12         37.1ns ± 1%  27.0ns ± 1%  -27.42%  (p=0.002 n=6+6)
Atof32FloatExp-12      68.4ns ± 1%  54.2ns ± 1%  -20.77%  (p=0.002 n=6+6)
Atof32Random-12        92.0ns ± 1%  77.4ns ± 0%  -15.81%  (p=0.000 n=6+5)
ParseInt/Pos/7bit-12   19.4ns ± 1%  13.8ns ±10%  -28.94%  (p=0.002 n=6+6)
ParseInt/Pos/26bit-12  29.1ns ± 1%  19.8ns ± 2%  -31.92%  (p=0.002 n=6+6)
ParseInt/Pos/31bit-12  33.1ns ± 0%  22.3ns ± 3%  -32.62%  (p=0.004 n=5+6)
ParseInt/Pos/56bit-12  47.8ns ± 1%  30.7ns ± 1%  -35.78%  (p=0.004 n=6+5)
ParseInt/Pos/63bit-12  51.9ns ± 1%  33.4ns ± 2%  -35.49%  (p=0.002 n=6+6)
ParseInt/Neg/7bit-12   18.5ns ± 4%  13.4ns ± 3%  -27.88%  (p=0.002 n=6+6)
ParseInt/Neg/26bit-12  28.4ns ± 3%  19.7ns ± 3%  -30.38%  (p=0.002 n=6+6)
ParseInt/Neg/31bit-12  31.9ns ± 1%  21.8ns ± 2%  -31.56%  (p=0.002 n=6+6)
ParseInt/Neg/56bit-12  46.2ns ± 0%  30.6ns ± 1%  -33.73%  (p=0.004 n=5+6)
ParseInt/Neg/63bit-12  50.2ns ± 1%  33.2ns ± 1%  -33.96%  (p=0.002 n=6+6)

Fixes golang#33330

Change-Id: I119da66457c2fbaf6e88bb90cf56417a16df8f0e
Reviewed-on: https://go-review.googlesource.com/c/go/+/187957
Run-TryBot: Robert Griesemer <gri@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
t4n6a1ka pushed a commit to t4n6a1ka/go that referenced this issue Sep 5, 2019
@sarnold64 @t4n6a1ka
strconv: Speed improvement to number parsing

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
Learn about vigilant mode
aa5d7a7 
Run underscore validation only if we have seen underscores.

Some performance results on my laptop:
name                   old time/op  new time/op  delta
Atof64Decimal-12       30.5ns ± 0%  23.8ns ± 0%  -22.02%  (p=0.016 n=5+4)
Atof64Float-12         39.0ns ± 0%  28.7ns ± 0%  -26.39%  (p=0.002 n=6+6)
Atof64FloatExp-12      64.4ns ± 1%  54.4ns ± 1%  -15.65%  (p=0.002 n=6+6)
Atof64Big-12            115ns ± 1%    87ns ± 1%  -24.45%  (p=0.002 n=6+6)
Atof64RandomBits-12     187ns ±14%   156ns ±19%  -16.46%  (p=0.032 n=6+6)
Atof64RandomFloats-12   126ns ± 0%   105ns ± 1%  -16.65%  (p=0.000 n=6+5)
Atof32Decimal-12       32.0ns ± 1%  24.0ns ± 1%  -24.97%  (p=0.002 n=6+6)
Atof32Float-12         37.1ns ± 1%  27.0ns ± 1%  -27.42%  (p=0.002 n=6+6)
Atof32FloatExp-12      68.4ns ± 1%  54.2ns ± 1%  -20.77%  (p=0.002 n=6+6)
Atof32Random-12        92.0ns ± 1%  77.4ns ± 0%  -15.81%  (p=0.000 n=6+5)
ParseInt/Pos/7bit-12   19.4ns ± 1%  13.8ns ±10%  -28.94%  (p=0.002 n=6+6)
ParseInt/Pos/26bit-12  29.1ns ± 1%  19.8ns ± 2%  -31.92%  (p=0.002 n=6+6)
ParseInt/Pos/31bit-12  33.1ns ± 0%  22.3ns ± 3%  -32.62%  (p=0.004 n=5+6)
ParseInt/Pos/56bit-12  47.8ns ± 1%  30.7ns ± 1%  -35.78%  (p=0.004 n=6+5)
ParseInt/Pos/63bit-12  51.9ns ± 1%  33.4ns ± 2%  -35.49%  (p=0.002 n=6+6)
ParseInt/Neg/7bit-12   18.5ns ± 4%  13.4ns ± 3%  -27.88%  (p=0.002 n=6+6)
ParseInt/Neg/26bit-12  28.4ns ± 3%  19.7ns ± 3%  -30.38%  (p=0.002 n=6+6)
ParseInt/Neg/31bit-12  31.9ns ± 1%  21.8ns ± 2%  -31.56%  (p=0.002 n=6+6)
ParseInt/Neg/56bit-12  46.2ns ± 0%  30.6ns ± 1%  -33.73%  (p=0.004 n=5+6)
ParseInt/Neg/63bit-12  50.2ns ± 1%  33.2ns ± 1%  -33.96%  (p=0.002 n=6+6)

Fixes golang#33330

Change-Id: I119da66457c2fbaf6e88bb90cf56417a16df8f0e
Reviewed-on: https://go-review.googlesource.com/c/go/+/187957
Run-TryBot: Robert Griesemer <gri@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
@golang golang locked and limited conversation to collaborators Aug 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Performance
Projects
None yet
Milestone
Go1.14
Development

No branches or pull requests
3 participants
@ALTree @gopherbot @sarnold64