From 95bec3331cc42db65c048a475b28022c7a303ec9 Mon Sep 17 00:00:00 2001 From: Colin Cross Date: Tue, 31 Oct 2023 15:15:30 -0700 Subject: [PATCH] Use strings instead of simpleNinjaStrings where possible Storing every string without ninja variable references through simpleNinjaString costs 24 bytes and a heap allocation. 16 bytes is used for the ninjaString.str string, 8 bytes for the ninjaString.variables *[]variableReference. An additional 8 bytes is used for the resulting pointer into the heap. The vast majority of calls to simpleNinjaString originate in blueprint.parseBuildParams, which converts all of the parameters passed to ctx.Build into ninjaStrings. All together this was allocating 1.575 GB of *ninjaString objects. Add a parseNinjaOrSimpleStrings function that converts input strings into ninjaStrings if they have ninja variable references, but also returns a slice of plain strings for input strings without any ninja variable references. That still results in 1.39 GB of allocations just for the output string slice, so also add an optimization that reuses the input string slice as the output slice if all of the strings had no variable references. Plumb the resulting strings through everywhere that the []*ninjaStrings were used. This reduces the total memory allocations inside blueprint.parseBuildParams in my AOSP aosp_cf_x86_64_phone-userdebug build from 3.337 GB to 1.786 GB. Test: ninja_strings_test.go Change-Id: I51bc138a2a6b1cc7383c7df0a483ccb067ffa02b --- bootstrap/command.go | 4 +- context.go | 44 ++++++++++++------- context_test.go | 43 ++++++++---------- go.mod | 2 +- ninja_defs.go | 71 ++++++++++++++++++------------ ninja_strings.go | 73 +++++++++++++++++++++++++++--- ninja_strings_test.go | 100 ++++++++++++++++++++++++++++++++++++++++++ ninja_writer.go | 50 ++++++++++++++++++--- ninja_writer_test.go | 24 +++++----- 9 files changed, 317 insertions(+), 94 deletions(-) diff --git a/bootstrap/command.go b/bootstrap/command.go index 08b6a84..bc1d32d 100644 --- a/bootstrap/command.go +++ b/bootstrap/command.go @@ -135,7 +135,7 @@ func RunBlueprint(args Args, stopBefore StopBefore, ctx *blueprint.Context, conf } const outFilePermissions = 0666 - var out io.StringWriter + var out blueprint.StringWriterWriter var f *os.File var buf *bufio.Writer @@ -145,7 +145,7 @@ func RunBlueprint(args Args, stopBefore StopBefore, ctx *blueprint.Context, conf if err := os.WriteFile(joinPath(ctx.SrcDir(), args.OutFile), []byte(nil), outFilePermissions); err != nil { return nil, fmt.Errorf("error writing empty Ninja file: %s", err) } - out = io.Discard.(io.StringWriter) + out = io.Discard.(blueprint.StringWriterWriter) } else { f, err := os.OpenFile(joinPath(ctx.SrcDir(), args.OutFile), os.O_WRONLY|os.O_CREATE|os.O_TRUNC, outFilePermissions) if err != nil { diff --git a/context.go b/context.go index 75ea2d6..1c08a3b 100644 --- a/context.go +++ b/context.go @@ -2786,11 +2786,16 @@ func jsonModuleWithActionsFromModuleInfo(m *moduleInfo) *JsonModule { var actions []JSONAction for _, bDef := range m.actionDefs.buildDefs { a := JSONAction{ - Inputs: append( - getNinjaStringsWithNilPkgNames(bDef.Inputs), + Inputs: append(append(append( + bDef.InputStrings, + bDef.ImplicitStrings...), + getNinjaStringsWithNilPkgNames(bDef.Inputs)...), getNinjaStringsWithNilPkgNames(bDef.Implicits)...), - Outputs: append( - getNinjaStringsWithNilPkgNames(bDef.Outputs), + + Outputs: append(append(append( + bDef.OutputStrings, + bDef.ImplicitOutputStrings...), + getNinjaStringsWithNilPkgNames(bDef.Outputs)...), getNinjaStringsWithNilPkgNames(bDef.ImplicitOutputs)...), } if d, ok := bDef.Variables["description"]; ok { @@ -3992,6 +3997,9 @@ func (c *Context) AllTargets() (map[string]string, error) { } targets[outputValue] = ruleName } + for _, output := range append(buildDef.OutputStrings, buildDef.ImplicitOutputStrings...) { + targets[output] = ruleName + } } return nil } @@ -4214,7 +4222,7 @@ func (c *Context) SingletonName(singleton Singleton) string { // WriteBuildFile writes the Ninja manifest 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.StringWriter) error { +func (c *Context) WriteBuildFile(w StringWriterWriter) error { var err error pprof.Do(c.Context, pprof.Labels("blueprint", "WriteBuildFile"), func(ctx context.Context) { if !c.buildActionsReady { @@ -4696,7 +4704,7 @@ type phonyCandidate struct { // keyForPhonyCandidate gives a unique identifier for a set of deps. // 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 { +func keyForPhonyCandidate(deps []*ninjaString, stringDeps []string) string { hasher := sha256.New() for _, d := range deps { if len(d.Variables()) != 0 { @@ -4704,6 +4712,9 @@ func keyForPhonyCandidate(deps []*ninjaString) string { } io.WriteString(hasher, d.Value(nil)) } + for _, d := range stringDeps { + io.WriteString(hasher, d) + } return base64.RawURLEncoding.EncodeToString(hasher.Sum(nil)) } @@ -4713,7 +4724,7 @@ func keyForPhonyCandidate(deps []*ninjaString) string { // (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) + key := keyForPhonyCandidate(b.OrderOnly, b.OrderOnlyStrings) if key == "" { return } @@ -4726,17 +4737,20 @@ func scanBuildDef(wg *sync.WaitGroup, candidates *sync.Map, phonyCount *atomic.U // extract it as a phony output phonyCount.Add(1) m.phony = &buildDef{ - Rule: Phony, - Outputs: []*ninjaString{simpleNinjaString("dedup-" + key)}, - Inputs: m.first.OrderOnly, //we could also use b.OrderOnly - Optional: true, + Rule: Phony, + OutputStrings: []string{"dedup-" + key}, + Inputs: m.first.OrderOnly, //we could also use b.OrderOnly + InputStrings: m.first.OrderOnlyStrings, + 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.OrderOnlyStrings = m.phony.OutputStrings + m.first.OrderOnly = nil m.first = nil }) - b.OrderOnly = m.phony.Outputs + b.OrderOnlyStrings = m.phony.OutputStrings + b.OrderOnly = nil } } @@ -4753,7 +4767,7 @@ func (c *Context) deduplicateOrderOnlyDeps(infos []*moduleInfo) *localBuildActio wg := sync.WaitGroup{} for _, info := range infos { for _, b := range info.actionDefs.buildDefs { - if len(b.OrderOnly) > 0 { + if len(b.OrderOnly) > 0 || len(b.OrderOnlyStrings) > 0 { wg.Add(1) go scanBuildDef(&wg, &candidates, &phonyCount, b) } @@ -4774,7 +4788,7 @@ func (c *Context) deduplicateOrderOnlyDeps(infos []*moduleInfo) *localBuildActio c.EventHandler.Do("sort_phony_builddefs", func() { // sorting for determinism, the phony output names are stable sort.Slice(phonys, func(i int, j int) bool { - return phonys[i].Outputs[0].Value(nil) < phonys[j].Outputs[0].Value(nil) + return phonys[i].OutputStrings[0] < phonys[j].OutputStrings[0] }) }) diff --git a/context_test.go b/context_test.go index a10f681..521d163 100644 --- a/context_test.go +++ b/context_test.go @@ -1159,18 +1159,11 @@ func TestPackageIncludes(t *testing.T) { } func TestDeduplicateOrderOnlyDeps(t *testing.T) { - outputs := func(names ...string) []*ninjaString { - r := make([]*ninjaString, len(names)) - for i, name := range names { - r[i] = simpleNinjaString(name) - } - return r - } b := func(output string, inputs []string, orderOnlyDeps []string) *buildDef { return &buildDef{ - Outputs: outputs(output), - Inputs: outputs(inputs...), - OrderOnly: outputs(orderOnlyDeps...), + OutputStrings: []string{output}, + InputStrings: inputs, + OrderOnlyStrings: orderOnlyDeps, } } m := func(bs ...*buildDef) *moduleInfo { @@ -1179,7 +1172,7 @@ func TestDeduplicateOrderOnlyDeps(t *testing.T) { type testcase struct { modules []*moduleInfo expectedPhonys []*buildDef - conversions map[string][]*ninjaString + conversions map[string][]string } testCases := []testcase{{ modules: []*moduleInfo{ @@ -1189,9 +1182,9 @@ func TestDeduplicateOrderOnlyDeps(t *testing.T) { expectedPhonys: []*buildDef{ b("dedup-GKw-c0PwFokMUQ6T-TUmEWnZ4_VlQ2Qpgw-vCTT0-OQ", []string{"d"}, nil), }, - conversions: map[string][]*ninjaString{ - "A": outputs("dedup-GKw-c0PwFokMUQ6T-TUmEWnZ4_VlQ2Qpgw-vCTT0-OQ"), - "B": outputs("dedup-GKw-c0PwFokMUQ6T-TUmEWnZ4_VlQ2Qpgw-vCTT0-OQ"), + conversions: map[string][]string{ + "A": []string{"dedup-GKw-c0PwFokMUQ6T-TUmEWnZ4_VlQ2Qpgw-vCTT0-OQ"}, + "B": []string{"dedup-GKw-c0PwFokMUQ6T-TUmEWnZ4_VlQ2Qpgw-vCTT0-OQ"}, }, }, { modules: []*moduleInfo{ @@ -1205,10 +1198,10 @@ func TestDeduplicateOrderOnlyDeps(t *testing.T) { m(b("C", nil, []string{"a"})), }, expectedPhonys: []*buildDef{b("dedup-ypeBEsobvcr6wjGzmiPcTaeG7_gUfE5yuYB3ha_uSLs", []string{"a"}, nil)}, - conversions: map[string][]*ninjaString{ - "A": outputs("dedup-ypeBEsobvcr6wjGzmiPcTaeG7_gUfE5yuYB3ha_uSLs"), - "B": outputs("b"), - "C": outputs("dedup-ypeBEsobvcr6wjGzmiPcTaeG7_gUfE5yuYB3ha_uSLs"), + conversions: map[string][]string{ + "A": []string{"dedup-ypeBEsobvcr6wjGzmiPcTaeG7_gUfE5yuYB3ha_uSLs"}, + "B": []string{"b"}, + "C": []string{"dedup-ypeBEsobvcr6wjGzmiPcTaeG7_gUfE5yuYB3ha_uSLs"}, }, }, { modules: []*moduleInfo{ @@ -1220,11 +1213,11 @@ func TestDeduplicateOrderOnlyDeps(t *testing.T) { expectedPhonys: []*buildDef{ b("dedup--44g_C5MPySMYMOb1lLzwTRymLuXe4tNWQO4UFViBgM", []string{"a", "b"}, nil), b("dedup-9F3lHN7zCZFVHkHogt17VAR5lkigoAdT9E_JZuYVP8E", []string{"a", "c"}, nil)}, - conversions: map[string][]*ninjaString{ - "A": outputs("dedup--44g_C5MPySMYMOb1lLzwTRymLuXe4tNWQO4UFViBgM"), - "B": outputs("dedup--44g_C5MPySMYMOb1lLzwTRymLuXe4tNWQO4UFViBgM"), - "C": outputs("dedup-9F3lHN7zCZFVHkHogt17VAR5lkigoAdT9E_JZuYVP8E"), - "D": outputs("dedup-9F3lHN7zCZFVHkHogt17VAR5lkigoAdT9E_JZuYVP8E"), + conversions: map[string][]string{ + "A": []string{"dedup--44g_C5MPySMYMOb1lLzwTRymLuXe4tNWQO4UFViBgM"}, + "B": []string{"dedup--44g_C5MPySMYMOb1lLzwTRymLuXe4tNWQO4UFViBgM"}, + "C": []string{"dedup-9F3lHN7zCZFVHkHogt17VAR5lkigoAdT9E_JZuYVP8E"}, + "D": []string{"dedup-9F3lHN7zCZFVHkHogt17VAR5lkigoAdT9E_JZuYVP8E"}, }, }} for index, tc := range testCases { @@ -1253,7 +1246,7 @@ func TestDeduplicateOrderOnlyDeps(t *testing.T) { find := func(k string) *buildDef { for _, m := range tc.modules { for _, b := range m.actionDefs.buildDefs { - if reflect.DeepEqual(b.Outputs, outputs(k)) { + if reflect.DeepEqual(b.OutputStrings, []string{k}) { return b } } @@ -1265,7 +1258,7 @@ func TestDeduplicateOrderOnlyDeps(t *testing.T) { if actual == nil { t.Errorf("Couldn't find %s", k) } - if !reflect.DeepEqual(actual.OrderOnly, conversion) { + if !reflect.DeepEqual(actual.OrderOnlyStrings, conversion) { t.Errorf("expected %s.OrderOnly = %v but got %v", k, conversion, actual.OrderOnly) } } diff --git a/go.mod b/go.mod index e278f2f..7b8869e 100644 --- a/go.mod +++ b/go.mod @@ -1,3 +1,3 @@ module github.com/google/blueprint -go 1.18 +go 1.20 diff --git a/ninja_defs.go b/ninja_defs.go index 482c80c..a1a3124 100644 --- a/ninja_defs.go +++ b/ninja_defs.go @@ -261,18 +261,24 @@ func (r *ruleDef) WriteTo(nw *ninjaWriter, name string, // A buildDef describes a build target definition. type buildDef struct { - Comment string - Rule Rule - RuleDef *ruleDef - Outputs []*ninjaString - ImplicitOutputs []*ninjaString - Inputs []*ninjaString - Implicits []*ninjaString - OrderOnly []*ninjaString - Validations []*ninjaString - Args map[Variable]*ninjaString - Variables map[string]*ninjaString - Optional bool + Comment string + Rule Rule + RuleDef *ruleDef + Outputs []*ninjaString + OutputStrings []string + ImplicitOutputs []*ninjaString + ImplicitOutputStrings []string + Inputs []*ninjaString + InputStrings []string + Implicits []*ninjaString + ImplicitStrings []string + OrderOnly []*ninjaString + OrderOnlyStrings []string + Validations []*ninjaString + ValidationStrings []string + Args map[Variable]*ninjaString + Variables map[string]*ninjaString + Optional bool } func formatTags(tags map[string]string, rule Rule) string { @@ -319,32 +325,32 @@ func parseBuildParams(scope scope, params *BuildParams, } var err error - b.Outputs, err = parseNinjaStrings(scope, params.Outputs) + b.Outputs, b.OutputStrings, err = parseNinjaOrSimpleStrings(scope, params.Outputs) if err != nil { return nil, fmt.Errorf("error parsing Outputs param: %s", err) } - b.ImplicitOutputs, err = parseNinjaStrings(scope, params.ImplicitOutputs) + b.ImplicitOutputs, b.ImplicitOutputStrings, err = parseNinjaOrSimpleStrings(scope, params.ImplicitOutputs) if err != nil { return nil, fmt.Errorf("error parsing ImplicitOutputs param: %s", err) } - b.Inputs, err = parseNinjaStrings(scope, params.Inputs) + b.Inputs, b.InputStrings, err = parseNinjaOrSimpleStrings(scope, params.Inputs) if err != nil { return nil, fmt.Errorf("error parsing Inputs param: %s", err) } - b.Implicits, err = parseNinjaStrings(scope, params.Implicits) + b.Implicits, b.ImplicitStrings, err = parseNinjaOrSimpleStrings(scope, params.Implicits) if err != nil { return nil, fmt.Errorf("error parsing Implicits param: %s", err) } - b.OrderOnly, err = parseNinjaStrings(scope, params.OrderOnly) + b.OrderOnly, b.OrderOnlyStrings, err = parseNinjaOrSimpleStrings(scope, params.OrderOnly) if err != nil { return nil, fmt.Errorf("error parsing OrderOnly param: %s", err) } - b.Validations, err = parseNinjaStrings(scope, params.Validations) + b.Validations, b.ValidationStrings, err = parseNinjaOrSimpleStrings(scope, params.Validations) if err != nil { return nil, fmt.Errorf("error parsing Validations param: %s", err) } @@ -411,14 +417,20 @@ func parseBuildParams(scope scope, params *BuildParams, func (b *buildDef) WriteTo(nw *ninjaWriter, pkgNames map[*packageContext]string) error { var ( - comment = b.Comment - rule = b.Rule.fullName(pkgNames) - outputs = b.Outputs - implicitOuts = b.ImplicitOutputs - explicitDeps = b.Inputs - implicitDeps = b.Implicits - orderOnlyDeps = b.OrderOnly - validations = b.Validations + comment = b.Comment + rule = b.Rule.fullName(pkgNames) + outputs = b.Outputs + implicitOuts = b.ImplicitOutputs + explicitDeps = b.Inputs + implicitDeps = b.Implicits + orderOnlyDeps = b.OrderOnly + validations = b.Validations + outputStrings = b.OutputStrings + implicitOutStrings = b.ImplicitOutputStrings + explicitDepStrings = b.InputStrings + implicitDepStrings = b.ImplicitStrings + orderOnlyDepStrings = b.OrderOnlyStrings + validationStrings = b.ValidationStrings ) if b.RuleDef != nil { @@ -426,7 +438,10 @@ func (b *buildDef) WriteTo(nw *ninjaWriter, pkgNames map[*packageContext]string) orderOnlyDeps = append(b.RuleDef.CommandOrderOnly, orderOnlyDeps...) } - err := nw.Build(comment, rule, outputs, implicitOuts, explicitDeps, implicitDeps, orderOnlyDeps, validations, pkgNames) + err := nw.Build(comment, rule, outputs, implicitOuts, explicitDeps, implicitDeps, orderOnlyDeps, validations, + outputStrings, implicitOutStrings, explicitDepStrings, + implicitDepStrings, orderOnlyDepStrings, validationStrings, + pkgNames) if err != nil { return err } @@ -456,7 +471,7 @@ func (b *buildDef) WriteTo(nw *ninjaWriter, pkgNames map[*packageContext]string) } if !b.Optional { - err = nw.Default(pkgNames, outputs...) + err = nw.Default(pkgNames, outputs, outputStrings) if err != nil { return err } diff --git a/ninja_strings.go b/ninja_strings.go index 0e565e2..78368ac 100644 --- a/ninja_strings.go +++ b/ninja_strings.go @@ -87,16 +87,31 @@ func (ps *parseState) pushVariable(start, end int, v Variable) { type stateFunc func(*parseState, int, rune) (stateFunc, error) // parseNinjaString parses an unescaped ninja string (i.e. all $ -// occurrences are expected to be variables or $$) and returns a list of the -// variable names that the string references. +// occurrences are expected to be variables or $$) and returns a *ninjaString +// that contains the original string and a list of the referenced variables. func parseNinjaString(scope scope, str string) (*ninjaString, error) { + ninjaString, str, err := parseNinjaOrSimpleString(scope, str) + if err != nil { + return nil, err + } + if ninjaString != nil { + return ninjaString, nil + } + return simpleNinjaString(str), nil +} + +// parseNinjaOrSimpleString parses an unescaped ninja string (i.e. all $ +// occurrences are expected to be variables or $$) and returns either a *ninjaString +// if the string contains ninja variable references, or the original string and nil +// for the *ninjaString if it doesn't. +func parseNinjaOrSimpleString(scope scope, str string) (*ninjaString, string, error) { // naively pre-allocate slice by counting $ signs n := strings.Count(str, "$") if n == 0 { if len(str) > 0 && str[0] == ' ' { str = "$" + str } - return simpleNinjaString(str), nil + return nil, str, nil } variableReferences := make([]variableReference, 0, n) result := &ninjaString{ @@ -116,13 +131,13 @@ func parseNinjaString(scope scope, str string) (*ninjaString, error) { r := rune(str[i]) state, err = state(parseState, i, r) if err != nil { - return nil, fmt.Errorf("error parsing ninja string %q: %s", str, err) + return nil, "", fmt.Errorf("error parsing ninja string %q: %s", str, err) } } _, err = state(parseState, len(parseState.str), eof) if err != nil { - return nil, err + return nil, "", err } // All the '$' characters counted initially could have been "$$" escapes, leaving no @@ -131,7 +146,7 @@ func parseNinjaString(scope scope, str string) (*ninjaString, error) { result.variables = nil } - return result, nil + return result, "", nil } func parseFirstRuneState(state *parseState, i int, r rune) (stateFunc, error) { @@ -246,6 +261,8 @@ func parseBracketsState(state *parseState, i int, r rune) (stateFunc, error) { } } +// parseNinjaStrings converts a list of strings to *ninjaStrings by finding the references +// to ninja variables contained in the strings. func parseNinjaStrings(scope scope, strs []string) ([]*ninjaString, error) { @@ -263,6 +280,50 @@ func parseNinjaStrings(scope scope, strs []string) ([]*ninjaString, return result, nil } +// parseNinjaOrSimpleStrings splits a list of strings into *ninjaStrings if they have ninja +// variable references or a list of strings if they don't. If none of the input strings contain +// ninja variable references (a very common case) then it returns the unmodified input slice as +// the output slice. +func parseNinjaOrSimpleStrings(scope scope, strs []string) ([]*ninjaString, []string, error) { + if len(strs) == 0 { + return nil, strs, nil + } + + // allSimpleStrings is true until the first time a string with ninja variable references is found. + allSimpleStrings := true + var simpleStrings []string + var ninjaStrings []*ninjaString + + for i, str := range strs { + ninjaStr, simpleStr, err := parseNinjaOrSimpleString(scope, str) + if err != nil { + return nil, nil, fmt.Errorf("error parsing element %d: %s", i, err) + } else if ninjaStr != nil { + ninjaStrings = append(ninjaStrings, ninjaStr) + if allSimpleStrings && i > 0 { + // If all previous strings had no ninja variable references then they weren't copied into + // simpleStrings to avoid allocating it if the input slice is reused as the output. Allocate + // simpleStrings and copy all the previous strings into it. + simpleStrings = make([]string, i, len(strs)) + copy(simpleStrings, strs[:i]) + } + allSimpleStrings = false + } else { + if !allSimpleStrings { + // Only copy into the output slice if at least one string with ninja variable references + // was found. Skipped strings will be copied the first time a string with ninja variable + // is found. + simpleStrings = append(simpleStrings, simpleStr) + } + } + } + if allSimpleStrings { + // None of the input strings had ninja variable references, return the input slice as the output. + return nil, strs, nil + } + return ninjaStrings, simpleStrings, nil +} + func (n *ninjaString) Value(pkgNames map[*packageContext]string) string { if n.variables == nil || len(*n.variables) == 0 { return defaultEscaper.Replace(n.str) diff --git a/ninja_strings_test.go b/ninja_strings_test.go index 2fa4937..8669e60 100644 --- a/ninja_strings_test.go +++ b/ninja_strings_test.go @@ -19,6 +19,7 @@ import ( "strconv" "strings" "testing" + "unsafe" ) type testVariableRef struct { @@ -215,6 +216,105 @@ func TestParseNinjaStringWithImportedVar(t *testing.T) { } } +func Test_parseNinjaOrSimpleStrings(t *testing.T) { + testCases := []struct { + name string + in []string + outStrings []string + outNinjaStrings []string + sameSlice bool + }{ + { + name: "nil", + in: nil, + sameSlice: true, + }, + { + name: "empty", + in: []string{}, + sameSlice: true, + }, + { + name: "string", + in: []string{"abc"}, + sameSlice: true, + }, + { + name: "ninja string", + in: []string{"$abc"}, + outStrings: nil, + outNinjaStrings: []string{"${abc}"}, + }, + { + name: "ninja string first", + in: []string{"$abc", "def", "ghi"}, + outStrings: []string{"def", "ghi"}, + outNinjaStrings: []string{"${abc}"}, + }, + { + name: "ninja string middle", + in: []string{"abc", "$def", "ghi"}, + outStrings: []string{"abc", "ghi"}, + outNinjaStrings: []string{"${def}"}, + }, + { + name: "ninja string last", + in: []string{"abc", "def", "$ghi"}, + outStrings: []string{"abc", "def"}, + outNinjaStrings: []string{"${ghi}"}, + }, + } + + for _, tt := range testCases { + t.Run(tt.name, func(t *testing.T) { + inCopy := append([]string(nil), tt.in...) + + scope := newLocalScope(nil, "") + scope.AddLocalVariable("abc", "abc") + scope.AddLocalVariable("def", "def") + scope.AddLocalVariable("ghi", "ghi") + gotNinjaStrings, gotStrings, err := parseNinjaOrSimpleStrings(scope, tt.in) + if err != nil { + t.Errorf("unexpected error %s", err) + } + + wantStrings := tt.outStrings + if tt.sameSlice { + wantStrings = tt.in + } + + wantNinjaStrings := tt.outNinjaStrings + + var evaluatedNinjaStrings []string + if gotNinjaStrings != nil { + evaluatedNinjaStrings = make([]string, 0, len(gotNinjaStrings)) + for _, ns := range gotNinjaStrings { + evaluatedNinjaStrings = append(evaluatedNinjaStrings, ns.Value(nil)) + } + } + + if !reflect.DeepEqual(gotStrings, wantStrings) { + t.Errorf("incorrect strings output, want %q got %q", wantStrings, gotStrings) + } + if !reflect.DeepEqual(evaluatedNinjaStrings, wantNinjaStrings) { + t.Errorf("incorrect ninja strings output, want %q got %q", wantNinjaStrings, evaluatedNinjaStrings) + } + if len(inCopy) != len(tt.in) && (len(tt.in) == 0 || !reflect.DeepEqual(inCopy, tt.in)) { + t.Errorf("input modified, want %#v, got %#v", inCopy, tt.in) + } + + if (unsafe.SliceData(tt.in) == unsafe.SliceData(gotStrings)) != tt.sameSlice { + if tt.sameSlice { + t.Errorf("expected input and output slices to have the same backing arrays") + } else { + t.Errorf("expected input and output slices to have different backing arrays") + } + } + + }) + } +} + func Benchmark_parseNinjaString(b *testing.B) { b.Run("constant", func(b *testing.B) { for _, l := range []int{1, 10, 100, 1000} { diff --git a/ninja_writer.go b/ninja_writer.go index fbbfb9e..11dd0d0 100644 --- a/ninja_writer.go +++ b/ninja_writer.go @@ -34,12 +34,12 @@ type StringWriterWriter interface { } type ninjaWriter struct { - writer io.StringWriter + writer StringWriterWriter justDidBlankLine bool // true if the last operation was a BlankLine } -func newNinjaWriter(writer io.StringWriter) *ninjaWriter { +func newNinjaWriter(writer StringWriterWriter) *ninjaWriter { return &ninjaWriter{ writer: writer, } @@ -115,6 +115,8 @@ func (n *ninjaWriter) Rule(name string) error { func (n *ninjaWriter) Build(comment string, rule string, outputs, implicitOuts, explicitDeps, implicitDeps, orderOnlyDeps, validations []*ninjaString, + outputStrings, implicitOutStrings, explicitDepStrings, + implicitDepStrings, orderOnlyDepStrings, validationStrings []string, pkgNames map[*packageContext]string) error { n.justDidBlankLine = false @@ -136,14 +138,22 @@ func (n *ninjaWriter) Build(comment string, rule string, outputs, implicitOuts, wrapper.WriteString("build") + for _, output := range outputStrings { + wrapper.Space() + outputEscaper.WriteString(wrapper, output) + } for _, output := range outputs { wrapper.Space() output.ValueWithEscaper(wrapper, pkgNames, outputEscaper) } - if len(implicitOuts) > 0 { + if len(implicitOuts) > 0 || len(implicitOutStrings) > 0 { wrapper.WriteStringWithSpace("|") + for _, out := range implicitOutStrings { + wrapper.Space() + outputEscaper.WriteString(wrapper, out) + } for _, out := range implicitOuts { wrapper.Space() out.ValueWithEscaper(wrapper, pkgNames, outputEscaper) @@ -154,32 +164,48 @@ func (n *ninjaWriter) Build(comment string, rule string, outputs, implicitOuts, wrapper.WriteStringWithSpace(rule) + for _, dep := range explicitDepStrings { + wrapper.Space() + inputEscaper.WriteString(wrapper, dep) + } for _, dep := range explicitDeps { wrapper.Space() dep.ValueWithEscaper(wrapper, pkgNames, inputEscaper) } - if len(implicitDeps) > 0 { + if len(implicitDeps) > 0 || len(implicitDepStrings) > 0 { wrapper.WriteStringWithSpace("|") + for _, dep := range implicitDepStrings { + wrapper.Space() + inputEscaper.WriteString(wrapper, dep) + } for _, dep := range implicitDeps { wrapper.Space() dep.ValueWithEscaper(wrapper, pkgNames, inputEscaper) } } - if len(orderOnlyDeps) > 0 { + if len(orderOnlyDeps) > 0 || len(orderOnlyDepStrings) > 0 { wrapper.WriteStringWithSpace("||") + for _, dep := range orderOnlyDepStrings { + wrapper.Space() + inputEscaper.WriteString(wrapper, dep) + } for _, dep := range orderOnlyDeps { wrapper.Space() dep.ValueWithEscaper(wrapper, pkgNames, inputEscaper) } } - if len(validations) > 0 { + if len(validations) > 0 || len(validationStrings) > 0 { wrapper.WriteStringWithSpace("|@") + for _, dep := range validationStrings { + wrapper.Space() + inputEscaper.WriteString(wrapper, dep) + } for _, dep := range validations { wrapper.Space() dep.ValueWithEscaper(wrapper, pkgNames, inputEscaper) @@ -235,7 +261,7 @@ func (n *ninjaWriter) ScopedAssign(name, value string) error { return nil } -func (n *ninjaWriter) Default(pkgNames map[*packageContext]string, targets ...*ninjaString) error { +func (n *ninjaWriter) Default(pkgNames map[*packageContext]string, targets []*ninjaString, targetStrings []string) error { n.justDidBlankLine = false const lineWrapLen = len(" $") @@ -248,6 +274,10 @@ func (n *ninjaWriter) Default(pkgNames map[*packageContext]string, targets ...*n wrapper.WriteString("default") + for _, target := range targetStrings { + wrapper.Space() + outputEscaper.WriteString(wrapper, target) + } for _, target := range targets { wrapper.Space() target.ValueWithEscaper(wrapper, pkgNames, outputEscaper) @@ -347,6 +377,7 @@ func (n *ninjaWriterWithWrap) WriteString(s string) (written int, noError error) n.lineLen = indentWidth*2 + n.pendingLen s = strings.TrimLeftFunc(s, unicode.IsSpace) n.pending = append(n.pending, s) + n.lineLen += len(s) n.writePending() n.space = false @@ -361,6 +392,11 @@ func (n *ninjaWriterWithWrap) WriteString(s string) (written int, noError error) return } +func (n *ninjaWriterWithWrap) Write(p []byte) (written int, noError error) { + // Write is rarely used, implement it via WriteString. + return n.WriteString(string(p)) +} + // Space inserts a space that is also a possible wrapping point into the string. func (n *ninjaWriterWithWrap) Space() { if n.err != nil { diff --git a/ninja_writer_test.go b/ninja_writer_test.go index c70d5df..f558617 100644 --- a/ninja_writer_test.go +++ b/ninja_writer_test.go @@ -50,12 +50,15 @@ var ninjaWriterTestCases = []struct { }, { input: func(w *ninjaWriter) { - 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)) + ck(w.Build("foo comment", "foo", testNinjaStrings("o3", "o4"), + testNinjaStrings("io3", "io4"), testNinjaStrings("e3", "e4"), + testNinjaStrings("i3", "i4"), testNinjaStrings("oo3", "oo4"), + testNinjaStrings("v3", "v4"), []string{"o1", "o2"}, + []string{"io1", "io2"}, []string{"e1", "e2"}, + []string{"i1", "i2"}, []string{"oo1", "oo2"}, + []string{"v1", "v2"}, nil)) }, - output: "# foo comment\nbuild o1 o2 | io1 io2: foo e1 e2 | i1 i2 || oo1 oo2 |@ v1 v2\n", + output: "# foo comment\nbuild o1 o2 o3 o4 | io1 io2 io3 io4: foo e1 e2 e3 e4 | i1 i2 i3 i4 || oo1 oo2 $\n oo3 oo4 |@ v1 v2 v3 v4\n", }, { input: func(w *ninjaWriter) { @@ -63,15 +66,15 @@ var ninjaWriterTestCases = []struct { testNinjaStrings(strings.Repeat("o", lineWidth)), nil, testNinjaStrings(strings.Repeat("i", lineWidth)), - nil, nil, nil, nil)) + nil, nil, nil, nil, nil, nil, nil, nil, nil, nil)) }, - output: "# foo comment\nbuild $\n " + strings.Repeat("o", lineWidth) + ": foo $\n " + strings.Repeat("i", lineWidth) + "\n", + output: "# foo comment\nbuild $\n " + strings.Repeat("o", lineWidth) + ": $\n foo $\n " + strings.Repeat("i", lineWidth) + "\n", }, { input: func(w *ninjaWriter) { - ck(w.Default(nil, testNinjaStrings("foo")...)) + ck(w.Default(nil, testNinjaStrings("foo"), []string{"bar"})) }, - output: "default foo\n", + output: "default bar foo\n", }, { input: func(w *ninjaWriter) { @@ -108,7 +111,8 @@ var ninjaWriterTestCases = []struct { ck(w.ScopedAssign("pool", "p")) ck(w.BlankLine()) ck(w.Build("r comment", "r", testNinjaStrings("foo.o"), - nil, testNinjaStrings("foo.in"), nil, nil, nil, nil)) + nil, testNinjaStrings("foo.in"), nil, nil, nil, nil, + nil, nil, nil, nil, nil, nil)) ck(w.ScopedAssign("_arg", "arg value")) }, output: `pool p