Improve multi stop plan unit generation

[dirkschumacher]

Explored a bit to reduce some overhead when generating sequences for move generation. In addition this has the effect that we don't spawn go routines so deep in the search.

Below are two smaller benchmark tests. The full set I cannot yet run until #47 is fixed, but this already captures some of the benefits here as it's mostly about allocations.

name                                          old time/op    new time/op    delta
pkg:github.com/nextmv-io/nextroute goos:darwin goarch:arm64
SequenceGenerator3-8                            15.2µs ± 0%    13.6µs ± 0%  -10.39%
pkg:github.com/nextmv-io/nextroute/tests/golden goos:darwin goarch:arm64
Golden/testdata/precedence_pathologic.json-8    4.56ms ± 0%    2.29ms ± 0%  -49.73%

name                                          old alloc/op   new alloc/op   delta
pkg:github.com/nextmv-io/nextroute goos:darwin goarch:arm64
SequenceGenerator3-8                            6.38kB ± 0%    5.99kB ± 0%   -6.04%
pkg:github.com/nextmv-io/nextroute/tests/golden goos:darwin goarch:arm64
Golden/testdata/precedence_pathologic.json-8    9.74MB ± 0%    5.10MB ± 0%  -47.66%

name                                          old allocs/op  new allocs/op  delta
pkg:github.com/nextmv-io/nextroute goos:darwin goarch:arm64
SequenceGenerator3-8                              71.0 ± 0%      69.0 ± 0%   -2.82%
pkg:github.com/nextmv-io/nextroute/tests/golden goos:darwin goarch:arm64
Golden/testdata/precedence_pathologic.json-8     73.3k ± 0%     49.0k ± 0%  -33.26%

[merschformann]

In which state is this? 🤔

[dirkschumacher]

@merschformann I want to do some further benchmarking. I'll ping you all again once that is done

[dirkschumacher]

🤔 linter

[dirkschumacher]

Waiting on #47 to fix some benchmark tests

[merschformann]

Looks good. Just left some thoughts, nothing blocking at all. Should we finally merge this?

[merschformann]

I think you can remove the TODO now, right?

[merschformann]

Maybe we can leave a comment on the func or fully write out the rec as recursive? I was confused for a minute trying to understand what this func now records. 😅
Or am I missing a point here? 🤔

[merschformann]

Technically, this can be moved down a bit, right? Not that it will matter much though. 😹

[merschformann]

It kind of makes sense that this is faster (I assume). I wonder whether it would make sense to introduce a helper func to make the intention more obvious. Something like:

func recycleSlice(s *[]int, n int) {
	*s = (*s)[:n]
}

Then again, this may defeat the purpose. 😅
Adding comments all over the place like this is probably also not great:

// we recycle the slice instead of creating a new one for performance reasons
stopOrder = stopOrder[:1]
stopOrder[0] = stopIdx

I suppose in a performance driven code-base it's fine like it is. What do you think @dirkschumacher ?

[merschformann]

Just FYI: a quick benchmark on the func approach yields this to me, but double check me as I am a bit side-tracked right now:

goos: linux
goarch: amd64
pkg: xxx
cpu: AMD Ryzen 7 5800H with Radeon Graphics         
BenchmarkRecycleInline-16      	1000000000	         1.877 ns/op	       8 B/op	       0 allocs/op
BenchmarkRecycleFunction-16    	1000000000	         1.164 ns/op	       8 B/op	       0 allocs/op
BenchmarkRecycleNew-16         	91827982	        12.27 ns/op	      16 B/op	       1 allocs/op
PASS

Based on this code:

package recycle_slice_test

import "testing"

func recycleSlice(s *[]int, n int) {
	*s = (*s)[:n]
}

func BenchmarkRecycleInline(b *testing.B) {
	slice := make([]int, b.N)
	value := 20
	for i := 0; i < b.N; i++ {
		slice = slice[:1]
		slice[0] = value
	}
}

func BenchmarkRecycleFunction(b *testing.B) {
	slice := make([]int, b.N)
	value := 20
	for i := 0; i < b.N; i++ {
		recycleSlice(&slice, 1)
		slice[0] = value
	}
}

func BenchmarkRecycleNew(b *testing.B) {
	slice := make([]int, b.N)
	value := 20
	for i := 0; i < b.N; i++ {
		slice = make([]int, 1)
		slice[0] = value
	}
}

[dirkschumacher]

I personally find

stopOrder = stopOrder[:1]
stopOrder[0] = stopIdx

relatively clear without a comment.