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

use uint64 in reduce when possible #20

Merged
merged 1 commit into from
Feb 16, 2017
Merged

use uint64 in reduce when possible #20

merged 1 commit into from
Feb 16, 2017

Conversation

maddyblue
Copy link
Contributor

@maddyblue maddyblue commented Feb 16, 2017

name          old time/op  new time/op  delta
GDA/reduce-4   215µs ±30%   124µs ±10%  -42.48%  (p=0.008 n=5+5)

Idea from: https://github.com/cockroachdb/cockroach/blob/0ba52ff84a3e67efe3532b9119058aab8231478a/pkg/util/encoding/decimal.go#L773


This change is Reviewable

@RaduBerinde
Copy link
Member

:lgtm:


Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion.


decimal.go, line 480 at r1 (raw file):

		i := d.Coeff.Uint64()
		e := d.Exponent
		for i%10 == 0 {

maybe add a for i >= 10000 && i % 10000 == 0 { i /= 10000; e += 4 }


Comments from Reviewable

name          old time/op  new time/op  delta
GDA/reduce-4   215µs ±30%   124µs ±10%  -42.48%  (p=0.008 n=5+5)

Idea from: https://github.com/cockroachdb/cockroach/blob/0ba52ff84a3e67efe3532b9119058aab8231478a/pkg/util/encoding/decimal.go#L773
@maddyblue
Copy link
Contributor Author

Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion.


decimal.go, line 480 at r1 (raw file):

Previously, RaduBerinde wrote…

maybe add a for i >= 10000 && i % 10000 == 0 { i /= 10000; e += 4 }

Wow, this yielded another very large speed up in the benchmark. Thanks.


Comments from Reviewable

@maddyblue maddyblue merged commit 082dde4 into master Feb 16, 2017
@maddyblue maddyblue deleted the reduce branch February 16, 2017 19:26
@RaduBerinde
Copy link
Member

decimal.go, line 480 at r1 (raw file):

Previously, mjibson (Matt Jibson) wrote…

Wow, this yielded another very large speed up in the benchmark. Thanks.

👍


Comments from Reviewable

@RaduBerinde
Copy link
Member

decimal.go, line 480 at r1 (raw file):

Previously, RaduBerinde wrote…

👍

The same idea can be useful in the big loop below as well. We could:

  • divide by 10^10 until the remainder is non-zero
  • convert the remainder to uint64
  • use uint64 code to count the zeros on the remainder
  • do a final Quo with the exact power of 10 (as determined in the previous step). We can precalculate all powers of ten from 1 to 10.

Probably beyond the intention of this PR but maybe file an issue if you think this would help.


Comments from Reviewable

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants