From dbf18bec9803687acd203f81b89c0d4a0f635958 Mon Sep 17 00:00:00 2001 From: Colin Cross Date: Thu, 1 Feb 2024 12:31:03 -0800 Subject: [PATCH] Optimize deduplicateOrderOnlyDeps keyForPhonyCandidate was using sha256, which is a crypto hash and unnecessarily expensive for this use case. hash/maphash would be much faster because it implements WriteString and so doesn't cause an extra allocation to copy to a byte slice for every write, but it insists on randomizing the seed, which makes it unsuitable for writing to the build.ninja file. Use hash/fnv instead, and use unsafe to write strings to the hash to avoid the extra allocation. Also replace the manually rolled parallelism with the existing parallelVisit, which will reuse goroutines and limit the parallelism to a useful value. The hash could collide, and using a 64-bit hash makes that more likely, so also check the full contents to make sure they are really equal. Cuts 1 second off Soong analysis time. Test: SOONG_PROFILE_MEM=/tmp/mem.pprof m nothing Change-Id: I4d1292cb158cfc5823a0f4d8b4aeac1d0b10230e --- context.go | 101 ++++++++++++++++++++++++++--------------------- context_test.go | 31 +++++++++------ ninja_strings.go | 8 ++++ 3 files changed, 83 insertions(+), 57 deletions(-) diff --git a/context.go b/context.go index 4130700..a53d421 100644 --- a/context.go +++ b/context.go @@ -18,11 +18,10 @@ import ( "bytes" "cmp" "context" - "crypto/sha256" - "encoding/base64" "encoding/json" "errors" "fmt" + "hash/fnv" "io" "io/ioutil" "os" @@ -37,6 +36,7 @@ import ( "sync/atomic" "text/scanner" "text/template" + "unsafe" "github.com/google/blueprint/metrics" "github.com/google/blueprint/parser" @@ -4710,60 +4710,73 @@ func (c *Context) SetBeforePrepareBuildActionsHook(hookFn func() error) { // to be extracted as a phony output type phonyCandidate struct { sync.Once - phony *buildDef // the phony buildDef that wraps the set - first *buildDef // the first buildDef that uses this set + phony *buildDef // the phony buildDef that wraps the set + first *buildDef // the first buildDef that uses this set + orderOnlyStrings []string // the original OrderOnlyStrings of the first buildDef that uses this set + orderOnly []*ninjaString // the original OrderOnly of the first buildDef that uses this set } // 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, stringDeps []string) string { - hasher := sha256.New() +func keyForPhonyCandidate(deps []*ninjaString, stringDeps []string) uint64 { + hasher := fnv.New64a() + write := func(s string) { + // The hasher doesn't retain or modify the input slice, so pass the string data directly to avoid + // an extra allocation and copy. + _, err := hasher.Write(unsafe.Slice(unsafe.StringData(s), len(s))) + if err != nil { + panic(fmt.Errorf("write failed: %w", err)) + } + } for _, d := range deps { if len(d.Variables()) != 0 { - return "" + return 0 } - io.WriteString(hasher, d.Value(nil)) + write(d.Value(nil)) } for _, d := range stringDeps { - io.WriteString(hasher, d) + write(d) } - return base64.RawURLEncoding.EncodeToString(hasher.Sum(nil)) + return hasher.Sum64() } // 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() +func scanBuildDef(candidates *sync.Map, b *buildDef) { key := keyForPhonyCandidate(b.OrderOnly, b.OrderOnlyStrings) - if key == "" { + if key == 0 { return } if v, loaded := candidates.LoadOrStore(key, &phonyCandidate{ - first: b, + first: b, + orderOnly: b.OrderOnly, + orderOnlyStrings: b.OrderOnlyStrings, }); loaded { m := v.(*phonyCandidate) - m.Do(func() { - // 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, - 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.OrderOnlyStrings = m.phony.OutputStrings - m.first.OrderOnly = nil - m.first = nil - }) - b.OrderOnlyStrings = m.phony.OutputStrings - b.OrderOnly = nil + if slices.EqualFunc(m.orderOnly, b.OrderOnly, ninjaStringsEqual) && + slices.Equal(m.orderOnlyStrings, b.OrderOnlyStrings) { + m.Do(func() { + // this is the second occurrence and hence it makes sense to + // extract it as a phony output + m.phony = &buildDef{ + Rule: Phony, + OutputStrings: []string{fmt.Sprintf("dedup-%x", 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.OrderOnlyStrings = m.phony.OutputStrings + m.first.OrderOnly = nil + m.first = nil + }) + b.OrderOnlyStrings = m.phony.OutputStrings + b.OrderOnly = nil + } } } @@ -4771,25 +4784,23 @@ func scanBuildDef(wg *sync.WaitGroup, candidates *sync.Map, phonyCount *atomic.U // 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) deduplicateOrderOnlyDeps(infos []*moduleInfo) *localBuildActions { +func (c *Context) deduplicateOrderOnlyDeps(modules []*moduleInfo) *localBuildActions { c.BeginEvent("deduplicate_order_only_deps") defer c.EndEvent("deduplicate_order_only_deps") 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 || len(b.OrderOnlyStrings) > 0 { - wg.Add(1) - go scanBuildDef(&wg, &candidates, &phonyCount, b) + parallelVisit(modules, unorderedVisitorImpl{}, parallelVisitLimit, + func(m *moduleInfo, pause chan<- pauseSpec) bool { + for _, b := range m.actionDefs.buildDefs { + if len(b.OrderOnly) > 0 || len(b.OrderOnlyStrings) > 0 { + scanBuildDef(&candidates, b) + } } - } - } - wg.Wait() + return false + }) // now collect all created phonys to return - phonys := make([]*buildDef, 0, phonyCount.Load()) + var phonys []*buildDef candidates.Range(func(_ any, v any) bool { candidate := v.(*phonyCandidate) if candidate.phony != nil { diff --git a/context_test.go b/context_test.go index 521d163..b946497 100644 --- a/context_test.go +++ b/context_test.go @@ -18,8 +18,10 @@ import ( "bytes" "errors" "fmt" + "hash/fnv" "path/filepath" "reflect" + "strconv" "strings" "sync" "testing" @@ -1174,17 +1176,22 @@ func TestDeduplicateOrderOnlyDeps(t *testing.T) { expectedPhonys []*buildDef conversions map[string][]string } + fnvHash := func(s string) string { + hash := fnv.New64a() + hash.Write([]byte(s)) + return strconv.FormatUint(hash.Sum64(), 16) + } testCases := []testcase{{ modules: []*moduleInfo{ m(b("A", nil, []string{"d"})), m(b("B", nil, []string{"d"})), }, expectedPhonys: []*buildDef{ - b("dedup-GKw-c0PwFokMUQ6T-TUmEWnZ4_VlQ2Qpgw-vCTT0-OQ", []string{"d"}, nil), + b("dedup-"+fnvHash("d"), []string{"d"}, nil), }, conversions: map[string][]string{ - "A": []string{"dedup-GKw-c0PwFokMUQ6T-TUmEWnZ4_VlQ2Qpgw-vCTT0-OQ"}, - "B": []string{"dedup-GKw-c0PwFokMUQ6T-TUmEWnZ4_VlQ2Qpgw-vCTT0-OQ"}, + "A": []string{"dedup-" + fnvHash("d")}, + "B": []string{"dedup-" + fnvHash("d")}, }, }, { modules: []*moduleInfo{ @@ -1197,11 +1204,11 @@ func TestDeduplicateOrderOnlyDeps(t *testing.T) { m(b("B", nil, []string{"b"})), m(b("C", nil, []string{"a"})), }, - expectedPhonys: []*buildDef{b("dedup-ypeBEsobvcr6wjGzmiPcTaeG7_gUfE5yuYB3ha_uSLs", []string{"a"}, nil)}, + expectedPhonys: []*buildDef{b("dedup-"+fnvHash("a"), []string{"a"}, nil)}, conversions: map[string][]string{ - "A": []string{"dedup-ypeBEsobvcr6wjGzmiPcTaeG7_gUfE5yuYB3ha_uSLs"}, + "A": []string{"dedup-" + fnvHash("a")}, "B": []string{"b"}, - "C": []string{"dedup-ypeBEsobvcr6wjGzmiPcTaeG7_gUfE5yuYB3ha_uSLs"}, + "C": []string{"dedup-" + fnvHash("a")}, }, }, { modules: []*moduleInfo{ @@ -1211,13 +1218,13 @@ func TestDeduplicateOrderOnlyDeps(t *testing.T) { b("D", nil, []string{"a", "c"})), }, expectedPhonys: []*buildDef{ - b("dedup--44g_C5MPySMYMOb1lLzwTRymLuXe4tNWQO4UFViBgM", []string{"a", "b"}, nil), - b("dedup-9F3lHN7zCZFVHkHogt17VAR5lkigoAdT9E_JZuYVP8E", []string{"a", "c"}, nil)}, + b("dedup-"+fnvHash("ab"), []string{"a", "b"}, nil), + b("dedup-"+fnvHash("ac"), []string{"a", "c"}, nil)}, conversions: map[string][]string{ - "A": []string{"dedup--44g_C5MPySMYMOb1lLzwTRymLuXe4tNWQO4UFViBgM"}, - "B": []string{"dedup--44g_C5MPySMYMOb1lLzwTRymLuXe4tNWQO4UFViBgM"}, - "C": []string{"dedup-9F3lHN7zCZFVHkHogt17VAR5lkigoAdT9E_JZuYVP8E"}, - "D": []string{"dedup-9F3lHN7zCZFVHkHogt17VAR5lkigoAdT9E_JZuYVP8E"}, + "A": []string{"dedup-" + fnvHash("ab")}, + "B": []string{"dedup-" + fnvHash("ab")}, + "C": []string{"dedup-" + fnvHash("ac")}, + "D": []string{"dedup-" + fnvHash("ac")}, }, }} for index, tc := range testCases { diff --git a/ninja_strings.go b/ninja_strings.go index 8576eae..c351b93 100644 --- a/ninja_strings.go +++ b/ninja_strings.go @@ -18,6 +18,7 @@ import ( "bytes" "fmt" "io" + "slices" "strings" ) @@ -477,3 +478,10 @@ func validateArgNames(argNames []string) error { return nil } + +func ninjaStringsEqual(a, b *ninjaString) bool { + return a.str == b.str && + (a.variables == nil) == (b.variables == nil) && + (a.variables == nil || + slices.Equal(*a.variables, *b.variables)) +}