From 526e02f0c68dc1cd0545ffc8ea51db577faa7575 Mon Sep 17 00:00:00 2001 From: Colin Cross Date: Thu, 21 Jun 2018 13:31:53 -0700 Subject: [PATCH] Prevent duplicate visit calls in WalkDeps WalkDeps was following every possible path through the dependency tree, which can be enormous. Modify it to only call visit for any particular (child, parent) pair once for each direct dependency by not recursing into child if visitDown returns true but child has already been visited. Test: TestWalkDeps, TestWalkDepsDuplicates Change-Id: Ieef28399bd10e744417cdeb661dfa04fbeb4ec60 --- context.go | 4 ++-- context_test.go | 49 +++++++++++++++++++++++++------------------------ module_ctx.go | 10 +++++----- 3 files changed, 32 insertions(+), 31 deletions(-) diff --git a/context.go b/context.go index 11ee6f0..919377c 100644 --- a/context.go +++ b/context.go @@ -2397,15 +2397,15 @@ func (c *Context) walkDeps(topModule *moduleInfo, allowDuplicates bool, walk = func(module *moduleInfo) { for _, dep := range module.directDeps { if allowDuplicates || !visited[dep.module] { - visited[dep.module] = true visiting = dep.module recurse := true if visitDown != nil { recurse = visitDown(dep, module) } - if recurse { + if recurse && !visited[dep.module] { walk(dep.module) } + visited[dep.module] = true if visitUp != nil { visitUp(dep, module) } diff --git a/context_test.go b/context_test.go index c08b0b7..bc21a1c 100644 --- a/context_test.go +++ b/context_test.go @@ -122,9 +122,9 @@ func TestContextParse(t *testing.T) { } } -// |---B===D - represents a non-walkable edge +// |===B---D - represents a non-walkable edge // A = represents a walkable edge -// |===C---E===G +// |===C===E---G // | | A should not be visited because it's the root node. // |===F===| B, D and E should not be walked. func TestWalkDeps(t *testing.T) { @@ -191,31 +191,29 @@ func TestWalkDeps(t *testing.T) { topModule := ctx.modulesFromName("A", nil)[0] ctx.walkDeps(topModule, false, func(dep depInfo, parent *moduleInfo) bool { + outputDown += ctx.ModuleName(dep.module.logicModule) if dep.module.logicModule.(Walker).Walk() { - outputDown += ctx.ModuleName(dep.module.logicModule) return true } return false }, func(dep depInfo, parent *moduleInfo) { - if dep.module.logicModule.(Walker).Walk() { - outputUp += ctx.ModuleName(dep.module.logicModule) - } + outputUp += ctx.ModuleName(dep.module.logicModule) }) - if outputDown != "CFG" { - t.Errorf("unexpected walkDeps behaviour: %s\ndown should be: CFG", outputDown) + if outputDown != "BCEFG" { + t.Errorf("unexpected walkDeps behaviour: %s\ndown should be: BCEFG", outputDown) } - if outputUp != "GFC" { - t.Errorf("unexpected walkDeps behaviour: %s\nup should be: GFC", outputUp) + if outputUp != "BEGFC" { + t.Errorf("unexpected walkDeps behaviour: %s\nup should be: BEGFC", outputUp) } } -// |---B===D - represents a non-walkable edge -// A = represents a walkable edge -// |===C===E===\ -// | | A should not be visited because it's the root node. -// |===F===G B, D should not be walked. -// \===/ G should be visited multiple times +// |===B---D - represents a non-walkable edge +// A = represents a walkable edge +// |===C===E===\ A should not be visited because it's the root node. +// | | B, D should not be walked. +// |===F===G===H G should be visited multiple times +// \===/ H should only be visited once func TestWalkDepsDuplicates(t *testing.T) { ctx := NewContext() ctx.MockFileSystem(map[string][]byte{ @@ -251,6 +249,11 @@ func TestWalkDepsDuplicates(t *testing.T) { foo_module { name: "G", + deps: ["H"], + } + + foo_module { + name: "H", } `), }) @@ -280,22 +283,20 @@ func TestWalkDepsDuplicates(t *testing.T) { topModule := ctx.modulesFromName("A", nil)[0] ctx.walkDeps(topModule, true, func(dep depInfo, parent *moduleInfo) bool { + outputDown += ctx.ModuleName(dep.module.logicModule) if dep.module.logicModule.(Walker).Walk() { - outputDown += ctx.ModuleName(dep.module.logicModule) return true } return false }, func(dep depInfo, parent *moduleInfo) { - if dep.module.logicModule.(Walker).Walk() { - outputUp += ctx.ModuleName(dep.module.logicModule) - } + outputUp += ctx.ModuleName(dep.module.logicModule) }) - if outputDown != "CEGFGG" { - t.Errorf("unexpected walkDeps behaviour: %s\ndown should be: CEGFGG", outputDown) + if outputDown != "BCEGHFGG" { + t.Errorf("unexpected walkDeps behaviour: %s\ndown should be: BCEGHFGG", outputDown) } - if outputUp != "GEGGFC" { - t.Errorf("unexpected walkDeps behaviour: %s\nup should be: GEGGFC", outputUp) + if outputUp != "BHGEGGFC" { + t.Errorf("unexpected walkDeps behaviour: %s\nup should be: BHGEGGFC", outputUp) } } diff --git a/module_ctx.go b/module_ctx.go index 200fd9a..62646f1 100644 --- a/module_ctx.go +++ b/module_ctx.go @@ -164,7 +164,7 @@ type ModuleContext interface { VisitDirectDepsIf(pred func(Module) bool, visit func(Module)) VisitDepsDepthFirst(visit func(Module)) VisitDepsDepthFirstIf(pred func(Module) bool, visit func(Module)) - WalkDeps(visit func(Module, Module) bool) + WalkDeps(visit func(child, parent Module) bool) ModuleSubDir() string @@ -446,10 +446,10 @@ func (m *baseModuleContext) VisitDepsDepthFirstIf(pred func(Module) bool, } // WalkDeps calls visit for each transitive dependency, traversing the dependency tree in top down order. visit may be -// called multiple times for the same module if there are multiple paths through the dependency tree to the module or -// multiple direct dependencies with different tags. OtherModuleDependencyTag will return the tag for the currently -// visited path to the module. If visit returns false WalkDeps will not continue recursing down the current path. -func (m *baseModuleContext) WalkDeps(visit func(Module, Module) bool) { +// called multiple times for the same (child, parent) pair if there are multiple direct dependencies between the +// child and parent with different tags. OtherModuleDependencyTag will return the tag for the currently visited +// (child, parent) pair. If visit returns false WalkDeps will not continue recursing down to child. +func (m *baseModuleContext) WalkDeps(visit func(child, parent Module) bool) { m.context.walkDeps(m.module, true, func(dep depInfo, parent *moduleInfo) bool { m.visitingParent = parent m.visitingDep = dep -- GitLab