Skip to content

Faster hilbert.Encode and hilbert.Decode#234

Open
matthinrichsen-wf wants to merge 5 commits intomasterfrom
faster_hilbert
Open

Faster hilbert.Encode and hilbert.Decode#234
matthinrichsen-wf wants to merge 5 commits intomasterfrom
faster_hilbert

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

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)
)

Co-authored-by: Simeon Steward <160513246+simeonsteward-wk@users.noreply.github.com>
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.

4 participants