Symbol search: improve performance for anchored queries #682
Conversation
matchtree.go
Outdated
| anchored := false | ||
| expr := s.Expr | ||
| switch e := s.Expr.(type) { | ||
| case *query.Regexp: | ||
| pattern := e.Regexp.String() | ||
| if strings.HasPrefix(pattern, "^") && strings.HasSuffix(pattern, "$") { | ||
| pattern = pattern[1 : len(pattern)-1] | ||
| parsedPattern, err := syntax.Parse(pattern, e.Regexp.Flags) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| eCopy := *e | ||
| eCopy.Regexp = parsedPattern | ||
| expr = &eCopy | ||
| anchored = true | ||
| } | ||
| } |
There was a problem hiding this comment.
When constructing the match tree for symbols, try to strip anchors off the input pattern. We keep track of whether the pattern was anchored, which we then use to post-filter the results so we only return ones that exactly match the symbol boundaries.
matchtree.go
Outdated
| if t.anchored && !(start == sections[secIdx].Start && end == sections[secIdx].End) { | ||
| t.current = t.current[1:] | ||
| continue | ||
| } |
There was a problem hiding this comment.
If the pattern was anchored, check that the match is not just contained in a symbol's range, but matches it exactly.
There was a problem hiding this comment.
based on this code should the variable name be more something along the lines of exact? This is on substr not a regexp, I think anchored can be misinterpreted a bit. There are others parts in the zoekt API where we use the word exact to mean the string must match exactly.
| } | ||
| } | ||
|
|
||
| subMT, err := d.newMatchTree(expr, optCopy) |
There was a problem hiding this comment.
I looked at putting this logic into newMatchTree, but the problem is that anchors aren't meaningful until we get to the symbol layer because we want the anchors to match the start and end of the symbol name which are not available lower in the tree.
There was a problem hiding this comment.
Overall the idea looks good to me and nice find. I think how you decide this in newMatchTree needs to be more robust/clearer before merging.
Looking at the symbolSubstrMatchTree with fresh eyes, I can see it has lots of opportunity to be better. From what I can tell we find all substring matches, then filter those based on document sections. It feels like we could treat document sections like we treat the trigram iterator. IE use it to help filter candidate matches inside the substring match tree. That can be a follow-up.
Out of interest here is another perf PR we never finished up to help speed up regex search #216
matchtree.go
Outdated
| if t.anchored && !(start == sections[secIdx].Start && end == sections[secIdx].End) { | ||
| t.current = t.current[1:] | ||
| continue | ||
| } |
There was a problem hiding this comment.
based on this code should the variable name be more something along the lines of exact? This is on substr not a regexp, I think anchored can be misinterpreted a bit. There are others parts in the zoekt API where we use the word exact to mean the string must match exactly.
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| if substr, ok := subMT.(*substrMatchTree); ok { | ||
| return &symbolSubstrMatchTree{ | ||
| substrMatchTree: substr, | ||
| exact: wasAnchored, |
There was a problem hiding this comment.
one last bug here, I think the regexp we pass to symbolRegexpMatchTree (if we get to that path) will now exclude anchors.
|
Going to close this for now since the perf benefit I was aiming for came mostly from fixing the |
Search-based code intel makes heavy use of exact, anchored searches for symbol names like
^NewFoo$. It uses regex patterns to ensure that the symbol name exactly matches the symbol being hovered over. Zoekt does not efficiently handle anchors, falling back to a brute-force regex search that executes the regex against every symbol in the index.This is a dirty PR that fixes the issue, but in a probably non-ideal way. I'm looking for early feedback on whether this is the correct approach. If so, I'll do some more extensive and re-request review before merge.
Explanations inline.