perf: speed up path conflict detection

[lczyk]

• Have you signed the CLA?

---

two optimisations for pathConflictTree, on top of the trie from this branch. no behaviour change intended: same conflicts detected, same error type. tests pass unchanged. checked with my fuzz tests too.

• perf: use a custom trie for path conflict validation canonical/chisel#302

1. reuse the traversal queues (f449d7a)

pathHasConflict used to grow currentQueue and nextQueue from nil on every path insertion, and seeded the first level with slices.Collect(maps.Values(...)). with a few thousand paths this dominated allocation: the queues were the single largest source of gc work in the whole conflict check.

i've changed currentQueue and nextQueue to fields on pathConflictTree, reset with [:0] per call and restored through a defer, so the backing arrays are grown once and reused.

proof of gc work (drop somewhere in internal/setup):

func BenchmarkConflictAlloc(b *testing.B) {
      paths := map[string][]*setup.Slice{}
      for i := 0; i < 1000; i++ {
              pkg := fmt.Sprintf("pkg%04d", i)
              p := "/usr/bin/" + pkg + "-*"
              paths[p] = []*setup.Slice{{Package: pkg, Name: "s",
                      Contents: map[string]setup.PathInfo{p: {Kind: setup.GlobPath}}}}
      }
      for i := 0; i < b.N; i++ {
              tree := setup.NewConflictTree(paths)
              if err := tree.HasConflict(); err != nil {
                      b.Fatal(err)
              }
      }
}

$ go test ./internal/setup/ -run '^$' -bench ConflictAlloc -benchtime 20x \
  -memprofile /tmp/mem.prof -o /tmp/base.test
$ go tool pprof -list 'pathHasConflict$' -sample_index=alloc_space /tmp/base.test /tmp/mem.prof | grep -E '(nextQueue =|of Total)'
  218.43MB   218.43MB (flat, cum) 90.63% of Total
  218.43MB   218.43MB    156:								nextQueue = append(nextQueue, child)
$ GODEBUG=gctrace=1 go test ./internal/setup/ -run '^$' -bench ConflictAlloc -benchtime 20x 2>&1 | grep '^gc ' | wc -l
     149

after:

$ go tool pprof -list 'pathHasConflict$' -sample_index=alloc_space /tmp/base.test /tmp/mem.prof | grep -E '(nextQueue =|of Total)'
  514.38kB   514.38kB (flat, cum)  1.72% of Total
         .          .    111:		g.currentQueue, g.nextQueue = currentQueue, nextQueue
  514.38kB   514.38kB    166:								nextQueue = append(nextQueue, child)
$ GODEBUG=gctrace=1 go test ./internal/setup/ -run '^$' -bench ConflictAlloc -benchtime 20x 2>&1 | grep '^gc ' | wc -l
      40

2. exact-match child lookup (3c097a7)

when a segment matched, the old traversal enqueued every child of the matched node and compared each one against the next segment on the following round. but Children is already keyed by segment text, so for a literal next-segment the only children that can possibly match are the one with the identical text and
any child whose segment holds a wildcard.

now appendCandidates does and exact map lookup, plus a scan of the new node.GlobChildren list. a wildcard next-segment still considers every child, since it can match anything. glob-vs-glob comparisons are unchanged.

this turns the linear sibling scan in wide directories into a map lookup. real chisel-releases paths are at present aboout ~60% literal directories and this speeds this up, especially because the wide nodes tend to also be mostly literal so the quadratic scan hurts a lot there).

proof, with fix 1 above applied, otherwise GC noise swamps the signal:

func BenchmarkConflictWideDir(b *testing.B) {
      for _, n := range []int{1250, 2500, 5000} {
              paths := map[string][]*setup.Slice{}
              for i := 0; i < n; i++ {
                      pkg := fmt.Sprintf("pkg%04d", i)
                      p := fmt.Sprintf("/usr/share/doc/file%05d", i)
                      paths[p] = []*setup.Slice{{Package: pkg, Name: "s",
                              Contents: map[string]setup.PathInfo{p: {Kind: setup.CopyPath}}}}
              }
              b.Run(fmt.Sprintf("files=%d", n), func(b *testing.B) {
                      for i := 0; i < b.N; i++ {
                              tree := setup.NewConflictTree(paths)
                              if err := tree.HasConflict(); err != nil {
                                      b.Fatal(err)
                              }
                      }
              })
      }
}

$ go test ./internal/setup/ -run '^$' -bench ConflictWideDir -count=1 | grep 'BenchmarkConflictWideDir'
BenchmarkConflictWideDir/files=1250-8         	      68	  14806423 ns/op
BenchmarkConflictWideDir/files=2500-8         	      16	  69988763 ns/op
BenchmarkConflictWideDir/files=5000-8         	       4	 273993969 ns/op

after:

go test ./internal/setup/ -run '^$' -bench ConflictWideDir -count=1 | grep 'BenchmarkConflictWideDir'
BenchmarkConflictWideDir/files=1250-8         	    1753	    679446 ns/op
BenchmarkConflictWideDir/files=2500-8         	     903	   1294642 ns/op
BenchmarkConflictWideDir/files=5000-8         	     466	   2629180 ns/op

[letFunny]

Thank you looking into this Marcin, I added comment about each improvement but basically IMO one of them should certainly go in, and for the other one we can discuss it.

With that said, I think it is better if we wait for the original PR to be merged. I spoke with Gustavo and he was very clear in that he didn't want to pile up more improvements; he wanted to have a clear algorithm first then improve it later. Let's do the following, once the original PR is merged you can create a new PR with these improvements, I wouldn't want to claim authorship :)

[letFunny]

When writing the original PR I cached the queues, then I removed it, then added it back, etc. and in the end I don't have a strong opinion on whether to do it or not. It obviously increases performance (around 20% on my machine, so 20ms out of 100ms). The only reason I didn't add it from the get go was a similar case in the past where there was an optimization by caching the results and Gustavo argued against it as it increased the complexity.

After seeing you came up with the same idea I think we should start the debate again.

[letFunny]

This is very interesting. I didn't expect such a simple change to have 25% improvement (my machine). I had done another more complex optimization for looking for candidates that had the same % improvement but the implementation was far worse.

I think we should go for this one.

[lczyk]

yep. no problem :)) happy to submit it afterwards and yeah, i agree that it's better to split them. i wanted to look into this now in case 1) there would be a l-line improvement or 2) there would be a bigger perf gain but behind some slight reengineer which might as well land at this stage. none of those are the case and yeah, this PR can easily go in afterwards.

re the improvements here; they are somewhat coupled, bit throughout but through effect. i did add this line from my initial message, but it's just one and maybe i should have been clearer about it:

proof, with fix 1 above applied, otherwise GC noise swamps the signal: ...

fix 2 by itself works ok, but does induce much more GC pressure w/out fix 1.

tbd after your bade PR is merged and we send this one in, i think 👍