Skip to content

/ gopherjs

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

Jump to bottom

compiler/natives/src/strconv: Use js string conversion utilities to improve performance #786

Open
wants to merge 4 commits into
base: master
from
Open

compiler/natives/src/strconv: Use js string conversion utilities to improve performance #786

wants to merge 4 commits into from

Conversation

@lologarithm
Copy link
Contributor

@lologarithm lologarithm commented Apr 3, 2018 •
edited

Added native implemntations for Atoi and Itoa. This generates smaller and faster code.

Before (using go stdlib)

$ gopherjs test strconv --run=Atoi --bench=Atoi
goos: linux
goarch: js
BenchmarkAtoi/Pos/7bit    	 2000000	       606 ns/op
BenchmarkAtoi/Pos/26bit   	 2000000	       628 ns/op
BenchmarkAtoi/Pos/31bit   	  200000	      8700 ns/op
BenchmarkAtoi/Neg/7bit    	 2000000	       660 ns/op
BenchmarkAtoi/Neg/26bit   	 2000000	       735 ns/op
BenchmarkAtoi/Neg/31bit   	  200000	      9030 ns/op
PASS
ok  	strconv	12.110s

After (using new js native)

$ gopherjs test strconv --run=Atoi --bench=Atoi
goos: linux
goarch: js
BenchmarkAtoi/Pos/7bit    	10000000	       111 ns/op
BenchmarkAtoi/Pos/26bit   	10000000	       150 ns/op
BenchmarkAtoi/Pos/31bit   	10000000	       214 ns/op
BenchmarkAtoi/Neg/7bit    	10000000	       138 ns/op
BenchmarkAtoi/Neg/26bit   	10000000	       158 ns/op
BenchmarkAtoi/Neg/31bit   	10000000	       222 ns/op
PASS
ok  	strconv	11.481s

I also wrote a small Itoa benchmark since there wasn't one in std lib. This uses 123456789 as the test number

gopherjs test --bench=.
goos: linux
goarch: js
BenchmarkGoItoa      1000000          1217 ns/op
BenchmarkJSItoa     10000000           142 ns/op
PASS
@lologarithm
Upgraded int conversion libraries
Loading status checks…
b57ecb9
@lologarithm lologarithm changed the title Upgraded int conversion libraries Added strconv natives Apr 3, 2018
@lologarithm lologarithm changed the title Added strconv natives Added strconv natives (for integers) Apr 3, 2018
@lologarithm lologarithm changed the title Added strconv natives (for integers) Added strconv package natives (for integers) Apr 3, 2018
@flimzy
flimzy reviewed Apr 4, 2018
View changes
compiler/natives/src/strconv/atoi.go Outdated
// Bounds checking
floatval := jsValue.Float()
if floatval > maxInt32 {
return 1<<31 - 1, rangeError(fnAtoi, s)

This comment has been minimized.

Sign in to view
@flimzy

flimzy Apr 4, 2018 •
edited
Member

Does it make sense to replace 1<<31 - 1 with the maxInt32 const here? And similar for minInt32 below?

This comment has been minimized.

Sign in to view
@lologarithm

lologarithm Apr 4, 2018
Author Contributor

That makes sense
@flimzy
flimzy reviewed Apr 4, 2018
View changes
compiler/natives/src/strconv/atoi.go Outdated
if len(s) == 0 {
return 0, syntaxError(fnAtoi, s)
}
jsValue := js.Global.Call("Number", s, 10)

This comment has been minimized.

Sign in to view
@flimzy

flimzy Apr 4, 2018
Member

I believe JS's Number constructor is far more forgiving than Go's Atoi implementation. Is it therefore reasonable to validate that s contains only Go-acceptable numbers?

Some examples of values that Number() accepts but Atoi() should not:

  • "0x10"
  • "10.2"
  • "0b11"
  • "0o10"

This comment has been minimized.

Sign in to view
@lologarithm

lologarithm Apr 4, 2018
Author Contributor

Probably should do some input validation first - if the resulting code is slower than just using Atoi directly I will probably remove this piece of code from the PR

This comment has been minimized.

Sign in to view
@lologarithm

lologarithm Apr 4, 2018
Author Contributor

Validate doesn't seem to have hurt perf at all -- pushing up the changes.

This comment has been minimized.

Sign in to view
@flimzy

flimzy Apr 4, 2018
Member

Looks like a good improvement. And glad to hear it doesn't have a noticeable impact on performance!
lologarithm added 2 commits Apr 4, 2018
@lologarithm
Add validate to input for numbers invalid in Go but valid for Number …
Loading status checks…
0781817 
…parsing
@lologarithm
go generate
Loading status checks…
9b8755a
@lologarithm
Copy link
Contributor Author

@lologarithm lologarithm commented Apr 5, 2018

@shurcooL another optimization PR for you :)
@lologarithm
Merge remote-tracking branch 'origin/master' into native_strconv
Loading status checks…
3a19b1d 
 Conflicts:
	compiler/natives/fs_vfsdata.go
@lologarithm lologarithm changed the title Added strconv package natives (for integers) compiler/natives/src/strconv: Use js string conversion utilities to improve performance Apr 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@flimzy flimzy

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
@lologarithm @flimzy
You can’t perform that action at this time.