From 073699ea1342b90ed0ca28ab39c90bc71c223e9a Mon Sep 17 00:00:00 2001 From: Tyler French <66684063+tyler-french@users.noreply.github.com> Date: Fri, 15 Dec 2023 11:05:01 -0500 Subject: [PATCH] feat(resolve): optimize applying overrides to be efficient This PR addresses the inefficiency of processing `# gazelle:resolve` directives. Previously, the system iterated over a slice for each import to find matching directives, which is very inefficient. This PR introduces two major improvements: 1. **Transition from Slice to Map**: We've shifted from using a slice to a map for storing override configurations. This change reduces lookup time complexity from O(n) to O(1) (or O(length of chain of enclosing dirs containing resolves)). 2. **Parent-Child Config Structure**: To maintain directory-specific configurations without the overhead of copying maps (which is very inefficient), we've implemented a parent-child relationship in configurations. This structure allows each directory to have its unique overrides while inheriting unmodified settings from its parent. We can't use a single shared map, because this could mistakenly apply directives from one directory to another. `regexp` overrides are always copied and evaluated inline. Note on GC: to avoid a really complex object tree, the parent/child relationship skips all ancestors without `# gazelle:resolve` directives, greatly reducing the complexity. Note of Regexp: although we need to iterate through these due to the matching logic, we could at least have a `map: lang -> []overrides` to improve the lookup some. Fixes #1688 --- resolve/BUILD.bazel | 15 +++- resolve/config.go | 169 ++++++++++++++++++++++------------------ resolve/resolve_test.go | 126 ++++++++++++++++++++++++++++++ 3 files changed, 235 insertions(+), 75 deletions(-) create mode 100644 resolve/resolve_test.go diff --git a/resolve/BUILD.bazel b/resolve/BUILD.bazel index 02936e246..0acd29e3c 100644 --- a/resolve/BUILD.bazel +++ b/resolve/BUILD.bazel @@ -1,4 +1,4 @@ -load("@io_bazel_rules_go//go:def.bzl", "go_library") +load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test") go_library( name = "resolve", @@ -23,6 +23,7 @@ filegroup( "BUILD.bazel", "config.go", "index.go", + "resolve_test.go", ], visibility = ["//visibility:public"], ) @@ -32,3 +33,15 @@ alias( actual = ":resolve", visibility = ["//visibility:public"], ) + +go_test( + name = "resolve_test", + srcs = ["resolve_test.go"], + embed = [":resolve"], + deps = [ + "//config", + "//label", + "//rule", + "@com_github_google_go_cmp//cmp", + ], +) diff --git a/resolve/config.go b/resolve/config.go index 8f8dc535c..d5eba0259 100644 --- a/resolve/config.go +++ b/resolve/config.go @@ -32,11 +32,8 @@ import ( // returned first. If no override is found, label.NoLabel is returned. func FindRuleWithOverride(c *config.Config, imp ImportSpec, lang string) (label.Label, bool) { rc := getResolveConfig(c) - for i := len(rc.overrides) - 1; i >= 0; i-- { - o := rc.overrides[i] - if o.matches(imp, lang) { - return o.dep, true - } + if dep, ok := rc.findOverride(imp, lang); ok { + return dep, true } for i := len(rc.regexpOverrides) - 1; i >= 0; i-- { o := rc.regexpOverrides[i] @@ -44,20 +41,12 @@ func FindRuleWithOverride(c *config.Config, imp ImportSpec, lang string) (label. return o.dep, true } } - return label.NoLabel, false } -type overrideSpec struct { +type overrideKey struct { imp ImportSpec lang string - dep label.Label -} - -func (o overrideSpec) matches(imp ImportSpec, lang string) bool { - return imp.Lang == o.imp.Lang && - imp.Imp == o.imp.Imp && - (o.lang == "" || o.lang == lang) } type regexpOverrideSpec struct { @@ -74,8 +63,37 @@ func (o regexpOverrideSpec) matches(imp ImportSpec, lang string) bool { } type resolveConfig struct { - overrides []overrideSpec + overrides map[overrideKey]label.Label regexpOverrides []regexpOverrideSpec + parent *resolveConfig +} + +// newResolveConfig creates a new resolveConfig with the given overrides and +// regexpOverrides. If the new overrides are the same as the parent's, the +// parent is returned instead. +func newResolveConfig(parent *resolveConfig, newOverrides map[overrideKey]label.Label, regexpOverrides []regexpOverrideSpec) *resolveConfig { + if len(newOverrides) == 0 && len(regexpOverrides) == len(parent.regexpOverrides) { + return parent + } + return &resolveConfig{ + overrides: newOverrides, + regexpOverrides: regexpOverrides, + parent: parent, + } +} + +// findOverride searches the current configuration for an override matching +// the given import and language. If no override is found, the parent +// configuration is searched recursively. +func (rc *resolveConfig) findOverride(imp ImportSpec, lang string) (label.Label, bool) { + key := overrideKey{imp: imp, lang: lang} + if dep, ok := rc.overrides[key]; ok { + return dep, ok + } + if rc.parent != nil { + return rc.parent.findOverride(imp, lang) + } + return label.NoLabel, false } const resolveName = "_resolve" @@ -97,78 +115,81 @@ func (*Configurer) KnownDirectives() []string { } func (*Configurer) Configure(c *config.Config, rel string, f *rule.File) { - rc := getResolveConfig(c) - rcCopy := &resolveConfig{ - overrides: rc.overrides[:len(rc.overrides):len(rc.overrides)], - regexpOverrides: rc.regexpOverrides[:len(rc.regexpOverrides):len(rc.regexpOverrides)], + if f == nil || len(f.Directives) == 0 { + return } - if f != nil { - for _, d := range f.Directives { - if d.Key == "resolve" { - parts := strings.Fields(d.Value) - o := overrideSpec{} - var lbl string - if len(parts) == 3 { - o.imp.Lang = parts[0] - o.imp.Imp = parts[1] - lbl = parts[2] - } else if len(parts) == 4 { - o.imp.Lang = parts[0] - o.lang = parts[1] - o.imp.Imp = parts[2] - lbl = parts[3] - } else { - log.Printf("could not parse directive: %s\n\texpected gazelle:resolve source-language [import-language] import-string label", d.Value) - continue - } + rc := getResolveConfig(c) + var newOverrides map[overrideKey]label.Label + regexpOverrides := rc.regexpOverrides[:len(rc.regexpOverrides):len(rc.regexpOverrides)] + + for _, d := range f.Directives { + if d.Key == "resolve" { + parts := strings.Fields(d.Value) + key := overrideKey{} + var lbl string + if len(parts) == 3 { + key.imp.Lang = parts[0] + key.lang = parts[0] + key.imp.Imp = parts[1] + lbl = parts[2] + } else if len(parts) == 4 { + key.imp.Lang = parts[0] + key.lang = parts[1] + key.imp.Imp = parts[2] + lbl = parts[3] + } else { + log.Printf("could not parse directive: %s\n\texpected gazelle:resolve source-language [import-language] import-string label", d.Value) + continue + } + dep, err := label.Parse(lbl) + if err != nil { + log.Printf("gazelle:resolve %s: %v", d.Value, err) + continue + } + dep = dep.Abs("", rel) + if newOverrides == nil { + newOverrides = make(map[overrideKey]label.Label, len(f.Directives)) + } + newOverrides[key] = dep + } else if d.Key == "resolve_regexp" { + parts := strings.Fields(d.Value) + o := regexpOverrideSpec{} + var lbl string + if len(parts) == 3 { + o.ImpLang = parts[0] var err error - o.dep, err = label.Parse(lbl) + o.ImpRegex, err = regexp.Compile(parts[1]) if err != nil { - log.Printf("gazelle:resolve %s: %v", d.Value, err) - continue - } - o.dep = o.dep.Abs("", rel) - rcCopy.overrides = append(rcCopy.overrides, o) - } else if d.Key == "resolve_regexp" { - parts := strings.Fields(d.Value) - o := regexpOverrideSpec{} - var lbl string - if len(parts) == 3 { - o.ImpLang = parts[0] - var err error - o.ImpRegex, err = regexp.Compile(parts[1]) - if err != nil { - log.Printf("gazelle:resolve_exp %s: %v", d.Value, err) - continue - } - lbl = parts[2] - } else if len(parts) == 4 { - o.ImpLang = parts[0] - o.lang = parts[1] - var err error - o.ImpRegex, err = regexp.Compile(parts[2]) - if err != nil { - log.Printf("gazelle:resolve_exp %s: %v", d.Value, err) - continue - } - - lbl = parts[3] - } else { - log.Printf("could not parse directive: %s\n\texpected gazelle:resolve_regexp source-language [import-language] import-string-regex label", d.Value) + log.Printf("gazelle:resolve_regexp %s: %v", d.Value, err) continue } + lbl = parts[2] + } else if len(parts) == 4 { + o.ImpLang = parts[0] + o.lang = parts[1] var err error - o.dep, err = label.Parse(lbl) + o.ImpRegex, err = regexp.Compile(parts[2]) if err != nil { log.Printf("gazelle:resolve_regexp %s: %v", d.Value, err) continue } - o.dep = o.dep.Abs("", rel) - rcCopy.regexpOverrides = append(rcCopy.regexpOverrides, o) + + lbl = parts[3] + } else { + log.Printf("could not parse directive: %s\n\texpected gazelle:resolve_regexp source-language [import-language] import-string-regex label", d.Value) + continue + } + var err error + o.dep, err = label.Parse(lbl) + if err != nil { + log.Printf("gazelle:resolve_regexp %s: %v", d.Value, err) + continue } + o.dep = o.dep.Abs("", rel) + regexpOverrides = append(regexpOverrides, o) } } - c.Exts[resolveName] = rcCopy + c.Exts[resolveName] = newResolveConfig(rc, newOverrides, regexpOverrides) } diff --git a/resolve/resolve_test.go b/resolve/resolve_test.go new file mode 100644 index 000000000..c3e87f1cb --- /dev/null +++ b/resolve/resolve_test.go @@ -0,0 +1,126 @@ +package resolve + +import ( + "testing" + + "github.com/bazelbuild/bazel-gazelle/rule" + "github.com/bazelbuild/bazel-gazelle/config" + "github.com/bazelbuild/bazel-gazelle/label" + "github.com/google/go-cmp/cmp" +) + +func TestFindRuleWithOverride_ParentTraversal(t *testing.T) { + rootCfg := getConfig(t, "", []rule.Directive{ + {Key: "resolve", Value: "go go github.com/root/repo @com_example//root:replacement"}, + {Key: "resolve_regexp", Value: "go ^github.com/root/(.*)$ @com_example//regexp:replacement"}, + }, nil) + + childCfg := getConfig(t, "child/rel", []rule.Directive{ + {Key: "resolve", Value: "go github.com/child/repo //some/local/child:replacement"}, + {Key: "resolve_regexp", Value: "go ^github.com/child/(.*)$ relative/child/regexp"}, + }, rootCfg) + + secondChildCfg := getConfig(t, "second/child/rel", nil, rootCfg) + + tests := []struct { + name string + cfg *config.Config + importSpec ImportSpec + lang string + want label.Label + wantFound bool + }{ + { + name: "Child exact match", + cfg: childCfg, + importSpec: ImportSpec{Lang: "go", Imp: "github.com/child/repo"}, + lang: "go", + want: getTestLabel(t, "//some/local/child:replacement"), + wantFound: true, + }, + { + name: "Child regexp match", + cfg: childCfg, + importSpec: ImportSpec{Lang: "go", Imp: "github.com/child/other"}, + lang: "go", + want: getTestLabel(t, "//child/rel:relative/child/regexp"), + wantFound: true, + }, + { + name: "Root exact match from child", + cfg: childCfg, + importSpec: ImportSpec{Lang: "go", Imp: "github.com/root/repo"}, + lang: "go", + want: getTestLabel(t, "@com_example//root:replacement"), + wantFound: true, + }, + { + name: "Root regexp match from child", + cfg: childCfg, + importSpec: ImportSpec{Lang: "go", Imp: "github.com/root/some"}, + lang: "go", + want: getTestLabel(t, "@com_example//regexp:replacement"), + wantFound: true, + }, + { + name: "No match in child or root", + cfg: childCfg, + importSpec: ImportSpec{Lang: "go", Imp: "github.com/nonexistent/repo"}, + lang: "go", + want: label.NoLabel, + wantFound: false, + }, + { + name: "Second child does not find child directive", + cfg: secondChildCfg, + importSpec: ImportSpec{Lang: "go", Imp: "github.com/child/repo"}, + lang: "go", + want: label.NoLabel, + wantFound: false, + }, + { + name: "Second child finds root directive", + cfg: secondChildCfg, + importSpec: ImportSpec{Lang: "go", Imp: "github.com/root/repo"}, + lang: "go", + want: getTestLabel(t, "@com_example//root:replacement"), + wantFound: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, found := FindRuleWithOverride(tt.cfg, tt.importSpec, tt.lang) + if found != tt.wantFound { + t.Fatalf("FindRuleWithOverride() found = %v, wantFound %v", found, tt.wantFound) + } + if diff := cmp.Diff(tt.want, got); diff != "" { + t.Errorf("FindRuleWithOverride() mismatch (-want +got):\n%s", diff) + } + }) + } +} + +func getConfig(t *testing.T, path string, directives []rule.Directive, parent *config.Config) *config.Config { + cfg := &config.Config{ + Exts: map[string]interface{}{}, + } + configurer := &Configurer{} + configurer.RegisterFlags(nil, "", cfg) + configurer.CheckFlags(nil, cfg) + + if parent != nil { + cfg.Exts[resolveName] = parent.Exts[resolveName] + } + + configurer.Configure(cfg, path, &rule.File{Directives: directives}) + return cfg +} + +func getTestLabel(t *testing.T, str string) label.Label { + l, err := label.Parse(str) + if err != nil { + t.Fatal(err) + } + return l +}