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

@@ -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
+	phony            *buildDef      // the phony buildDef that wraps the set
-	first *buildDef // the first buildDef that uses this 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 {
+func keyForPhonyCandidate(deps []*ninjaString, stringDeps []string) uint64 {
-	hasher := sha256.New()
+	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) {
+func scanBuildDef(candidates *sync.Map, b *buildDef) {
-	defer wg.Done()
 	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() {
+		if slices.EqualFunc(m.orderOnly, b.OrderOnly, ninjaStringsEqual) &&
-			// this is the second occurrence and hence it makes sense to
+			slices.Equal(m.orderOnlyStrings, b.OrderOnlyStrings) {
-			// extract it as a phony output
+			m.Do(func() {
-			phonyCount.Add(1)
+				// this is the second occurrence and hence it makes sense to
-			m.phony = &buildDef{
+				// extract it as a phony output
-				Rule:          Phony,
+				m.phony = &buildDef{
-				OutputStrings: []string{"dedup-" + key},
+					Rule:          Phony,
-				Inputs:        m.first.OrderOnly, //we could also use b.OrderOnly
+					OutputStrings: []string{fmt.Sprintf("dedup-%x", key)},
-				InputStrings:  m.first.OrderOnlyStrings,
+					Inputs:        m.first.OrderOnly, //we could also use b.OrderOnly
-				Optional:      true,
+					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
+				// the previously recorded build-def, which first had these deps as its
-			m.first.OrderOnlyStrings = m.phony.OutputStrings
+				// order-only deps, should now use this phony output instead
-			m.first.OrderOnly = nil
+				m.first.OrderOnlyStrings = m.phony.OutputStrings
-			m.first = nil
+				m.first.OrderOnly = nil
-		})
+				m.first = nil
-		b.OrderOnlyStrings = m.phony.OutputStrings
+			})
-		b.OrderOnly = 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{}
+	parallelVisit(modules, unorderedVisitorImpl{}, parallelVisitLimit,
-	wg := sync.WaitGroup{}
+		func(m *moduleInfo, pause chan<- pauseSpec) bool {
-	for _, info := range infos {
+			for _, b := range m.actionDefs.buildDefs {
-		for _, b := range info.actionDefs.buildDefs {
+				if len(b.OrderOnly) > 0 || len(b.OrderOnlyStrings) > 0 {
-			if len(b.OrderOnly) > 0 || len(b.OrderOnlyStrings) > 0 {
+					scanBuildDef(&candidates, b)
-				wg.Add(1)
+				}
-				go scanBuildDef(&wg, &candidates, &phonyCount, b)
 			}
-		}
+			return false
-	}
+		})
-	wg.Wait()
 
 	// 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 {

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"},
+			"A": []string{"dedup-" + fnvHash("d")},
-			"B": []string{"dedup-GKw-c0PwFokMUQ6T-TUmEWnZ4_VlQ2Qpgw-vCTT0-OQ"},
+			"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-"+fnvHash("ab"), []string{"a", "b"}, nil),
-			b("dedup-9F3lHN7zCZFVHkHogt17VAR5lkigoAdT9E_JZuYVP8E", []string{"a", "c"}, nil)},
+			b("dedup-"+fnvHash("ac"), []string{"a", "c"}, nil)},
 		conversions: map[string][]string{
-			"A": []string{"dedup--44g_C5MPySMYMOb1lLzwTRymLuXe4tNWQO4UFViBgM"},
+			"A": []string{"dedup-" + fnvHash("ab")},
-			"B": []string{"dedup--44g_C5MPySMYMOb1lLzwTRymLuXe4tNWQO4UFViBgM"},
+			"B": []string{"dedup-" + fnvHash("ab")},
-			"C": []string{"dedup-9F3lHN7zCZFVHkHogt17VAR5lkigoAdT9E_JZuYVP8E"},
+			"C": []string{"dedup-" + fnvHash("ac")},
-			"D": []string{"dedup-9F3lHN7zCZFVHkHogt17VAR5lkigoAdT9E_JZuYVP8E"},
+			"D": []string{"dedup-" + fnvHash("ac")},
 		},
 	}}
 	for index, tc := range testCases {

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))
+}