Improve location related performance #113
Conversation
|
|
||
| // haversineDistance processes the locations to make sure that they are valid | ||
| // to return the corresponding distance. | ||
| func haversineDistance(from, to common.Location) common.Distance { |
There was a problem hiding this comment.
This function is really quite expensive. It is now replaced by a simpler function that still makes these validation checks on locations.
There was a problem hiding this comment.
Also dropped this function because haversineDistance could not be inlined because of the if branch.
There was a problem hiding this comment.
The math functions in it are also a little bit of a drag. Sqrt is typically not very fast, right?
There was a problem hiding this comment.
Pull Request Overview
This PR optimizes location-related performance by making Speed and SpeedUnit simple structs for better inlining and encoding locations in radians internally. The changes significantly improve performance - speed.Value takes 90% less time and Haversine calculations are more efficient due to eliminating degree-to-radian conversions.
- Converted
SpeedandSpeedUnitfrom interfaces to structs to enable inlining - Changed
Locationinternal storage to use radians instead of degrees - Replaced
haversineDistancefunction with optimizedHaversine0method
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| solution_unplan.go | Updated to use new Haversine0 function |
| model_haversine.go | Removed obsolete haversineDistance wrapper function |
| model_expression_haversine.go | Updated to use Haversine0 instead of haversineDistance |
| model_cluster.go | Updated multiple calls to use Haversine0 |
| common/speed_test.go | Added benchmark test for speed value conversion |
| common/speed.go | Converted Speed/SpeedUnit from interfaces to structs |
| common/location.go | Changed internal storage to radians, updated validation logic |
| common/haversine.go | Added Haversine0 function, optimized calculations |
| common/distance.go | Minor formatting cleanup |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| // NewInvalidLocation creates a new invalid Location. Longitude and latitude | ||
| // are not important. | ||
| func NewInvalidLocation() Location { | ||
| // Invalid locations are encoded as NaN in the longitude. |
There was a problem hiding this comment.
The comment should clarify that invalid locations are encoded as NaN in the longitude radians, since the field name is longitudeRadians.
| // Invalid locations are encoded as NaN in the longitude. | |
| // Invalid locations are encoded as NaN in the longitude radians. |
| // Equals returns true if the invoking location is equal to the other location. | ||
| func (l Location) Equals(other Location) bool { | ||
| return l.longitude == other.Longitude() && l.latitude == other.Latitude() | ||
| return l.longitudeRadians == other.longitudeRadians && l.latitudeRadians == other.latitudeRadians |
There was a problem hiding this comment.
Direct floating-point equality comparison may fail due to precision issues. Consider using a small epsilon for comparison or document that exact equality is intended for radians converted from the same degree values.
There was a problem hiding this comment.
Normally this should be fine, since no computation is done on these values. They are read from JSON typically, where to equal values should result in an equal float representation.
| // NewInvalidLocation creates a new invalid Location. Longitude and latitude | ||
| // are not important. | ||
| func NewInvalidLocation() Location { | ||
| // Invalid locations are encoded as NaN in the longitude. |
| // Equals returns true if the invoking location is equal to the other location. | ||
| func (l Location) Equals(other Location) bool { | ||
| return l.longitude == other.Longitude() && l.latitude == other.Latitude() | ||
| return l.longitudeRadians == other.longitudeRadians && l.latitudeRadians == other.latitudeRadians |
There was a problem hiding this comment.
Normally this should be fine, since no computation is done on these values. They are read from JSON typically, where to equal values should result in an equal float representation.
|
|
||
| // haversineDistance processes the locations to make sure that they are valid | ||
| // to return the corresponding distance. | ||
| func haversineDistance(from, to common.Location) common.Distance { |
There was a problem hiding this comment.
The math functions in it are also a little bit of a drag. Sqrt is typically not very fast, right?
This improves the performance by doing a few things:
speed.Valuenow takes 90% less time than before.Computational experiments also very promising.
In theory these changes are breaking so they go into the draft branch for now.