Optimize and simplify order-only dep deduplication am: bef8688e45 am: 86c4dd91a5 am: 60af4fd79c am: e151d1cc0e

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

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

context.go

@@ -17,6 +17,8 @@ package blueprint
 import (
 	"bytes"
 	"context"
+	"crypto/sha256"
+	"encoding/base64"
 	"encoding/json"
 	"errors"
 	"fmt"
@@ -4333,7 +4335,7 @@ func (c *Context) writeAllModuleActions(nw *ninjaWriter) error {
 	}
 	sort.Sort(moduleSorter{modules, c.nameInterface})
 
-	phonys := c.extractPhonys(modules)
+	phonys := c.deduplicateOrderOnlyDeps(modules)
 	if err := c.writeLocalBuildActions(nw, phonys); err != nil {
 		return err
 	}
@@ -4463,36 +4465,23 @@ func (c *Context) SetBeforePrepareBuildActionsHook(hookFn func() error) {
 // phonyCandidate represents the state of a set of deps that decides its eligibility
 // to be extracted as a phony output
 type phonyCandidate struct {
-	sync.Mutex
+	sync.Once
-	frequency int       // the number of buildDef instances that use this set
+	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
-	key       string    // a unique identifier for the set
-}
-
-func (c *phonyCandidate) less(other *phonyCandidate) bool {
-	if c.frequency == other.frequency {
-		if len(c.phony.OrderOnly) == len(other.phony.OrderOnly) {
-			return c.key < other.key
-		}
-		return len(c.phony.OrderOnly) < len(other.phony.OrderOnly)
-	}
-	return c.frequency < other.frequency
 }
 
 // keyForPhonyCandidate gives a unique identifier for a set of deps.
-// We are not using hash because string concatenation proved cheaper.
 // 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 {
-	s := make([]string, len(deps))
+	hasher := sha256.New()
-	for i, d := range deps {
+	for _, d := range deps {
 		if len(d.Variables()) != 0 {
 			return ""
 		}
-		s[i] = d.Value(nil)
+		io.WriteString(hasher, d.Value(nil))
 	}
-	return strings.Join(s, "\n")
+	return base64.RawURLEncoding.EncodeToString(hasher.Sum(nil))
 }
 
 // scanBuildDef is called for every known buildDef `b` that has a non-empty `b.OrderOnly`.
@@ -4506,45 +4495,35 @@ func scanBuildDef(wg *sync.WaitGroup, candidates *sync.Map, phonyCount *atomic.U
 		return
 	}
 	if v, loaded := candidates.LoadOrStore(key, &phonyCandidate{
-		frequency: 1,
+		first: b,
-		first:     b,
-		key:       key,
 	}); loaded {
 		m := v.(*phonyCandidate)
-		func() {
+		m.Do(func() {
-			m.Lock()
+			// this is the second occurrence and hence it makes sense to
-			defer m.Unlock()
+			// extract it as a phony output
-			if m.frequency == 1 {
+			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,
-				phonyCount.Add(1)
+				Outputs:  []ninjaString{simpleNinjaString("dedup-" + key)},
-				m.phony = &buildDef{
+				Inputs:   m.first.OrderOnly, //we could also use b.OrderOnly
-					Rule: Phony,
+				Optional: true,
-					// We are using placeholder because we don't have a deterministic
-					// name for the phony output; m.key is unique and could be used but
-					// it's rather long (and has characters we would need to escape)
-					Outputs:  make([]ninjaString, 1),
-					Inputs:   m.first.OrderOnly, //we could also use b.OrderOnly
-					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 = nil
 			}
-			m.frequency += 1
+			// the previously recorded build-def, which first had these deps as its
-			b.OrderOnly = m.phony.Outputs
+			// order-only deps, should now use this phony output instead
-		}()
+			m.first.OrderOnly = m.phony.Outputs
+			m.first = nil
+		})
+		b.OrderOnly = m.phony.Outputs
 	}
 }
 
-// extractPhonys searches for common sets of order-only dependencies across all
+// deduplicateOrderOnlyDeps searches for common sets of order-only dependencies across all
 // 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) extractPhonys(infos []*moduleInfo) *localBuildActions {
+func (c *Context) deduplicateOrderOnlyDeps(infos []*moduleInfo) *localBuildActions {
-	c.BeginEvent("extract_phonys")
+	c.BeginEvent("deduplicate_order_only_deps")
-	defer c.EndEvent("extract_phonys")
+	defer c.EndEvent("deduplicate_order_only_deps")
 
 	candidates := sync.Map{} //used as map[key]*candidate
 	phonyCount := atomic.Uint32{}
@@ -4559,30 +4538,24 @@ func (c *Context) extractPhonys(infos []*moduleInfo) *localBuildActions {
 	}
 	wg.Wait()
 
-	//now filter candidates with freq > 1
+	// now collect all created phonys to return
-	phonys := make([]*phonyCandidate, 0, phonyCount.Load())
+	phonys := make([]*buildDef, 0, phonyCount.Load())
 	candidates.Range(func(_ any, v any) bool {
 		candidate := v.(*phonyCandidate)
-		if candidate.frequency > 1 {
+		if candidate.phony != nil {
-			phonys = append(phonys, candidate)
+			phonys = append(phonys, candidate.phony)
 		}
 		return true
 	})
 
-	phonyBuildDefs := make([]*buildDef, len(phonys))
+	c.EventHandler.Do("sort_phony_builddefs", func() {
-	c.EventHandler.Do("name", func() {
+		// sorting for determinism, the phony output names are stable
-		// sorting for determinism
 		sort.Slice(phonys, func(i int, j int) bool {
-			return phonys[i].less(phonys[j])
+			return phonys[i].Outputs[0].Value(nil) < phonys[j].Outputs[0].Value(nil)
 		})
-		for index, p := range phonys {
-			// use the index to set the name for the phony output
-			p.phony.Outputs[0] = literalNinjaString(fmt.Sprintf("phony-%d", index))
-			phonyBuildDefs[index] = p.phony
-		}
 	})
 
-	return &localBuildActions{buildDefs: phonyBuildDefs}
+	return &localBuildActions{buildDefs: phonys}
 }
 
 func (c *Context) writeLocalBuildActions(nw *ninjaWriter,

context_test.go

@@ -1158,7 +1158,7 @@ func TestPackageIncludes(t *testing.T) {
 
 }
 
-func TestExtractPhonys(t *testing.T) {
+func TestDeduplicateOrderOnlyDeps(t *testing.T) {
 	outputs := func(names ...string) []ninjaString {
 		r := make([]ninjaString, len(names))
 		for i, name := range names {
@@ -1187,11 +1187,11 @@ func TestExtractPhonys(t *testing.T) {
 			m(b("B", nil, []string{"d"})),
 		},
 		expectedPhonys: []*buildDef{
-			b("phony-0", []string{"d"}, nil),
+			b("dedup-GKw-c0PwFokMUQ6T-TUmEWnZ4_VlQ2Qpgw-vCTT0-OQ", []string{"d"}, nil),
 		},
 		conversions: map[string][]ninjaString{
-			"A": outputs("phony-0"),
+			"A": outputs("dedup-GKw-c0PwFokMUQ6T-TUmEWnZ4_VlQ2Qpgw-vCTT0-OQ"),
-			"B": outputs("phony-0"),
+			"B": outputs("dedup-GKw-c0PwFokMUQ6T-TUmEWnZ4_VlQ2Qpgw-vCTT0-OQ"),
 		},
 	}, {
 		modules: []*moduleInfo{
@@ -1204,11 +1204,11 @@ func TestExtractPhonys(t *testing.T) {
 			m(b("B", nil, []string{"b"})),
 			m(b("C", nil, []string{"a"})),
 		},
-		expectedPhonys: []*buildDef{b("phony-0", []string{"a"}, nil)},
+		expectedPhonys: []*buildDef{b("dedup-ypeBEsobvcr6wjGzmiPcTaeG7_gUfE5yuYB3ha_uSLs", []string{"a"}, nil)},
 		conversions: map[string][]ninjaString{
-			"A": outputs("phony-0"),
+			"A": outputs("dedup-ypeBEsobvcr6wjGzmiPcTaeG7_gUfE5yuYB3ha_uSLs"),
 			"B": outputs("b"),
-			"C": outputs("phony-0"),
+			"C": outputs("dedup-ypeBEsobvcr6wjGzmiPcTaeG7_gUfE5yuYB3ha_uSLs"),
 		},
 	}, {
 		modules: []*moduleInfo{
@@ -1218,19 +1218,19 @@ func TestExtractPhonys(t *testing.T) {
 				b("D", nil, []string{"a", "c"})),
 		},
 		expectedPhonys: []*buildDef{
-			b("phony-0", []string{"a", "b"}, nil),
+			b("dedup--44g_C5MPySMYMOb1lLzwTRymLuXe4tNWQO4UFViBgM", []string{"a", "b"}, nil),
-			b("phony-1", []string{"a", "c"}, nil)},
+			b("dedup-9F3lHN7zCZFVHkHogt17VAR5lkigoAdT9E_JZuYVP8E", []string{"a", "c"}, nil)},
 		conversions: map[string][]ninjaString{
-			"A": outputs("phony-0"),
+			"A": outputs("dedup--44g_C5MPySMYMOb1lLzwTRymLuXe4tNWQO4UFViBgM"),
-			"B": outputs("phony-0"),
+			"B": outputs("dedup--44g_C5MPySMYMOb1lLzwTRymLuXe4tNWQO4UFViBgM"),
-			"C": outputs("phony-1"),
+			"C": outputs("dedup-9F3lHN7zCZFVHkHogt17VAR5lkigoAdT9E_JZuYVP8E"),
-			"D": outputs("phony-1"),
+			"D": outputs("dedup-9F3lHN7zCZFVHkHogt17VAR5lkigoAdT9E_JZuYVP8E"),
 		},
 	}}
 	for index, tc := range testCases {
 		t.Run(fmt.Sprintf("TestCase-%d", index), func(t *testing.T) {
 			ctx := NewContext()
-			actualPhonys := ctx.extractPhonys(tc.modules)
+			actualPhonys := ctx.deduplicateOrderOnlyDeps(tc.modules)
 			if len(actualPhonys.variables) != 0 {
 				t.Errorf("No variables expected but found %v", actualPhonys.variables)
 			}