Merge changes I5894f739,I4d1292cb into main am: 9beaa476d6

Original change: https://android-review.googlesource.com/c/platform/build/blueprint/+/2947016

Change-Id: I2fdb25ebda8e2679fc9a1a0ad8acefbce5cab070
Signed-off-by: Automerger Merge Worker <android-build-automerger-merge-worker@system.gserviceaccount.com>

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"
@@ -4729,60 +4729,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
+		}
 	}
 }
 
@@ -4790,25 +4803,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 {

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 {

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

proptools/hash_provider.go

@@ -19,7 +19,6 @@ import (
 	"encoding/binary"
 	"fmt"
 	"hash/maphash"
-	"io"
 	"math"
 	"reflect"
 	"sort"
@@ -44,7 +43,7 @@ func HashProvider(provider interface{}) (uint64, error) {
 	return hasher.Sum64(), err
 }
 
-func hashProviderInternal(hasher io.Writer, v reflect.Value, ptrs map[uintptr]bool) error {
+func hashProviderInternal(hasher *maphash.Hash, v reflect.Value, ptrs map[uintptr]bool) error {
 	var int64Array [8]byte
 	int64Buf := int64Array[:]
 	binary.LittleEndian.PutUint64(int64Buf, uint64(v.Kind()))
@@ -129,7 +128,7 @@ func hashProviderInternal(hasher io.Writer, v reflect.Value, ptrs map[uintptr]bo
 			}
 		}
 	case reflect.String:
-		hasher.Write([]byte(v.String()))
+		hasher.WriteString(v.String())
 	case reflect.Bool:
 		if v.Bool() {
 			int64Buf[0] = 1