Skip to content

Conversation

@matthinrichsen-wf
Copy link
Contributor

Description

Encode/Decode can be improved.

This PR makes the following changes:

  • all divisions which are powers of two converted to bit shift
  • removes pointer dereferencing
  • prefers bitwise operators where we can

Results:

goos: darwin
goarch: arm64
pkg: github.com/Workiva/go-datastructures/numerics/hilbert
cpu: Apple M1 Max
          │  master.txt  │               pr4.txt               │
          │    sec/op    │   sec/op     vs base                │
Encode-10    78.71n ± 0%   50.64n ± 0%  -35.66% (p=0.000 n=30)
Decode-10   131.80n ± 1%   37.30n ± 0%  -71.70% (p=0.000 n=30)
geomean      101.9n        43.46n       -57.33%

          │  master.txt  │               pr4.txt               │
          │     B/op     │    B/op     vs base                 │
Encode-10   0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=30) ¹
Decode-10   0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=30) ¹
geomean                ²               +0.00%                ²
¹ all samples are equal
² summaries must be >0 to compute geomean

          │  master.txt  │               pr4.txt               │
          │  allocs/op   │ allocs/op   vs base                 │
Encode-10   0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=30) ¹
Decode-10   0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=30) ¹
geomean                ²               +0.00%                ²
¹ all samples are equal
² summaries must be >0 to compute geomean

}

func rotate(n, rx, ry int32, x, y *int32) {
func rotate(n, rx, ry int32, x, y int32) (int32, int32) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🌵

Suggested change
func rotate(n, rx, ry int32, x, y int32) (int32, int32) {
func rotate(n, rx, ry, x, y int32) (int32, int32) {

Comment on lines 85 to 86
x += int32(s * rx)
y += int32(s * ry)
Copy link

@joshprzybyszewski-wf joshprzybyszewski-wf Oct 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔮 I don't know definitively, but aren't rx and ry only ever zero or one? Aren't they essentially bools? Isn't it always true that rx != ry? I'm not sure. But if so, would a

if rx {
	x += int32(s)
} else {
	y += int32(s)
}

be faster than a multiplication?

Or maybe they're not mutually exclusive: is a bool check still faster than a mult?

if rx {
	x += int32(s)
}
if ry {
	y += int32(s)
}

ry = boolToInt(y&s > 0)
d += int64(int64(s) * int64(s) * int64(((3 * rx) ^ ry)))
rotate(s, rx, ry, &x, &y)
d += int64(int64(s) * int64(s) * int64(rx<<1|(rx^ry)))

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📖 There's a lot magic bitwise stuff happening in this file (there was before!). Especially since this is OSS, perhaps we could add comments to the code describing the intent of what's happening (and why we're doing it the obscure way, not the clear way).


for s := int64(1); s < int64(n); s *= 2 {
rx = 1 & (t / 2)
for s := int64(1); s < int64(n); s <<= 1 {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🌵 I wonder what the impact is to cast n to int64 on every iteration. Could we just have a sister const at the top of the file?

const (
	n = int32(1 << 31)
	n_64 = int64(n)
)

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.

5 participants