From 0335e0900d8ad9f44b2445d58f35ba59bb915040 Mon Sep 17 00:00:00 2001 From: Colin Cross Date: Thu, 21 Jan 2021 15:26:21 -0800 Subject: [PATCH 1/6] Use io.StringWriter in ninjaWriter ninjaWriter repeatedly called io.WriteString() on its writer, which does a type assertion every time. Replace its io.Writer with an io.StringWriter and call WriteString on it directly. Test: ninja_writer_test.go Change-Id: Ie073d996a319190242bf6a00af07a13a60d078b5 --- bootstrap/command.go | 4 +- context.go | 2 +- ninja_writer.go | 101 ++++++++++++++++++++++++++++++++++--------- 3 files changed, 83 insertions(+), 24 deletions(-) diff --git a/bootstrap/command.go b/bootstrap/command.go index c85644e..7f4f077 100644 --- a/bootstrap/command.go +++ b/bootstrap/command.go @@ -186,7 +186,7 @@ func Main(ctx *blueprint.Context, config interface{}, extraNinjaFileDeps ...stri } const outFilePermissions = 0666 - var out io.Writer + var out io.StringWriter var f *os.File var buf *bufio.Writer @@ -204,7 +204,7 @@ func Main(ctx *blueprint.Context, config interface{}, extraNinjaFileDeps ...stri buf = bufio.NewWriter(f) out = buf } else { - out = ioutil.Discard + out = ioutil.Discard.(io.StringWriter) } if globFile != "" { diff --git a/context.go b/context.go index 1a99bba..ef290f6 100644 --- a/context.go +++ b/context.go @@ -3489,7 +3489,7 @@ func (c *Context) SingletonName(singleton Singleton) string { // WriteBuildFile writes the Ninja manifeset text for the generated build // actions to w. If this is called before PrepareBuildActions successfully // completes then ErrBuildActionsNotReady is returned. -func (c *Context) WriteBuildFile(w io.Writer) error { +func (c *Context) WriteBuildFile(w io.StringWriter) error { var err error pprof.Do(c.Context, pprof.Labels("blueprint", "WriteBuildFile"), func(ctx context.Context) { if !c.buildActionsReady { diff --git a/ninja_writer.go b/ninja_writer.go index 5e69c08..6a4d73b 100644 --- a/ninja_writer.go +++ b/ninja_writer.go @@ -15,7 +15,6 @@ package blueprint import ( - "fmt" "io" "strings" "unicode" @@ -29,13 +28,18 @@ const ( var indentString = strings.Repeat(" ", indentWidth*maxIndentDepth) +type StringWriterWriter interface { + io.StringWriter + io.Writer +} + type ninjaWriter struct { - writer io.Writer + writer io.StringWriter justDidBlankLine bool // true if the last operation was a BlankLine } -func newNinjaWriter(writer io.Writer) *ninjaWriter { +func newNinjaWriter(writer io.StringWriter) *ninjaWriter { return &ninjaWriter{ writer: writer, } @@ -72,7 +76,7 @@ func (n *ninjaWriter) Comment(comment string) error { if writeLine { line = strings.TrimSpace("# "+line) + "\n" - _, err := io.WriteString(n.writer, line) + _, err := n.writer.WriteString(line) if err != nil { return err } @@ -82,7 +86,15 @@ func (n *ninjaWriter) Comment(comment string) error { if lineStart != len(comment) { line := strings.TrimSpace(comment[lineStart:]) - _, err := fmt.Fprintf(n.writer, "# %s\n", line) + _, err := n.writer.WriteString("# ") + if err != nil { + return err + } + _, err = n.writer.WriteString(line) + if err != nil { + return err + } + _, err = n.writer.WriteString("\n") if err != nil { return err } @@ -93,14 +105,12 @@ func (n *ninjaWriter) Comment(comment string) error { func (n *ninjaWriter) Pool(name string) error { n.justDidBlankLine = false - _, err := fmt.Fprintf(n.writer, "pool %s\n", name) - return err + return n.writeStatement("pool", name) } func (n *ninjaWriter) Rule(name string) error { n.justDidBlankLine = false - _, err := fmt.Fprintf(n.writer, "rule %s\n", name) - return err + return n.writeStatement("rule", name) } func (n *ninjaWriter) Build(comment string, rule string, outputs, implicitOuts, @@ -174,14 +184,48 @@ func (n *ninjaWriter) Build(comment string, rule string, outputs, implicitOuts, func (n *ninjaWriter) Assign(name, value string) error { n.justDidBlankLine = false - _, err := fmt.Fprintf(n.writer, "%s = %s\n", name, value) - return err + _, err := n.writer.WriteString(name) + if err != nil { + return err + } + _, err = n.writer.WriteString(" = ") + if err != nil { + return err + } + _, err = n.writer.WriteString(value) + if err != nil { + return err + } + _, err = n.writer.WriteString("\n") + if err != nil { + return err + } + return nil } func (n *ninjaWriter) ScopedAssign(name, value string) error { n.justDidBlankLine = false - _, err := fmt.Fprintf(n.writer, "%s%s = %s\n", indentString[:indentWidth], name, value) - return err + _, err := n.writer.WriteString(indentString[:indentWidth]) + if err != nil { + return err + } + _, err = n.writer.WriteString(name) + if err != nil { + return err + } + _, err = n.writer.WriteString(" = ") + if err != nil { + return err + } + _, err = n.writer.WriteString(value) + if err != nil { + return err + } + _, err = n.writer.WriteString("\n") + if err != nil { + return err + } + return nil } func (n *ninjaWriter) Default(targets ...string) error { @@ -206,19 +250,34 @@ func (n *ninjaWriter) Default(targets ...string) error { func (n *ninjaWriter) Subninja(file string) error { n.justDidBlankLine = false - _, err := fmt.Fprintf(n.writer, "subninja %s\n", file) - return err + return n.writeStatement("subninja", file) } func (n *ninjaWriter) BlankLine() (err error) { // We don't output multiple blank lines in a row. if !n.justDidBlankLine { n.justDidBlankLine = true - _, err = io.WriteString(n.writer, "\n") + _, err = n.writer.WriteString("\n") } return err } +func (n *ninjaWriter) writeStatement(directive, name string) error { + _, err := n.writer.WriteString(directive + " ") + if err != nil { + return err + } + _, err = n.writer.WriteString(name) + if err != nil { + return err + } + _, err = n.writer.WriteString("\n") + if err != nil { + return err + } + return nil +} + type ninjaWriterWithWrap struct { *ninjaWriter maxLineLen int @@ -237,25 +296,25 @@ func (n *ninjaWriterWithWrap) writeString(s string, space bool) { } if n.writtenLen+len(s)+spaceLen > n.maxLineLen { - _, n.err = io.WriteString(n.writer, " $\n") + _, n.err = n.writer.WriteString(" $\n") if n.err != nil { return } - _, n.err = io.WriteString(n.writer, indentString[:indentWidth*2]) + _, n.err = n.writer.WriteString(indentString[:indentWidth*2]) if n.err != nil { return } n.writtenLen = indentWidth * 2 s = strings.TrimLeftFunc(s, unicode.IsSpace) } else if space { - _, n.err = io.WriteString(n.writer, " ") + _, n.err = n.writer.WriteString(" ") if n.err != nil { return } n.writtenLen++ } - _, n.err = io.WriteString(n.writer, s) + _, n.err = n.writer.WriteString(s) n.writtenLen += len(s) } @@ -271,6 +330,6 @@ func (n *ninjaWriterWithWrap) Flush() error { if n.err != nil { return n.err } - _, err := io.WriteString(n.writer, "\n") + _, err := n.writer.WriteString("\n") return err } From c8b9e552895468f2eb39b73c6f1ec918920c5d62 Mon Sep 17 00:00:00 2001 From: Colin Cross Date: Thu, 21 Jan 2021 15:28:04 -0800 Subject: [PATCH 2/6] Increase ninja file writer buffer Increase the ninja file writer buffer from the default 4k to 16MB. Test: m checkbuild Change-Id: Ieb2c82218517b98469ef93f1ea4dd04b5651f7d1 --- bootstrap/command.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bootstrap/command.go b/bootstrap/command.go index 7f4f077..01e449f 100644 --- a/bootstrap/command.go +++ b/bootstrap/command.go @@ -201,7 +201,7 @@ func Main(ctx *blueprint.Context, config interface{}, extraNinjaFileDeps ...stri if err != nil { fatalf("error opening Ninja file: %s", err) } - buf = bufio.NewWriter(f) + buf = bufio.NewWriterSize(f, 16*1024*1024) out = buf } else { out = ioutil.Discard.(io.StringWriter) From 00890dd8f62580eb216d81b2ea02e5d5d61dc203 Mon Sep 17 00:00:00 2001 From: Colin Cross Date: Thu, 21 Jan 2021 15:29:16 -0800 Subject: [PATCH 3/6] Optimize arg parsing in buildDef.WriteTo Arguments to build definitions were copied from the input map to an map with the name and value expanded, then to a list of names for sorting, and then written, which required iterating over a map three times. Expand the name and value into a list of name value pairs, and then do the rest of the operations on the list instead. Test: ninja_writer_test.go Change-Id: Id8ff644dafbaa3b4812747c60dc28cce22e21dbe --- ninja_defs.go | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/ninja_defs.go b/ninja_defs.go index 33fbc47..5912d61 100644 --- a/ninja_defs.go +++ b/ninja_defs.go @@ -410,25 +410,25 @@ func (b *buildDef) WriteTo(nw *ninjaWriter, pkgNames map[*packageContext]string) return err } - args := make(map[string]string) - - for argVar, value := range b.Args { - args[argVar.fullName(pkgNames)] = value.Value(pkgNames) - } - err = writeVariables(nw, b.Variables, pkgNames) if err != nil { return err } - var keys []string - for k := range args { - keys = append(keys, k) + type nameValuePair struct { + name, value string } - sort.Strings(keys) - for _, name := range keys { - err = nw.ScopedAssign(name, args[name]) + args := make([]nameValuePair, 0, len(b.Args)) + + for argVar, value := range b.Args { + fullName := argVar.fullName(pkgNames) + args = append(args, nameValuePair{fullName, value.Value(pkgNames)}) + } + sort.Slice(args, func(i, j int) bool { return args[i].name < args[j].name }) + + for _, pair := range args { + err = nw.ScopedAssign(pair.name, pair.value) if err != nil { return err } From 92054a49d29bf3fb31366f84e48c3950942a6c36 Mon Sep 17 00:00:00 2001 From: Colin Cross Date: Thu, 21 Jan 2021 16:49:25 -0800 Subject: [PATCH 4/6] Memoize full names of variables, pools and rules Variables, pools and rules each computed their full names every time they were referenced, which required string concatenations. Since every one is guaranteed to be accessed at least twice, once when the definition is written into the ninja file and once for each reference, precompute the full name. For local variables that can be done during initialization, but for global variables add a pass to PrepareBuildActions to compute the name for each live variable using the final package names. Test: ninja_writer_test.go Change-Id: I2264b05e0409e36651db2fb5d463c16c698d4d5e --- context.go | 17 ++++++++ package_ctx.go | 108 ++++++++++++++++++++++++++++++++++++++++++------- scope.go | 51 ++++++++++++++--------- 3 files changed, 142 insertions(+), 34 deletions(-) diff --git a/context.go b/context.go index ef290f6..63a82b4 100644 --- a/context.go +++ b/context.go @@ -2309,6 +2309,8 @@ func (c *Context) PrepareBuildActions(config interface{}) (deps []string, errs [ deps = append(deps, depsPackages...) + c.memoizeFullNames(c.liveGlobals, pkgNames) + // This will panic if it finds a problem since it's a programming error. c.checkForVariableReferenceCycles(c.liveGlobals.variables, pkgNames) @@ -3173,6 +3175,21 @@ func (c *Context) makeUniquePackageNames( return pkgNames, deps } +// memoizeFullNames stores the full name of each live global variable, rule and pool since each is +// guaranteed to be used at least twice, once in the definition and once for each usage, and many +// are used much more than once. +func (c *Context) memoizeFullNames(liveGlobals *liveTracker, pkgNames map[*packageContext]string) { + for v := range liveGlobals.variables { + v.memoizeFullName(pkgNames) + } + for r := range liveGlobals.rules { + r.memoizeFullName(pkgNames) + } + for p := range liveGlobals.pools { + p.memoizeFullName(pkgNames) + } +} + func (c *Context) checkForVariableReferenceCycles( variables map[Variable]ninjaString, pkgNames map[*packageContext]string) { diff --git a/package_ctx.go b/package_ctx.go index 088239e..af78772 100644 --- a/package_ctx.go +++ b/package_ctx.go @@ -250,9 +250,10 @@ func (p *packageContext) ImportAs(as, pkgPath string) { } type staticVariable struct { - pctx *packageContext - name_ string - value_ string + pctx *packageContext + name_ string + value_ string + fullName_ string } // StaticVariable returns a Variable whose value does not depend on any @@ -271,7 +272,11 @@ func (p *packageContext) StaticVariable(name, value string) Variable { panic(err) } - v := &staticVariable{p, name, value} + v := &staticVariable{ + pctx: p, + name_: name, + value_: value, + } err = p.scope.AddVariable(v) if err != nil { panic(err) @@ -289,9 +294,16 @@ func (v *staticVariable) name() string { } func (v *staticVariable) fullName(pkgNames map[*packageContext]string) string { + if v.fullName_ != "" { + return v.fullName_ + } return packageNamespacePrefix(pkgNames[v.pctx]) + v.name_ } +func (v *staticVariable) memoizeFullName(pkgNames map[*packageContext]string) { + v.fullName_ = v.fullName(pkgNames) +} + func (v *staticVariable) value(interface{}) (ninjaString, error) { ninjaStr, err := parseNinjaString(v.pctx.scope, v.value_) if err != nil { @@ -306,9 +318,10 @@ func (v *staticVariable) String() string { } type variableFunc struct { - pctx *packageContext - name_ string - value_ func(interface{}) (string, error) + pctx *packageContext + name_ string + value_ func(interface{}) (string, error) + fullName_ string } // VariableFunc returns a Variable whose value is determined by a function that @@ -332,7 +345,11 @@ func (p *packageContext) VariableFunc(name string, panic(err) } - v := &variableFunc{p, name, f} + v := &variableFunc{ + pctx: p, + name_: name, + value_: f, + } err = p.scope.AddVariable(v) if err != nil { panic(err) @@ -371,7 +388,11 @@ func (p *packageContext) VariableConfigMethod(name string, return resultStr, nil } - v := &variableFunc{p, name, fun} + v := &variableFunc{ + pctx: p, + name_: name, + value_: fun, + } err = p.scope.AddVariable(v) if err != nil { panic(err) @@ -389,9 +410,16 @@ func (v *variableFunc) name() string { } func (v *variableFunc) fullName(pkgNames map[*packageContext]string) string { + if v.fullName_ != "" { + return v.fullName_ + } return packageNamespacePrefix(pkgNames[v.pctx]) + v.name_ } +func (v *variableFunc) memoizeFullName(pkgNames map[*packageContext]string) { + v.fullName_ = v.fullName(pkgNames) +} + func (v *variableFunc) value(config interface{}) (ninjaString, error) { value, err := v.value_(config) if err != nil { @@ -452,6 +480,10 @@ func (v *argVariable) fullName(pkgNames map[*packageContext]string) string { return v.name_ } +func (v *argVariable) memoizeFullName(pkgNames map[*packageContext]string) { + // Nothing to do, full name is known at initialization. +} + func (v *argVariable) value(config interface{}) (ninjaString, error) { return nil, errVariableIsArg } @@ -461,9 +493,10 @@ func (v *argVariable) String() string { } type staticPool struct { - pctx *packageContext - name_ string - params PoolParams + pctx *packageContext + name_ string + params PoolParams + fullName_ string } // StaticPool returns a Pool whose value does not depend on any configuration @@ -483,7 +516,11 @@ func (p *packageContext) StaticPool(name string, params PoolParams) Pool { panic(err) } - pool := &staticPool{p, name, params} + pool := &staticPool{ + pctx: p, + name_: name, + params: params, + } err = p.scope.AddPool(pool) if err != nil { panic(err) @@ -501,9 +538,16 @@ func (p *staticPool) name() string { } func (p *staticPool) fullName(pkgNames map[*packageContext]string) string { + if p.fullName_ != "" { + return p.fullName_ + } return packageNamespacePrefix(pkgNames[p.pctx]) + p.name_ } +func (p *staticPool) memoizeFullName(pkgNames map[*packageContext]string) { + p.fullName_ = p.fullName(pkgNames) +} + func (p *staticPool) def(config interface{}) (*poolDef, error) { def, err := parsePoolParams(p.pctx.scope, &p.params) if err != nil { @@ -520,6 +564,7 @@ type poolFunc struct { pctx *packageContext name_ string paramsFunc func(interface{}) (PoolParams, error) + fullName_ string } // PoolFunc returns a Pool whose value is determined by a function that takes a @@ -542,7 +587,11 @@ func (p *packageContext) PoolFunc(name string, f func(interface{}) (PoolParams, panic(err) } - pool := &poolFunc{p, name, f} + pool := &poolFunc{ + pctx: p, + name_: name, + paramsFunc: f, + } err = p.scope.AddPool(pool) if err != nil { panic(err) @@ -560,9 +609,16 @@ func (p *poolFunc) name() string { } func (p *poolFunc) fullName(pkgNames map[*packageContext]string) string { + if p.fullName_ != "" { + return p.fullName_ + } return packageNamespacePrefix(pkgNames[p.pctx]) + p.name_ } +func (p *poolFunc) memoizeFullName(pkgNames map[*packageContext]string) { + p.fullName_ = p.fullName(pkgNames) +} + func (p *poolFunc) def(config interface{}) (*poolDef, error) { params, err := p.paramsFunc(config) if err != nil { @@ -595,6 +651,10 @@ func (p *builtinPool) fullName(pkgNames map[*packageContext]string) string { return p.name_ } +func (p *builtinPool) memoizeFullName(pkgNames map[*packageContext]string) { + // Nothing to do, full name is known at initialization. +} + func (p *builtinPool) def(config interface{}) (*poolDef, error) { return nil, errPoolIsBuiltin } @@ -616,6 +676,7 @@ type staticRule struct { params RuleParams argNames map[string]bool scope_ *basicScope + fullName_ string sync.Mutex // protects scope_ during lazy creation } @@ -683,9 +744,16 @@ func (r *staticRule) name() string { } func (r *staticRule) fullName(pkgNames map[*packageContext]string) string { + if r.fullName_ != "" { + return r.fullName_ + } return packageNamespacePrefix(pkgNames[r.pctx]) + r.name_ } +func (r *staticRule) memoizeFullName(pkgNames map[*packageContext]string) { + r.fullName_ = r.fullName(pkgNames) +} + func (r *staticRule) def(interface{}) (*ruleDef, error) { def, err := parseRuleParams(r.scope(), &r.params) if err != nil { @@ -721,6 +789,7 @@ type ruleFunc struct { paramsFunc func(interface{}) (RuleParams, error) argNames map[string]bool scope_ *basicScope + fullName_ string sync.Mutex // protects scope_ during lazy creation } @@ -789,9 +858,16 @@ func (r *ruleFunc) name() string { } func (r *ruleFunc) fullName(pkgNames map[*packageContext]string) string { + if r.fullName_ != "" { + return r.fullName_ + } return packageNamespacePrefix(pkgNames[r.pctx]) + r.name_ } +func (r *ruleFunc) memoizeFullName(pkgNames map[*packageContext]string) { + r.fullName_ = r.fullName(pkgNames) +} + func (r *ruleFunc) def(config interface{}) (*ruleDef, error) { params, err := r.paramsFunc(config) if err != nil { @@ -843,6 +919,10 @@ func (r *builtinRule) fullName(pkgNames map[*packageContext]string) string { return r.name_ } +func (r *builtinRule) memoizeFullName(pkgNames map[*packageContext]string) { + // Nothing to do, full name is known at initialization. +} + func (r *builtinRule) def(config interface{}) (*ruleDef, error) { return nil, errRuleIsBuiltin } diff --git a/scope.go b/scope.go index 0a520d9..3f39eb7 100644 --- a/scope.go +++ b/scope.go @@ -28,6 +28,7 @@ type Variable interface { packageContext() *packageContext name() string // "foo" fullName(pkgNames map[*packageContext]string) string // "pkg.foo" or "path.to.pkg.foo" + memoizeFullName(pkgNames map[*packageContext]string) // precompute fullName if desired value(config interface{}) (ninjaString, error) String() string } @@ -38,6 +39,7 @@ type Pool interface { packageContext() *packageContext name() string // "foo" fullName(pkgNames map[*packageContext]string) string // "pkg.foo" or "path.to.pkg.foo" + memoizeFullName(pkgNames map[*packageContext]string) // precompute fullName if desired def(config interface{}) (*poolDef, error) String() string } @@ -48,6 +50,7 @@ type Rule interface { packageContext() *packageContext name() string // "foo" fullName(pkgNames map[*packageContext]string) string // "pkg.foo" or "path.to.pkg.foo" + memoizeFullName(pkgNames map[*packageContext]string) // precompute fullName if desired def(config interface{}) (*ruleDef, error) scope() *basicScope isArg(argName string) bool @@ -294,9 +297,9 @@ func (s *localScope) AddLocalVariable(name, value string) (*localVariable, } v := &localVariable{ - namePrefix: s.namePrefix, - name_: name, - value_: ninjaValue, + fullName_: s.namePrefix + name, + name_: name, + value_: ninjaValue, } err = s.scope.AddVariable(v) @@ -333,11 +336,11 @@ func (s *localScope) AddLocalRule(name string, params *RuleParams, } r := &localRule{ - namePrefix: s.namePrefix, - name_: name, - def_: def, - argNames: argNamesSet, - scope_: ruleScope, + fullName_: s.namePrefix + name, + name_: name, + def_: def, + argNames: argNamesSet, + scope_: ruleScope, } err = s.scope.AddRule(r) @@ -349,9 +352,9 @@ func (s *localScope) AddLocalRule(name string, params *RuleParams, } type localVariable struct { - namePrefix string - name_ string - value_ ninjaString + fullName_ string + name_ string + value_ ninjaString } func (l *localVariable) packageContext() *packageContext { @@ -363,7 +366,11 @@ func (l *localVariable) name() string { } func (l *localVariable) fullName(pkgNames map[*packageContext]string) string { - return l.namePrefix + l.name_ + return l.fullName_ +} + +func (l *localVariable) memoizeFullName(pkgNames map[*packageContext]string) { + // Nothing to do, full name is known at initialization. } func (l *localVariable) value(interface{}) (ninjaString, error) { @@ -371,15 +378,15 @@ func (l *localVariable) value(interface{}) (ninjaString, error) { } func (l *localVariable) String() string { - return ":" + l.namePrefix + l.name_ + return ":" + l.fullName_ } type localRule struct { - namePrefix string - name_ string - def_ *ruleDef - argNames map[string]bool - scope_ *basicScope + fullName_ string + name_ string + def_ *ruleDef + argNames map[string]bool + scope_ *basicScope } func (l *localRule) packageContext() *packageContext { @@ -391,7 +398,11 @@ func (l *localRule) name() string { } func (l *localRule) fullName(pkgNames map[*packageContext]string) string { - return l.namePrefix + l.name_ + return l.fullName_ +} + +func (l *localRule) memoizeFullName(pkgNames map[*packageContext]string) { + // Nothing to do, full name is known at initialization. } func (l *localRule) def(interface{}) (*ruleDef, error) { @@ -407,5 +418,5 @@ func (r *localRule) isArg(argName string) bool { } func (r *localRule) String() string { - return ":" + r.namePrefix + r.name_ + return ":" + r.fullName_ } From 8a40148408c1b91511a649fe135962bafefebc49 Mon Sep 17 00:00:00 2001 From: Colin Cross Date: Thu, 21 Jan 2021 18:27:14 -0800 Subject: [PATCH 5/6] Write build definitions directly to output writer buildDef.WriteTo was calling valueList to convert all the build parameter ninjaStrings into strings, which uses ValueWithEscaper to build a strings.Builder. This results in building a string only to immediately copy it into the output writer's buffer. Instead, pass an io.StringWriter to ValueWithEscaper so it can build the string directly into the output writer's buffer. This requires converting ninjaWriterWithWrap into an io.StringWriter. Test: ninja_writer_test.go Change-Id: I02e1cf8259306267b9d2d0ebe8c81e13dd443725 --- ninja_defs.go | 30 +++------ ninja_strings.go | 39 ++++++------ ninja_writer.go | 141 +++++++++++++++++++++++++++++++++---------- ninja_writer_test.go | 27 +++++++-- 4 files changed, 162 insertions(+), 75 deletions(-) diff --git a/ninja_defs.go b/ninja_defs.go index 5912d61..69233c2 100644 --- a/ninja_defs.go +++ b/ninja_defs.go @@ -392,20 +392,20 @@ func (b *buildDef) WriteTo(nw *ninjaWriter, pkgNames map[*packageContext]string) var ( comment = b.Comment rule = b.Rule.fullName(pkgNames) - outputs = valueList(b.Outputs, pkgNames, outputEscaper) - implicitOuts = valueList(b.ImplicitOutputs, pkgNames, outputEscaper) - explicitDeps = valueList(b.Inputs, pkgNames, inputEscaper) - implicitDeps = valueList(b.Implicits, pkgNames, inputEscaper) - orderOnlyDeps = valueList(b.OrderOnly, pkgNames, inputEscaper) - validations = valueList(b.Validations, pkgNames, inputEscaper) + outputs = b.Outputs + implicitOuts = b.ImplicitOutputs + explicitDeps = b.Inputs + implicitDeps = b.Implicits + orderOnlyDeps = b.OrderOnly + validations = b.Validations ) if b.RuleDef != nil { - implicitDeps = append(valueList(b.RuleDef.CommandDeps, pkgNames, inputEscaper), implicitDeps...) - orderOnlyDeps = append(valueList(b.RuleDef.CommandOrderOnly, pkgNames, inputEscaper), orderOnlyDeps...) + implicitDeps = append(b.RuleDef.CommandDeps, implicitDeps...) + orderOnlyDeps = append(b.RuleDef.CommandOrderOnly, orderOnlyDeps...) } - err := nw.Build(comment, rule, outputs, implicitOuts, explicitDeps, implicitDeps, orderOnlyDeps, validations) + err := nw.Build(comment, rule, outputs, implicitOuts, explicitDeps, implicitDeps, orderOnlyDeps, validations, pkgNames) if err != nil { return err } @@ -435,7 +435,7 @@ func (b *buildDef) WriteTo(nw *ninjaWriter, pkgNames map[*packageContext]string) } if !b.Optional { - err = nw.Default(outputs...) + err = nw.Default(pkgNames, outputs...) if err != nil { return err } @@ -444,16 +444,6 @@ func (b *buildDef) WriteTo(nw *ninjaWriter, pkgNames map[*packageContext]string) return nw.BlankLine() } -func valueList(list []ninjaString, pkgNames map[*packageContext]string, - escaper *strings.Replacer) []string { - - result := make([]string, len(list)) - for i, ninjaStr := range list { - result[i] = ninjaStr.ValueWithEscaper(pkgNames, escaper) - } - return result -} - func writeVariables(nw *ninjaWriter, variables map[string]ninjaString, pkgNames map[*packageContext]string) error { var keys []string diff --git a/ninja_strings.go b/ninja_strings.go index 190cae9..51a167d 100644 --- a/ninja_strings.go +++ b/ninja_strings.go @@ -17,6 +17,7 @@ package blueprint import ( "bytes" "fmt" + "io" "strings" ) @@ -36,7 +37,7 @@ var ( type ninjaString interface { Value(pkgNames map[*packageContext]string) string - ValueWithEscaper(pkgNames map[*packageContext]string, escaper *strings.Replacer) string + ValueWithEscaper(w io.StringWriter, pkgNames map[*packageContext]string, escaper *strings.Replacer) Eval(variables map[Variable]ninjaString) (string, error) Variables() []Variable } @@ -284,26 +285,24 @@ func parseNinjaStrings(scope scope, strs []string) ([]ninjaString, } func (n varNinjaString) Value(pkgNames map[*packageContext]string) string { - return n.ValueWithEscaper(pkgNames, defaultEscaper) + if len(n.strings) == 1 { + return defaultEscaper.Replace(n.strings[0]) + } + str := &strings.Builder{} + n.ValueWithEscaper(str, pkgNames, defaultEscaper) + return str.String() } -func (n varNinjaString) ValueWithEscaper(pkgNames map[*packageContext]string, - escaper *strings.Replacer) string { +func (n varNinjaString) ValueWithEscaper(w io.StringWriter, pkgNames map[*packageContext]string, + escaper *strings.Replacer) { - if len(n.strings) == 1 { - return escaper.Replace(n.strings[0]) - } - - str := strings.Builder{} - str.WriteString(escaper.Replace(n.strings[0])) + w.WriteString(escaper.Replace(n.strings[0])) for i, v := range n.variables { - str.WriteString("${") - str.WriteString(v.fullName(pkgNames)) - str.WriteString("}") - str.WriteString(escaper.Replace(n.strings[i+1])) + w.WriteString("${") + w.WriteString(v.fullName(pkgNames)) + w.WriteString("}") + w.WriteString(escaper.Replace(n.strings[i+1])) } - - return str.String() } func (n varNinjaString) Eval(variables map[Variable]ninjaString) (string, error) { @@ -327,12 +326,12 @@ func (n varNinjaString) Variables() []Variable { } func (l literalNinjaString) Value(pkgNames map[*packageContext]string) string { - return l.ValueWithEscaper(pkgNames, defaultEscaper) + return defaultEscaper.Replace(string(l)) } -func (l literalNinjaString) ValueWithEscaper(pkgNames map[*packageContext]string, - escaper *strings.Replacer) string { - return escaper.Replace(string(l)) +func (l literalNinjaString) ValueWithEscaper(w io.StringWriter, pkgNames map[*packageContext]string, + escaper *strings.Replacer) { + w.WriteString(escaper.Replace(string(l))) } func (l literalNinjaString) Eval(variables map[Variable]ninjaString) (string, error) { diff --git a/ninja_writer.go b/ninja_writer.go index 6a4d73b..f9951b4 100644 --- a/ninja_writer.go +++ b/ninja_writer.go @@ -114,14 +114,15 @@ func (n *ninjaWriter) Rule(name string) error { } func (n *ninjaWriter) Build(comment string, rule string, outputs, implicitOuts, - explicitDeps, implicitDeps, orderOnlyDeps, validations []string) error { + explicitDeps, implicitDeps, orderOnlyDeps, validations []ninjaString, + pkgNames map[*packageContext]string) error { n.justDidBlankLine = false const lineWrapLen = len(" $") const maxLineLen = lineWidth - lineWrapLen - wrapper := ninjaWriterWithWrap{ + wrapper := &ninjaWriterWithWrap{ ninjaWriter: n, maxLineLen: maxLineLen, } @@ -136,14 +137,16 @@ func (n *ninjaWriter) Build(comment string, rule string, outputs, implicitOuts, wrapper.WriteString("build") for _, output := range outputs { - wrapper.WriteStringWithSpace(output) + wrapper.Space() + output.ValueWithEscaper(wrapper, pkgNames, outputEscaper) } if len(implicitOuts) > 0 { wrapper.WriteStringWithSpace("|") for _, out := range implicitOuts { - wrapper.WriteStringWithSpace(out) + wrapper.Space() + out.ValueWithEscaper(wrapper, pkgNames, outputEscaper) } } @@ -152,14 +155,16 @@ func (n *ninjaWriter) Build(comment string, rule string, outputs, implicitOuts, wrapper.WriteStringWithSpace(rule) for _, dep := range explicitDeps { - wrapper.WriteStringWithSpace(dep) + wrapper.Space() + dep.ValueWithEscaper(wrapper, pkgNames, inputEscaper) } if len(implicitDeps) > 0 { wrapper.WriteStringWithSpace("|") for _, dep := range implicitDeps { - wrapper.WriteStringWithSpace(dep) + wrapper.Space() + dep.ValueWithEscaper(wrapper, pkgNames, inputEscaper) } } @@ -167,7 +172,8 @@ func (n *ninjaWriter) Build(comment string, rule string, outputs, implicitOuts, wrapper.WriteStringWithSpace("||") for _, dep := range orderOnlyDeps { - wrapper.WriteStringWithSpace(dep) + wrapper.Space() + dep.ValueWithEscaper(wrapper, pkgNames, inputEscaper) } } @@ -175,7 +181,8 @@ func (n *ninjaWriter) Build(comment string, rule string, outputs, implicitOuts, wrapper.WriteStringWithSpace("|@") for _, dep := range validations { - wrapper.WriteStringWithSpace(dep) + wrapper.Space() + dep.ValueWithEscaper(wrapper, pkgNames, inputEscaper) } } @@ -228,13 +235,13 @@ func (n *ninjaWriter) ScopedAssign(name, value string) error { return nil } -func (n *ninjaWriter) Default(targets ...string) error { +func (n *ninjaWriter) Default(pkgNames map[*packageContext]string, targets ...ninjaString) error { n.justDidBlankLine = false const lineWrapLen = len(" $") const maxLineLen = lineWidth - lineWrapLen - wrapper := ninjaWriterWithWrap{ + wrapper := &ninjaWriterWithWrap{ ninjaWriter: n, maxLineLen: maxLineLen, } @@ -242,7 +249,8 @@ func (n *ninjaWriter) Default(targets ...string) error { wrapper.WriteString("default") for _, target := range targets { - wrapper.WriteString(" " + target) + wrapper.Space() + target.ValueWithEscaper(wrapper, pkgNames, outputEscaper) } return wrapper.Flush() @@ -278,24 +286,56 @@ func (n *ninjaWriter) writeStatement(directive, name string) error { return nil } +// ninjaWriterWithWrap is an io.StringWriter that writes through to a ninjaWriter, but supports +// user-readable line wrapping on boundaries when ninjaWriterWithWrap.Space is called. +// It collects incoming calls to WriteString until either the line length is exceeded, in which case +// it inserts a wrap before the pending strings and then writes them, or the next call to Space, in +// which case it writes out the pending strings. +// +// WriteString never returns an error, all errors are held until Flush is called. Once an error has +// occurred all writes become noops. type ninjaWriterWithWrap struct { *ninjaWriter + // pending lists the strings that have been written since the last call to Space. + pending []string + + // pendingLen accumulates the lengths of the strings in pending. + pendingLen int + + // lineLen accumulates the number of bytes on the current line. + lineLen int + + // maxLineLen is the length of the line before wrapping. maxLineLen int - writtenLen int - err error + + // space is true if the strings in pending should be preceded by a space. + space bool + + // err holds any error that has occurred to return in Flush. + err error } -func (n *ninjaWriterWithWrap) writeString(s string, space bool) { +// WriteString writes the string to buffer, wrapping on a previous Space call if necessary. +// It never returns an error, all errors are held until Flush is called. +func (n *ninjaWriterWithWrap) WriteString(s string) (written int, noError error) { + // Always return the full length of the string and a nil error. + // ninjaWriterWithWrap doesn't return errors to the caller, it saves them until Flush() + written = len(s) + if n.err != nil { return } - spaceLen := 0 - if space { - spaceLen = 1 - } - - if n.writtenLen+len(s)+spaceLen > n.maxLineLen { + const spaceLen = 1 + if !n.space { + // No space is pending, so a line wrap can't be inserted before this, so just write + // the string. + n.lineLen += len(s) + _, n.err = n.writer.WriteString(s) + } else if n.lineLen+len(s)+spaceLen > n.maxLineLen { + // A space is pending, and the pending strings plus the current string would exceed the + // maximum line length. Wrap and indent before the pending space and strings, then write + // the pending and current strings. _, n.err = n.writer.WriteString(" $\n") if n.err != nil { return @@ -304,29 +344,68 @@ func (n *ninjaWriterWithWrap) writeString(s string, space bool) { if n.err != nil { return } - n.writtenLen = indentWidth * 2 + n.lineLen = indentWidth*2 + n.pendingLen s = strings.TrimLeftFunc(s, unicode.IsSpace) - } else if space { + n.pending = append(n.pending, s) + n.writePending() + + n.space = false + } else { + // A space is pending but the current string would not reach the maximum line length, + // add it to the pending list. + n.pending = append(n.pending, s) + n.pendingLen += len(s) + n.lineLen += len(s) + } + + return +} + +// Space inserts a space that is also a possible wrapping point into the string. +func (n *ninjaWriterWithWrap) Space() { + if n.err != nil { + return + } + if n.space { + // A space was already pending, and the space plus any strings written after the space did + // not reach the maxmimum line length, so write out the old space and pending strings. _, n.err = n.writer.WriteString(" ") + n.lineLen++ + n.writePending() + } + n.space = true +} + +// writePending writes out all the strings stored in pending and resets it. +func (n *ninjaWriterWithWrap) writePending() { + if n.err != nil { + return + } + for _, pending := range n.pending { + _, n.err = n.writer.WriteString(pending) if n.err != nil { return } - n.writtenLen++ } - - _, n.err = n.writer.WriteString(s) - n.writtenLen += len(s) -} - -func (n *ninjaWriterWithWrap) WriteString(s string) { - n.writeString(s, false) + // Reset the length of pending back to 0 without reducing its capacity to avoid reallocating + // the backing array. + n.pending = n.pending[:0] + n.pendingLen = 0 } +// WriteStringWithSpace is a helper that calls Space and WriteString. func (n *ninjaWriterWithWrap) WriteStringWithSpace(s string) { - n.writeString(s, true) + n.Space() + _, _ = n.WriteString(s) } +// Flush writes out any pending space or strings and then a newline. It also returns any errors +// that have previously occurred. func (n *ninjaWriterWithWrap) Flush() error { + if n.space { + _, n.err = n.writer.WriteString(" ") + } + n.writePending() if n.err != nil { return n.err } diff --git a/ninja_writer_test.go b/ninja_writer_test.go index 48ecb7c..82eeee5 100644 --- a/ninja_writer_test.go +++ b/ninja_writer_test.go @@ -16,6 +16,7 @@ package blueprint import ( "bytes" + "strings" "testing" ) @@ -49,14 +50,26 @@ var ninjaWriterTestCases = []struct { }, { input: func(w *ninjaWriter) { - ck(w.Build("foo comment", "foo", []string{"o1", "o2"}, []string{"io1", "io2"}, - []string{"e1", "e2"}, []string{"i1", "i2"}, []string{"oo1", "oo2"}, []string{"v1", "v2"})) + ck(w.Build("foo comment", "foo", testNinjaStrings("o1", "o2"), + testNinjaStrings("io1", "io2"), testNinjaStrings("e1", "e2"), + testNinjaStrings("i1", "i2"), testNinjaStrings("oo1", "oo2"), + testNinjaStrings("v1", "v2"), nil)) }, output: "# foo comment\nbuild o1 o2 | io1 io2: foo e1 e2 | i1 i2 || oo1 oo2 |@ v1 v2\n", }, { input: func(w *ninjaWriter) { - ck(w.Default("foo")) + ck(w.Build("foo comment", "foo", + testNinjaStrings(strings.Repeat("o", lineWidth)), + nil, + testNinjaStrings(strings.Repeat("i", lineWidth)), + nil, nil, nil, nil)) + }, + output: "# foo comment\nbuild $\n " + strings.Repeat("o", lineWidth) + ": foo $\n " + strings.Repeat("i", lineWidth) + "\n", + }, + { + input: func(w *ninjaWriter) { + ck(w.Default(nil, testNinjaStrings("foo")...)) }, output: "default foo\n", }, @@ -94,7 +107,8 @@ var ninjaWriterTestCases = []struct { ck(w.ScopedAssign("command", "echo out: $out in: $in _arg: $_arg")) ck(w.ScopedAssign("pool", "p")) ck(w.BlankLine()) - ck(w.Build("r comment", "r", []string{"foo.o"}, nil, []string{"foo.in"}, nil, nil, nil)) + ck(w.Build("r comment", "r", testNinjaStrings("foo.o"), + nil, testNinjaStrings("foo.in"), nil, nil, nil, nil)) ck(w.ScopedAssign("_arg", "arg value")) }, output: `pool p @@ -124,3 +138,8 @@ func TestNinjaWriter(t *testing.T) { } } } + +func testNinjaStrings(s ...string) []ninjaString { + ret, _ := parseNinjaStrings(nil, s) + return ret +} From 7ff2e8d2a42d77cd23e1a9af17662345e914e7ee Mon Sep 17 00:00:00 2001 From: Colin Cross Date: Thu, 21 Jan 2021 22:39:28 -0800 Subject: [PATCH 6/6] Optimize updateDependencies Avoid reallocating module.forwardDeps and module.reverseDeps every time through updateDependencies by resetting the slices without reducing their capacity. Accumulate dependencies to visit directly into module.forwardDeps. Use a loop instead of a map to check for duplicates, average number of dependencies is measured to be 9.5, although there are a few outliers with up to 2108. Reduces mean soong_build execution time on internal master from 87s to 82.7s (5%). Test: context_test.go Change-Id: I58fcd5514e494bafa965443461851b21b7bce382 --- context.go | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/context.go b/context.go index 1a99bba..e240f06 100644 --- a/context.go +++ b/context.go @@ -1355,6 +1355,8 @@ func (c *Context) createVariations(origModule *moduleInfo, mutatorName string, m := *origModule newModule := &m newModule.directDeps = append([]depInfo(nil), origModule.directDeps...) + newModule.reverseDeps = nil + newModule.forwardDeps = nil newModule.logicModule = newLogicModule newModule.variant = newVariant(origModule, mutatorName, variationName, local) newModule.properties = newProperties @@ -2180,7 +2182,9 @@ func (c *Context) updateDependencies() (errs []error) { checking[module] = true defer delete(checking, module) - deps := make(map[*moduleInfo]bool) + // Reset the forward and reverse deps without reducing their capacity to avoid reallocation. + module.reverseDeps = module.reverseDeps[:0] + module.forwardDeps = module.forwardDeps[:0] // Add an implicit dependency ordering on all earlier modules in the same module group for _, dep := range module.group.modules { @@ -2188,18 +2192,22 @@ func (c *Context) updateDependencies() (errs []error) { break } if depModule := dep.module(); depModule != nil { - deps[depModule] = true + module.forwardDeps = append(module.forwardDeps, depModule) } } + outer: for _, dep := range module.directDeps { - deps[dep.module] = true + // use a loop to check for duplicates, average number of directDeps measured to be 9.5. + for _, exists := range module.forwardDeps { + if dep.module == exists { + continue outer + } + } + module.forwardDeps = append(module.forwardDeps, dep.module) } - module.reverseDeps = []*moduleInfo{} - module.forwardDeps = []*moduleInfo{} - - for dep := range deps { + for _, dep := range module.forwardDeps { if checking[dep] { // This is a cycle. return []*moduleInfo{dep, module} @@ -2225,7 +2233,6 @@ func (c *Context) updateDependencies() (errs []error) { } } - module.forwardDeps = append(module.forwardDeps, dep) dep.reverseDeps = append(dep.reverseDeps, module) }