Skip to content

Conversation

fx408
Copy link
Contributor

@fx408 fx408 commented Sep 10, 2025

Pre-allocates memory to avoid triggering expansion, and enhances performance.

  • Before:
BenchmarkAllNeighbors/level12         	 3699290	       322.9 ns/op	     120 B/op	       4 allocs/op
BenchmarkAllNeighbors/level14         	 1699875	       700.6 ns/op	     504 B/op	       6 allocs/op
BenchmarkAllNeighbors/level16         	  594056	        2091 ns/op	    2040 B/op	       8 allocs/op
  • After:
BenchmarkAllNeighbors/level12         	 5084275	       254.1 ns/op	      64 B/op	       1 allocs/op
BenchmarkAllNeighbors/level14         	 2295961	       510.6 ns/op	     160 B/op	       1 allocs/op
BenchmarkAllNeighbors/level16        	  786132	        1610 ns/op	     576 B/op	       1 allocs/op

The method AllNeighbors returns a maximum of 8 CellIDs, pre-allocates memory to avoid triggering expansion, and enhances performance.
Copy link

google-cla bot commented Sep 10, 2025

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

Copy link
Collaborator

@panmari panmari left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution! Just a small style nit. Did you actually see this cause a performance issue on your end?

@jmr
Copy link
Collaborator

jmr commented Sep 11, 2025

Note there are some failing checks.


func BenchmarkAllNeighbors(b *testing.B) {
level := 12
ll := LatLngFromDegrees(10.100001, 10.100002)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure what these values buy over 10.1, 10.1 or 10.0, 10.0, but we can change it later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure what these values buy over 10.1, 10.1 or 10.0, 10.0, but we can change it later.

I think there is no difference.

@jmr jmr changed the title Allocate memory in one go CellID.AllNeighbors: Allocate memory in one go Sep 11, 2025
@jmr jmr merged commit b504328 into golang:master Sep 12, 2025
5 checks passed
@jmr
Copy link
Collaborator

jmr commented Sep 12, 2025

Thanks! Did you just happen to notice this, or was it actually causing a performance problem?

It would be great if you could look for other instances of the same problem.

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