diff --git a/context.go b/context.go index ddaea4d..bbc6246 100644 --- a/context.go +++ b/context.go @@ -3999,59 +3999,46 @@ func (c *Context) WriteBuildFile(w io.StringWriter) error { nw := newNinjaWriter(w) - err = c.writeBuildFileHeader(nw) - if err != nil { + if err = c.writeBuildFileHeader(nw); err != nil { return } - err = c.writeNinjaRequiredVersion(nw) - if err != nil { + if err = c.writeNinjaRequiredVersion(nw); err != nil { return } - err = c.writeSubninjas(nw) - if err != nil { + if err = c.writeSubninjas(nw); err != nil { return } // TODO: Group the globals by package. - err = c.writeGlobalVariables(nw) - if err != nil { + if err = c.writeGlobalVariables(nw); err != nil { return } - err = c.writeGlobalPools(nw) - if err != nil { + if err = c.writeGlobalPools(nw); err != nil { return } - err = c.writeBuildDir(nw) - if err != nil { + if err = c.writeBuildDir(nw); err != nil { return } - err = c.writeGlobalRules(nw) - if err != nil { + if err = c.writeGlobalRules(nw); err != nil { return } - err = c.writeAllModuleActions(nw) - if err != nil { + if err = c.writeAllModuleActions(nw); err != nil { return } - err = c.writeAllSingletonActions(nw) - if err != nil { + if err = c.writeAllSingletonActions(nw); err != nil { return } }) - if err != nil { - return err - } - - return nil + return err } type pkgAssociation struct { @@ -4332,9 +4319,10 @@ func (s moduleSorter) Swap(i, j int) { } func (c *Context) writeAllModuleActions(nw *ninjaWriter) error { + c.BeginEvent("modules") + defer c.EndEvent("modules") headerTemplate := template.New("moduleHeader") - _, err := headerTemplate.Parse(moduleHeaderTemplate) - if err != nil { + if _, err := headerTemplate.Parse(moduleHeaderTemplate); err != nil { // This is a programming error. panic(err) } @@ -4345,6 +4333,11 @@ func (c *Context) writeAllModuleActions(nw *ninjaWriter) error { } sort.Sort(moduleSorter{modules, c.nameInterface}) + phonys := c.extractPhonys(modules) + if err := c.writeLocalBuildActions(nw, phonys); err != nil { + return err + } + buf := bytes.NewBuffer(nil) for _, module := range modules { @@ -4371,28 +4364,23 @@ func (c *Context) writeAllModuleActions(nw *ninjaWriter) error { "pos": relPos, "variant": module.variant.name, } - err = headerTemplate.Execute(buf, infoMap) - if err != nil { + if err := headerTemplate.Execute(buf, infoMap); err != nil { return err } - err = nw.Comment(buf.String()) - if err != nil { + if err := nw.Comment(buf.String()); err != nil { return err } - err = nw.BlankLine() - if err != nil { + if err := nw.BlankLine(); err != nil { return err } - err = c.writeLocalBuildActions(nw, &module.actionDefs) - if err != nil { + if err := c.writeLocalBuildActions(nw, &module.actionDefs); err != nil { return err } - err = nw.BlankLine() - if err != nil { + if err := nw.BlankLine(); err != nil { return err } } @@ -4401,6 +4389,8 @@ func (c *Context) writeAllModuleActions(nw *ninjaWriter) error { } func (c *Context) writeAllSingletonActions(nw *ninjaWriter) error { + c.BeginEvent("singletons") + defer c.EndEvent("singletons") headerTemplate := template.New("singletonHeader") _, err := headerTemplate.Parse(singletonHeaderTemplate) if err != nil { @@ -4470,6 +4460,131 @@ func (c *Context) SetBeforePrepareBuildActionsHook(hookFn func() error) { c.BeforePrepareBuildActionsHook = hookFn } +// phonyCandidate represents the state of a set of deps that decides its eligibility +// to be extracted as a phony output +type phonyCandidate struct { + sync.Mutex + frequency int // the number of buildDef instances that use this set + phony *buildDef // the phony buildDef that wraps the set + first *buildDef // the first buildDef that uses this set + key string // a unique identifier for the set +} + +func (c *phonyCandidate) less(other *phonyCandidate) bool { + if c.frequency == other.frequency { + if len(c.phony.OrderOnly) == len(other.phony.OrderOnly) { + return c.key < other.key + } + return len(c.phony.OrderOnly) < len(other.phony.OrderOnly) + } + return c.frequency < other.frequency +} + +// keyForPhonyCandidate gives a unique identifier for a set of deps. +// We are not using hash because string concatenation proved cheaper. +// If any of the deps use a variable, we return an empty string to signal +// that this set of deps is ineligible for extraction. +func keyForPhonyCandidate(deps []ninjaString) string { + s := make([]string, len(deps)) + for i, d := range deps { + if len(d.Variables()) != 0 { + return "" + } + s[i] = d.Value(nil) + } + return strings.Join(s, "\n") +} + +// scanBuildDef is called for every known buildDef `b` that has a non-empty `b.OrderOnly`. +// If `b.OrderOnly` is not present in `candidates`, it gets stored. +// But if `b.OrderOnly` already exists in `candidates`, then `b.OrderOnly` +// (and phonyCandidate#first.OrderOnly) will be replaced with phonyCandidate#phony.Outputs +func scanBuildDef(wg *sync.WaitGroup, candidates *sync.Map, phonyCount *atomic.Uint32, b *buildDef) { + defer wg.Done() + key := keyForPhonyCandidate(b.OrderOnly) + if key == "" { + return + } + if v, loaded := candidates.LoadOrStore(key, &phonyCandidate{ + frequency: 1, + first: b, + key: key, + }); loaded { + m := v.(*phonyCandidate) + func() { + m.Lock() + defer m.Unlock() + if m.frequency == 1 { + // this is the second occurrence and hence it makes sense to + // extract it as a phony output + phonyCount.Add(1) + m.phony = &buildDef{ + Rule: Phony, + // We are using placeholder because we don't have a deterministic + // name for the phony output; m.key is unique and could be used but + // it's rather long (and has characters we would need to escape) + Outputs: make([]ninjaString, 1), + Inputs: m.first.OrderOnly, //we could also use b.OrderOnly + Optional: true, + } + // the previously recorded build-def, which first had these deps as its + // order-only deps, should now use this phony output instead + m.first.OrderOnly = m.phony.Outputs + m.first = nil + } + m.frequency += 1 + b.OrderOnly = m.phony.Outputs + }() + } +} + +// extractPhonys searches for common sets of order-only dependencies across all +// buildDef instances in the provided moduleInfo instances. Each such +// common set forms a new buildDef representing a phony output that then becomes +// the sole order-only dependency of those buildDef instances +func (c *Context) extractPhonys(infos []*moduleInfo) *localBuildActions { + c.BeginEvent("extract_phonys") + defer c.EndEvent("extract_phonys") + + candidates := sync.Map{} //used as map[key]*candidate + phonyCount := atomic.Uint32{} + wg := sync.WaitGroup{} + for _, info := range infos { + for _, b := range info.actionDefs.buildDefs { + if len(b.OrderOnly) > 0 { + wg.Add(1) + go scanBuildDef(&wg, &candidates, &phonyCount, b) + } + } + } + wg.Wait() + + //now filter candidates with freq > 1 + phonys := make([]*phonyCandidate, 0, phonyCount.Load()) + candidates.Range(func(_ any, v any) bool { + candidate := v.(*phonyCandidate) + if candidate.frequency > 1 { + phonys = append(phonys, candidate) + } + return true + }) + + phonyBuildDefs := make([]*buildDef, len(phonys)) + c.EventHandler.Do("name", func() { + // sorting for determinism + sort.Slice(phonys, func(i int, j int) bool { + return phonys[i].less(phonys[j]) + }) + for index, p := range phonys { + // use the index to set the name for the phony output + p.phony.Outputs[0] = literalNinjaString(fmt.Sprintf("phony-%d", index)) + phonyBuildDefs[index] = p.phony + } + }) + + return &localBuildActions{buildDefs: phonyBuildDefs} +} + func (c *Context) writeLocalBuildActions(nw *ninjaWriter, defs *localBuildActions) error { diff --git a/context_test.go b/context_test.go index 40c26c9..a398d51 100644 --- a/context_test.go +++ b/context_test.go @@ -1131,27 +1131,144 @@ func TestPackageIncludes(t *testing.T) { }, } for _, tc := range testCases { - ctx := NewContext() - // Register mock FS - ctx.MockFileSystem(mockFs) - // Register module types - ctx.RegisterModuleType("foo_module", newFooModule) - RegisterPackageIncludesModuleType(ctx) - // Add include tags for test case - ctx.AddIncludeTags(tc.includeTags...) - // Run test - _, actualErrs := ctx.ParseFileList(".", []string{"dir1/Android.bp", "dir2/Android.bp"}, nil) - // Evaluate - if !strings.Contains(fmt.Sprintf("%s", actualErrs), fmt.Sprintf("%s", tc.expectedErr)) { - t.Errorf("Expected errors: %s, got errors: %s\n", tc.expectedErr, actualErrs) - } - if tc.expectedErr != "" { - continue // expectedDir check not necessary - } - actualBpFile := ctx.moduleGroupFromName("foo", nil).modules.firstModule().relBlueprintsFile - if tc.expectedDir != filepath.Dir(actualBpFile) { - t.Errorf("Expected foo from %s, got %s\n", tc.expectedDir, filepath.Dir(actualBpFile)) - } + t.Run(tc.desc, func(t *testing.T) { + ctx := NewContext() + // Register mock FS + ctx.MockFileSystem(mockFs) + // Register module types + ctx.RegisterModuleType("foo_module", newFooModule) + RegisterPackageIncludesModuleType(ctx) + // Add include tags for test case + ctx.AddIncludeTags(tc.includeTags...) + // Run test + _, actualErrs := ctx.ParseFileList(".", []string{"dir1/Android.bp", "dir2/Android.bp"}, nil) + // Evaluate + if !strings.Contains(fmt.Sprintf("%s", actualErrs), fmt.Sprintf("%s", tc.expectedErr)) { + t.Errorf("Expected errors: %s, got errors: %s\n", tc.expectedErr, actualErrs) + } + if tc.expectedErr != "" { + return // expectedDir check not necessary + } + actualBpFile := ctx.moduleGroupFromName("foo", nil).modules.firstModule().relBlueprintsFile + if tc.expectedDir != filepath.Dir(actualBpFile) { + t.Errorf("Expected foo from %s, got %s\n", tc.expectedDir, filepath.Dir(actualBpFile)) + } + }) } } + +func TestExtractPhonys(t *testing.T) { + outputs := func(names ...string) []ninjaString { + r := make([]ninjaString, len(names)) + for i, name := range names { + r[i] = literalNinjaString(name) + } + return r + } + b := func(output string, inputs []string, orderOnlyDeps []string) *buildDef { + return &buildDef{ + Outputs: outputs(output), + Inputs: outputs(inputs...), + OrderOnly: outputs(orderOnlyDeps...), + } + } + m := func(bs ...*buildDef) *moduleInfo { + return &moduleInfo{actionDefs: localBuildActions{buildDefs: bs}} + } + type testcase struct { + modules []*moduleInfo + expectedPhonys []*buildDef + conversions map[string][]ninjaString + } + testCases := []testcase{{ + modules: []*moduleInfo{ + m(b("A", nil, []string{"d"})), + m(b("B", nil, []string{"d"})), + }, + expectedPhonys: []*buildDef{ + b("phony-0", []string{"d"}, nil), + }, + conversions: map[string][]ninjaString{ + "A": outputs("phony-0"), + "B": outputs("phony-0"), + }, + }, { + modules: []*moduleInfo{ + m(b("A", nil, []string{"a"})), + m(b("B", nil, []string{"b"})), + }, + }, { + modules: []*moduleInfo{ + m(b("A", nil, []string{"a"})), + m(b("B", nil, []string{"b"})), + m(b("C", nil, []string{"a"})), + }, + expectedPhonys: []*buildDef{b("phony-0", []string{"a"}, nil)}, + conversions: map[string][]ninjaString{ + "A": outputs("phony-0"), + "B": outputs("b"), + "C": outputs("phony-0"), + }, + }, { + modules: []*moduleInfo{ + m(b("A", nil, []string{"a", "b"}), + b("B", nil, []string{"a", "b"})), + m(b("C", nil, []string{"a", "c"}), + b("D", nil, []string{"a", "c"})), + }, + expectedPhonys: []*buildDef{ + b("phony-0", []string{"a", "b"}, nil), + b("phony-1", []string{"a", "c"}, nil)}, + conversions: map[string][]ninjaString{ + "A": outputs("phony-0"), + "B": outputs("phony-0"), + "C": outputs("phony-1"), + "D": outputs("phony-1"), + }, + }} + for index, tc := range testCases { + t.Run(fmt.Sprintf("TestCase-%d", index), func(t *testing.T) { + ctx := NewContext() + actualPhonys := ctx.extractPhonys(tc.modules) + if len(actualPhonys.variables) != 0 { + t.Errorf("No variables expected but found %v", actualPhonys.variables) + } + if len(actualPhonys.rules) != 0 { + t.Errorf("No rules expected but found %v", actualPhonys.rules) + } + if e, a := len(tc.expectedPhonys), len(actualPhonys.buildDefs); e != a { + t.Errorf("Expected %d build statements but got %d", e, a) + } + for i := 0; i < len(tc.expectedPhonys); i++ { + a := actualPhonys.buildDefs[i] + e := tc.expectedPhonys[i] + if !reflect.DeepEqual(e.Outputs, a.Outputs) { + t.Errorf("phonys expected %v but actualPhonys %v", e.Outputs, a.Outputs) + } + if !reflect.DeepEqual(e.Inputs, a.Inputs) { + t.Errorf("phonys expected %v but actualPhonys %v", e.Inputs, a.Inputs) + } + } + find := func(k string) *buildDef { + for _, m := range tc.modules { + for _, b := range m.actionDefs.buildDefs { + if reflect.DeepEqual(b.Outputs, outputs(k)) { + return b + } + } + } + return nil + } + for k, conversion := range tc.conversions { + actual := find(k) + if actual == nil { + t.Errorf("Couldn't find %s", k) + } + if !reflect.DeepEqual(actual.OrderOnly, conversion) { + t.Errorf("expected %s.OrderOnly = %v but got %v", k, conversion, actual.OrderOnly) + } + } + }) + } +} diff --git a/go.mod b/go.mod index fe96d45..e278f2f 100644 --- a/go.mod +++ b/go.mod @@ -1,3 +1,3 @@ module github.com/google/blueprint -go 1.13 +go 1.18