Merge branch 'develop' into ds/perf/fewer-alloc-2

common/location.go

@@ -4,20 +4,21 @@ package common
 
 import (
 	"fmt"
+	"math"
 )
 
 // NewLocation creates a new Location. An error is returned if the longitude is
 // not between (-180, 180) or the latitude is not between (-90, 90).
 func NewLocation(longitude float64, latitude float64) (Location, error) {
-	if longitude < -180 || longitude > 180 {
+	if !isValidLongitude(longitude) {
 		return NewInvalidLocation(),
 			fmt.Errorf("longitude %f must be between -180 and 180", longitude)
 	}
-	if latitude < -90 || latitude > 90 {
+	if !isValidLatitude(latitude) {
 		return NewInvalidLocation(),
 			fmt.Errorf("latitude %f must be between -90 and 90", latitude)
 	}
-	return location{
+	return Location{
 		longitude: longitude,
 		latitude:  latitude,
 		valid:     true,
@@ -27,8 +28,10 @@ func NewLocation(longitude float64, latitude float64) (Location, error) {
 // NewInvalidLocation creates a new invalid Location. Longitude and latitude
 // are not important.
 func NewInvalidLocation() Location {
-	return location{
-		valid: false,
+	return Location{
+		longitude: math.NaN(),
+		latitude:  math.NaN(),
+		valid:     false,
 	}
 }
 
@@ -37,15 +40,15 @@ type Locations []Location
 
 // Unique returns a new slice of Locations with unique locations.
 func (l Locations) Unique() Locations {
-	unique := make(map[string]Location)
+	unique := make(map[Location]struct{}, len(l))
 	for _, location := range l {
-		// TODO: in Go 1.20 we don't need to use fmt.Sprintf here.
-		// This can simply become unique[location] = struct{}{}
-		unique[fmt.Sprintf("%v", location)] = location
+		unique[location] = struct{}{}
 	}
-	result := make(Locations, 0, len(unique))
-	for _, location := range unique {
-		result = append(result, location)
+	result := make(Locations, len(unique))
+	i := 0
+	for location := range unique {
+		result[i] = location
+		i++
 	}
 	return result
 }
@@ -58,63 +61,61 @@ func (l Locations) Centroid() (Location, error) {
 	}
 	lat := 0.0
 	lon := 0.0
-	for l, location := range l {
-		if !location.IsValid() {
-			return NewInvalidLocation(),
-				fmt.Errorf(
-					"location %d (%f, %f) is invalid",
-					l,
-					location.Longitude(),
-					location.Latitude(),
-				)
-		}
+	for _, location := range l {
+		// invalid locations are encoded as NaN, which will propagate
+		// so we can avoid a check here.
 		lat += location.Latitude()
 		lon += location.Longitude()
 	}
-	return NewLocation(lon/float64(len(l)), lat/float64(len(l)))
-}
-
-// Location represents a physical location on the earth.
-type Location interface {
-	// Longitude returns the longitude of the location.
-	Longitude() float64
-	// Latitude returns the latitude of the location.
-	Latitude() float64
-	// Equals returns true if the location is equal to the location given as an
-	// argument.
-	Equals(Location) bool
-	// IsValid returns true if the location is valid. A location is valid if
-	// the bounds of the longitude and latitude are correct.
-	IsValid() bool
+	n := float64(len(l))
+	loc, err := NewLocation(lon/n, lat/n)
+	if err != nil {
+		return NewInvalidLocation(), err
+	}
+	return loc, nil
 }
 
-// Implements Location.
-type location struct {
+// Location represents a location on earth.
+type Location struct {
 	longitude float64
 	latitude  float64
 	valid     bool
 }
 
-func (l location) String() string {
+// String returns a string representation of the location.
+func (l Location) String() string {
 	return fmt.Sprintf(
 		"{lat: %v,lon: %v}",
 		l.latitude,
 		l.longitude,
 	)
 }
 
-func (l location) Longitude() float64 {
+// Longitude returns the longitude of the location.
+func (l Location) Longitude() float64 {
 	return l.longitude
 }
 
-func (l location) Latitude() float64 {
+// Latitude returns the latitude of the location.
+func (l Location) Latitude() float64 {
 	return l.latitude
 }
 
-func (l location) Equals(other Location) bool {
+// 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()
 }
 
-func (l location) IsValid() bool {
+// IsValid returns true if the location is valid. A location is valid if
+// the bounds of the longitude and latitude are correct.
+func (l Location) IsValid() bool {
 	return l.valid
 }
+
+func isValidLongitude(longitude float64) bool {
+	return longitude >= -180 && longitude <= 180
+}
+
+func isValidLatitude(latitude float64) bool {
+	return latitude >= -90 && latitude <= 90
+}

common/location_test.go

@@ -0,0 +1,102 @@
+// © 2019-present nextmv.io inc
+
+package common_test
+
+import (
+	"fmt"
+	"math/rand"
+	"testing"
+
+	"github.com/nextmv-io/nextroute/common"
+)
+
+func BenchmarkLocation(b *testing.B) {
+	r := rand.New(rand.NewSource(0))
+	lon, lat := r.Float64()*360-180, r.Float64()*180-90
+	l, _ := common.NewLocation(lon, lat)
+	b.ResetTimer()
+	for i := 0; i < b.N; i++ {
+		_ = l.IsValid()
+	}
+}
+
+func BenchmarkUnique(b *testing.B) {
+	// test for different number of locations
+	for _, n := range []int{10, 100, 1_000, 10_000} {
+		locations := make(common.Locations, 0, n)
+		r := rand.New(rand.NewSource(0))
+		for i := 0; i < n; i++ {
+			l, _ := common.NewLocation(r.Float64()*360-180, r.Float64()*180-90)
+			if !l.IsValid() {
+				b.Error("invalid location")
+			}
+			locations = append(locations, l)
+		}
+		b.Run(fmt.Sprintf("n=%v", n), func(b *testing.B) {
+			b.ResetTimer()
+			for i := 0; i < b.N; i++ {
+				_ = locations.Unique()
+			}
+		})
+	}
+}
+
+func TestLocation(t *testing.T) {
+	l, err := common.NewLocation(0, 0)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if !l.IsValid() {
+		t.Error("expected valid location")
+	}
+	invalid := common.NewInvalidLocation()
+	if invalid.IsValid() {
+		t.Error("expected invalid location")
+	}
+	l, err = common.NewLocation(180.1, 0)
+	if err == nil {
+		t.Error("expected error")
+	}
+	if l.IsValid() {
+		t.Error("expected invalid location")
+	}
+}
+
+func TestUnique(t *testing.T) {
+	newLocation := func(lon, lat float64) common.Location {
+		l, err := common.NewLocation(lon, lat)
+		if err != nil {
+			t.Fatal(err)
+		}
+		return l
+	}
+	locations := common.Locations{
+		newLocation(0, 0),
+		newLocation(123.234983434, 80.234983434),
+		newLocation(123.234983434, 80.234983434),
+		newLocation(0, 0),
+		newLocation(0, 0),
+		newLocation(0, 0),
+		newLocation(0, 0),
+	}
+	unique := locations.Unique()
+	if len(unique) != 2 {
+		t.Errorf("expected 2 unique locations, got %v", len(unique))
+	}
+}
+
+func BenchmarkCentroid(b *testing.B) {
+	locations := make(common.Locations, 0, 2_000)
+	r := rand.New(rand.NewSource(0))
+	for i := 0; i < 2_000; i++ {
+		l, _ := common.NewLocation(r.Float64()*360-180, r.Float64()*180-90)
+		if !l.IsValid() {
+			b.Error("invalid location")
+		}
+		locations = append(locations, l)
+	}
+	b.ResetTimer()
+	for i := 0; i < b.N; i++ {
+		_, _ = locations.Centroid()
+	}
+}

solution_construcation_sweep_test.go

@@ -32,8 +32,8 @@ func TestSweepTwoDepots(t *testing.T) {
 	input := singleVehiclePlanSingleStopsModel()
 
 	location := Location{
-		Lat: common.NewInvalidLocation().Latitude(),
-		Lon: common.NewInvalidLocation().Longitude(),
+		Lat: 0,
+		Lon: 0,
 	}
 	input.Vehicles = append(input.Vehicles, vehicles("truck", location, 1)...)
 	model, err := createModel(input)