tequilaOS/platform_build_blueprint
2
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Optimize and simplify order-only dep deduplication

Browse source 
This reduces the extract_phonys (now deduplicate_order_only_deps)
event's time from ~1.9s to ~1.5s on aosp-master, and from ~5.3s to ~4.6s
on internal master.

It does so by making keyForPhonyCandidate be based on a hash instead
of joining all the deps together. Having a hash allows us to also use
it as the name of the phony target, which simplifies the code a little.

Bug: None (original cl introducing extractPhonys also didn't have a bug)
Test: go tests
Change-Id: I2ff6e4614f19ccbfe99112ea7ae1ea33cd1df21b
This commit is contained in:
Cole Faust 2023-03-15 15:49:17 -07:00
parent ef778a12ac
commit bef8688e45
2 changed files with 52 additions and 79 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

103
context.go
View file

@ -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
frequency int // the number of buildDef instances that use this set
phony *buildDef // the phony buildDef that wraps the 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
sync.Once
phony *buildDef // the phony buildDef that wraps the set
first *buildDef // the first buildDef that uses this set
}
// 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))
for i, d := range deps {
hasher := sha256.New()
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,
key: key,
first: b,
}); loaded {
m := v.(*phonyCandidate)
func() {
m.Lock()
defer m.Unlock()
if m.frequency == 1 {
// 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,
// 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.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,
Outputs: []ninjaString{simpleNinjaString("dedup-" + key)},
Inputs: m.first.OrderOnly, //we could also use b.OrderOnly
Optional: true,
}
m.frequency += 1
b.OrderOnly = m.phony.Outputs
}()
// 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
})
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 {
c.BeginEvent("extract_phonys")
defer c.EndEvent("extract_phonys")
func (c *Context) deduplicateOrderOnlyDeps(infos []*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{}
@ -4559,30 +4538,24 @@ func (c *Context) extractPhonys(infos []*moduleInfo) *localBuildActions {
}
wg.Wait()
//now filter candidates with freq > 1
phonys := make([]*phonyCandidate, 0, phonyCount.Load())
// now collect all created phonys to return
phonys := make([]*buildDef, 0, phonyCount.Load())
candidates.Range(func(_ any, v any) bool {
candidate := v.(*phonyCandidate)
if candidate.frequency > 1 {
phonys = append(phonys, candidate)
if candidate.phony != nil {
phonys = append(phonys, candidate.phony)
}
return true
})
phonyBuildDefs := make([]*buildDef, len(phonys))
c.EventHandler.Do("name", func() {
// sorting for determinism
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].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,

28
context_test.go
View file

@ -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"),
"B": outputs("phony-0"),
"A": outputs("dedup-GKw-c0PwFokMUQ6T-TUmEWnZ4_VlQ2Qpgw-vCTT0-OQ"),
"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("phony-1", []string{"a", "c"}, nil)},
b("dedup--44g_C5MPySMYMOb1lLzwTRymLuXe4tNWQO4UFViBgM", []string{"a", "b"}, nil),
b("dedup-9F3lHN7zCZFVHkHogt17VAR5lkigoAdT9E_JZuYVP8E", []string{"a", "c"}, nil)},
conversions: map[string][]ninjaString{
"A": outputs("phony-0"),
"B": outputs("phony-0"),
"C": outputs("phony-1"),
"D": outputs("phony-1"),
"A": outputs("dedup--44g_C5MPySMYMOb1lLzwTRymLuXe4tNWQO4UFViBgM"),
"B": outputs("dedup--44g_C5MPySMYMOb1lLzwTRymLuXe4tNWQO4UFViBgM"),
"C": outputs("dedup-9F3lHN7zCZFVHkHogt17VAR5lkigoAdT9E_JZuYVP8E"),
"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)
}