Skip to content

Commit f1c8f7f

Browse files
committed
internal/lsp/source: optimize filter regular expression
When the default filter glob changed to **/node_modules, the performance of BenchmarkWorkspaceSymbols was observed to degrade by an astonishing 20%. (CPU profiles of the benchmark reported that the Disallow functions percentage had increased only slightly, but these measures are misleading since the benchmark has a very CPU-intensive set-up step, so all the percentages are quotients of this figure, masking their relative importance to the small region during which the benchmark timer is running.) This change removes the unnecessary ^.* prefix from the generated regular expression. Really the regexp package ought to do this. Also, minor cleanups and tweaks to the surrounding code. Change-Id: I806aad810ce2e7bbfb2c9b04009d8db752a3b10d Reviewed-on: https://go-review.googlesource.com/c/tools/+/446177 Run-TryBot: Alan Donovan <[email protected]> gopls-CI: kokoro <[email protected]> TryBot-Result: Gopher Robot <[email protected]> Reviewed-by: Robert Findley <[email protected]>
1 parent e074ef8 commit f1c8f7f

File tree

1 file changed

+18
-10
lines changed

1 file changed

+18
-10
lines changed

gopls/internal/lsp/source/workspace_symbol.go

Lines changed: 18 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -379,6 +379,7 @@ func NewFilterer(rawFilters []string) *Filterer {
379379
var f Filterer
380380
for _, filter := range rawFilters {
381381
filter = path.Clean(filepath.ToSlash(filter))
382+
// TODO(dungtuanle): fix: validate [+-] prefix.
382383
op, prefix := filter[0], filter[1:]
383384
// convertFilterToRegexp adds "/" at the end of prefix to handle cases where a filter is a prefix of another filter.
384385
// For example, it prevents [+foobar, -foo] from excluding "foobar".
@@ -391,20 +392,19 @@ func NewFilterer(rawFilters []string) *Filterer {
391392

392393
// Disallow return true if the path is excluded from the filterer's filters.
393394
func (f *Filterer) Disallow(path string) bool {
395+
// Ensure trailing but not leading slash.
394396
path = strings.TrimPrefix(path, "/")
395-
var excluded bool
397+
if !strings.HasSuffix(path, "/") {
398+
path += "/"
399+
}
396400

401+
// TODO(adonovan): opt: iterate in reverse and break at first match.
402+
excluded := false
397403
for i, filter := range f.filters {
398-
path := path
399-
if !strings.HasSuffix(path, "/") {
400-
path += "/"
401-
}
402-
if !filter.MatchString(path) {
403-
continue
404+
if filter.MatchString(path) {
405+
excluded = f.excluded[i] // last match wins
404406
}
405-
excluded = f.excluded[i]
406407
}
407-
408408
return excluded
409409
}
410410

@@ -419,15 +419,23 @@ func convertFilterToRegexp(filter string) *regexp.Regexp {
419419
ret.WriteString("^")
420420
segs := strings.Split(filter, "/")
421421
for _, seg := range segs {
422+
// Inv: seg != "" since path is clean.
422423
if seg == "**" {
423424
ret.WriteString(".*")
424425
} else {
425426
ret.WriteString(regexp.QuoteMeta(seg))
426427
}
427428
ret.WriteString("/")
428429
}
430+
pattern := ret.String()
431+
432+
// Remove unnecessary "^.*" prefix, which increased
433+
// BenchmarkWorkspaceSymbols time by ~20% (even though
434+
// filter CPU time increased by only by ~2.5%) when the
435+
// default filter was changed to "**/node_modules".
436+
pattern = strings.TrimPrefix(pattern, "^.*")
429437

430-
return regexp.MustCompile(ret.String())
438+
return regexp.MustCompile(pattern)
431439
}
432440

433441
// symbolFile holds symbol information for a single file.

0 commit comments

Comments
 (0)