Skip to content

perf: even faster trie #327

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
lczyk wants to merge 2 commits into
base: main
Choose a base branch
from
Open

perf: even faster trie #327

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
06a1c52
perf: reuse conflict traversal queues
lczyk Jul 10, 2026
abb0ccc
perf: exact-match child lookup in traversal
lczyk Jul 10, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 41 additions & 6 deletions internal/setup/conflict.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,10 @@ type node struct {
Segment segment
SegmentSlices []*segmentSlice
Children map[string]*node
// GlobChildren lists the children whose segment contains a wildcard, so

@letFunny letFunny Oct 2, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This also looks okay to me. There are many optimizations optimizations we can do when looking for Children to reduce the amount of comparisons. GlobChildren seems a good compromise between being quite simple and having an impact on performance. On that note, what is the effect? I remember you said ~20%.

Nit: I find the comment describes the usage with regard to the other field in a bit of a convoluted way. I would instead say this is an optimization that has the subset of children with globs and it is used in appendCandidates...

// that a literal segment can find its exact match in Children and only
// scan these.
GlobChildren []*node
}

// pathConflictTree uses a custom trie to find conflicts that might arise from
Expand All @@ -52,6 +56,10 @@ type node struct {
type pathConflictTree struct {
Root *node
PathToSlices map[string][]*Slice
// currentQueue and nextQueue are kept across pathHasConflict calls so

@letFunny letFunny Oct 2, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This is okay, the downside is that we are adding state and keeping the queues in memory for as long as the object lives. I think it is a good tradeoff as the graph will also be in memory for the same amount of time.

// that the traversal does not have to grow fresh queues for every path.
currentQueue []*node
nextQueue []*node
}

var rootSegment = segment{"/", false, false}
Expand Down Expand Up @@ -100,12 +108,19 @@ func (g *pathConflictTree) pathHasConflict(newSegments []segment, newSegmentSlic
return fmt.Errorf("slices %s and %s conflict on %s and %s", oldSlice, newSlice, oldPath, newPath)
}

var currentQueue []*node
var nextQueue []*node
currentQueue := g.currentQueue[:0]
nextQueue := g.nextQueue[:0]
Comment on lines +111 to +112

@letFunny letFunny Oct 2, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Even though we now have state we are clearing it before each run which makes it safe.

@upils upils Oct 7, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Nit: With the current approach both queues are non-empty. So one could think that the stored values are meaningful and may be used by another function/method between executions of pathHasConflict. What about also clearing the queues in the deferred call?

defer func() {
// Keep the grown queues for the next call.

@upils upils Oct 7, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Maybe it should be very clear that these are kept only to avoid more allocations, and it does not serve any other purpose in the algorithm. So far the recently added complexity was essentially due to implementing the logic itself. The propose change here is a pure optimization and being able to "ignore" it when reasoning about the logic might help.

g.currentQueue, g.nextQueue = currentQueue, nextQueue
}()

// Skip "/".
currentQueue = slices.Collect(maps.Values(g.Root.Children))
newSegments = newSegments[1:]
if len(newSegments) == 0 {
return nil
}
currentQueue = appendCandidates(currentQueue, g.Root, newSegments[0])

// If we run out of segments from the graph or the path there cannot be a
// conflict.
Expand Down Expand Up @@ -152,8 +167,8 @@ func (g *pathConflictTree) pathHasConflict(newSegments []segment, newSegmentSlic
// If we are at the terminal node of both paths we found a conflict.
return conflictErrMsg(oldSegmentSlice, newSegmentSlice)
}
for _, child := range oldNode.Children {
nextQueue = append(nextQueue, child)
if len(newSegments) > 1 {
nextQueue = appendCandidates(nextQueue, oldNode, newSegments[1])
}
break oldNodeLoop
} else {
Expand All @@ -174,6 +189,23 @@ func (g *pathConflictTree) pathHasConflict(newSegments []segment, newSegmentSlic
return nil
}

// appendCandidates appends the children of parent that can possibly match
// seg. A segment with a wildcard can match any child, but a literal segment
// can only match the child with the exact same text or children with
// wildcards.
func appendCandidates(queue []*node, parent *node, seg segment) []*node {
if seg.HasGlob {
for _, child := range parent.Children {
queue = append(queue, child)
}
return queue
Comment on lines +198 to +201

@upils upils Oct 7, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
for _, child := range parent.Children {
queue = append(queue, child)
}
return queue
return slices.AppendSeq(queue, maps.Values(parent.Children))

}
if child, ok := parent.Children[seg.Text]; ok {
queue = append(queue, child)
}
return append(queue, parent.GlobChildren...)
}

// insertSegments inserts the path's segments blindly in the graph without
// looking at conflicts.
func (g *pathConflictTree) insertSegments(segments []segment, segmentSlices []*segmentSlice) {
Expand All @@ -188,9 +220,12 @@ func (g *pathConflictTree) insertSegments(segments []segment, segmentSlices []*s
Segment: segment,
Children: map[string]*node{},
}
parent.Children[segment.Text] = current
if segment.HasGlob {
parent.GlobChildren = append(parent.GlobChildren, current)
}
}
current.SegmentSlices = append(current.SegmentSlices, segmentSlices...)
parent.Children[segment.Text] = current
parent = current
}
}
Expand Down
Loading