From 4cc5290aace2c46f211a3ccbbd41afdce925fc8f Mon Sep 17 00:00:00 2001 From: Colin Cross Date: Thu, 28 Mar 2024 12:37:22 -0700 Subject: [PATCH 1/6] Use maps and slices packages Use slices.Clone, slices.Contains, maps.Clone and maps.Equal. Test: go test ./... Change-Id: I96596f157dec2558007da4917c998a64d0cc4d79 --- bootstrap/bpdoc/properties.go | 5 +++-- context.go | 31 ++++++++----------------------- glob.go | 9 +++++---- ninja_strings_test.go | 3 ++- pathtools/fs_test.go | 5 +++-- proptools/escape.go | 7 ++++--- proptools/escape_test.go | 3 ++- proptools/extend.go | 3 ++- 8 files changed, 29 insertions(+), 37 deletions(-) diff --git a/bootstrap/bpdoc/properties.go b/bootstrap/bpdoc/properties.go index 31b93b1..29e6c56 100644 --- a/bootstrap/bpdoc/properties.go +++ b/bootstrap/bpdoc/properties.go @@ -20,6 +20,7 @@ import ( "go/doc" "html/template" "reflect" + "slices" "strconv" "strings" "unicode" @@ -34,7 +35,7 @@ import ( func (ps *PropertyStruct) Clone() *PropertyStruct { ret := *ps - ret.Properties = append([]Property(nil), ret.Properties...) + ret.Properties = slices.Clone(ret.Properties) for i, prop := range ret.Properties { ret.Properties[i] = prop.Clone() } @@ -44,7 +45,7 @@ func (ps *PropertyStruct) Clone() *PropertyStruct { func (p *Property) Clone() Property { ret := *p - ret.Properties = append([]Property(nil), ret.Properties...) + ret.Properties = slices.Clone(ret.Properties) for i, prop := range ret.Properties { ret.Properties[i] = prop.Clone() } diff --git a/context.go b/context.go index 6ec1641..a64250d 100644 --- a/context.go +++ b/context.go @@ -24,6 +24,7 @@ import ( "hash/fnv" "io" "io/ioutil" + "maps" "os" "path/filepath" "reflect" @@ -418,15 +419,7 @@ type Variation struct { type variationMap map[string]string func (vm variationMap) clone() variationMap { - if vm == nil { - return nil - } - newVm := make(variationMap) - for k, v := range vm { - newVm[k] = v - } - - return newVm + return maps.Clone(vm) } // Compare this variationMap to another one. Returns true if the every entry in this map @@ -441,10 +434,7 @@ func (vm variationMap) subsetOf(other variationMap) bool { } func (vm variationMap) equal(other variationMap) bool { - if len(vm) != len(other) { - return false - } - return vm.subsetOf(other) + return maps.Equal(vm, other) } type singletonInfo struct { @@ -800,15 +790,10 @@ type transitionMutatorImpl struct { // Adds each argument in items to l if it's not already there. func addToStringListIfNotPresent(l []string, items ...string) []string { -OUTER: for _, i := range items { - for _, existing := range l { - if existing == i { - continue OUTER - } + if !slices.Contains(l, i) { + l = append(l, i) } - - l = append(l, i) } return l @@ -1746,14 +1731,14 @@ func (c *Context) createVariations(origModule *moduleInfo, mutatorName string, m := *origModule newModule := &m - newModule.directDeps = append([]depInfo(nil), origModule.directDeps...) + newModule.directDeps = slices.Clone(origModule.directDeps) newModule.reverseDeps = nil newModule.forwardDeps = nil newModule.logicModule = newLogicModule newModule.variant = newVariant(origModule, mutatorName, variationName, local) newModule.properties = newProperties - newModule.providers = append([]interface{}(nil), origModule.providers...) - newModule.providerInitialValueHashes = append([]uint64(nil), origModule.providerInitialValueHashes...) + newModule.providers = slices.Clone(origModule.providers) + newModule.providerInitialValueHashes = slices.Clone(origModule.providerInitialValueHashes) newModules = append(newModules, newModule) diff --git a/glob.go b/glob.go index 91ae723..8d0748a 100644 --- a/glob.go +++ b/glob.go @@ -16,6 +16,7 @@ package blueprint import ( "fmt" + "slices" "sort" "strings" @@ -40,7 +41,7 @@ func verifyGlob(key globKey, pattern string, excludes []string, g pathtools.Glob func (c *Context) glob(pattern string, excludes []string) ([]string, error) { // Sort excludes so that two globs with the same excludes in a different order reuse the same // key. Make a copy first to avoid modifying the caller's version. - excludes = append([]string(nil), excludes...) + excludes = slices.Clone(excludes) sort.Strings(excludes) key := globToKey(pattern, excludes) @@ -54,7 +55,7 @@ func (c *Context) glob(pattern string, excludes []string) ([]string, error) { // Glob has already been done, double check it is identical verifyGlob(key, pattern, excludes, g) // Return a copy so that modifications don't affect the cached value. - return append([]string(nil), g.Matches...), nil + return slices.Clone(g.Matches), nil } // Get a globbed file list @@ -74,11 +75,11 @@ func (c *Context) glob(pattern string, excludes []string) ([]string, error) { // Getting the list raced with another goroutine, throw away the results and use theirs verifyGlob(key, pattern, excludes, g) // Return a copy so that modifications don't affect the cached value. - return append([]string(nil), g.Matches...), nil + return slices.Clone(g.Matches), nil } // Return a copy so that modifications don't affect the cached value. - return append([]string(nil), result.Matches...), nil + return slices.Clone(result.Matches), nil } func (c *Context) Globs() pathtools.MultipleGlobResults { diff --git a/ninja_strings_test.go b/ninja_strings_test.go index 50a80fa..2789b7a 100644 --- a/ninja_strings_test.go +++ b/ninja_strings_test.go @@ -16,6 +16,7 @@ package blueprint import ( "reflect" + "slices" "strconv" "strings" "testing" @@ -271,7 +272,7 @@ func Test_parseNinjaOrSimpleStrings(t *testing.T) { for _, tt := range testCases { t.Run(tt.name, func(t *testing.T) { - inCopy := append([]string(nil), tt.in...) + inCopy := slices.Clone(tt.in) scope := newLocalScope(nil, "") scope.AddLocalVariable("abc", "abc") diff --git a/pathtools/fs_test.go b/pathtools/fs_test.go index 3b4d4d0..8826c7c 100644 --- a/pathtools/fs_test.go +++ b/pathtools/fs_test.go @@ -18,6 +18,7 @@ import ( "os" "path/filepath" "reflect" + "slices" "syscall" "testing" ) @@ -233,7 +234,7 @@ func TestFs_ListDirsRecursiveFollowSymlinks(t *testing.T) { t.Run(test.name, func(t *testing.T) { got, err := fs.ListDirsRecursive(filepath.Join(dir, test.name), FollowSymlinks) checkErr(t, test.err, err) - want := append([]string(nil), test.dirs...) + want := slices.Clone(test.dirs) for i := range want { want[i] = filepath.Join(dir, want[i]) } @@ -279,7 +280,7 @@ func TestFs_ListDirsRecursiveDontFollowSymlinks(t *testing.T) { t.Run(test.name, func(t *testing.T) { got, err := fs.ListDirsRecursive(filepath.Join(dir, test.name), DontFollowSymlinks) checkErr(t, test.err, err) - want := append([]string(nil), test.dirs...) + want := slices.Clone(test.dirs) for i := range want { want[i] = filepath.Join(dir, want[i]) } diff --git a/proptools/escape.go b/proptools/escape.go index 9792444..8fcf82e 100644 --- a/proptools/escape.go +++ b/proptools/escape.go @@ -15,6 +15,7 @@ package proptools import ( + "slices" "strings" "unsafe" ) @@ -33,7 +34,7 @@ func NinjaEscapeList(slice []string) []string { if !sliceCopied { // If this was the first string that was modified by escaping then make a copy of the // input slice to use as the output slice. - slice = append([]string(nil), slice...) + slice = slices.Clone(slice) sliceCopied = true } slice[i] = escaped @@ -66,7 +67,7 @@ func ShellEscapeList(slice []string) []string { if !sliceCopied { // If this was the first string that was modified by escaping then make a copy of the // input slice to use as the output slice. - slice = append([]string(nil), slice...) + slice = slices.Clone(slice) sliceCopied = true } slice[i] = escaped @@ -83,7 +84,7 @@ func ShellEscapeListIncludingSpaces(slice []string) []string { if !sliceCopied { // If this was the first string that was modified by escaping then make a copy of the // input slice to use as the output slice. - slice = append([]string(nil), slice...) + slice = slices.Clone(slice) sliceCopied = true } slice[i] = escaped diff --git a/proptools/escape_test.go b/proptools/escape_test.go index 2937058..91c9e76 100644 --- a/proptools/escape_test.go +++ b/proptools/escape_test.go @@ -18,6 +18,7 @@ import ( "bytes" "os/exec" "reflect" + "slices" "testing" "unsafe" ) @@ -235,7 +236,7 @@ func TestNinjaEscapeList(t *testing.T) { t.Run(tf.name, func(t *testing.T) { for _, tt := range testCases { t.Run(tt.name, func(t *testing.T) { - inCopy := append([]string(nil), tt.in...) + inCopy := slices.Clone(tt.in) got := tf.f(tt.in) diff --git a/proptools/extend.go b/proptools/extend.go index eec5d43..af4e185 100644 --- a/proptools/extend.go +++ b/proptools/extend.go @@ -17,6 +17,7 @@ package proptools import ( "fmt" "reflect" + "slices" "strings" ) @@ -317,7 +318,7 @@ func extendPropertiesRecursive(dstValues []reflect.Value, srcValue reflect.Value // of destinations to consider. Make a copy of dstValues if necessary // to avoid modifying the backing array of an input parameter. if !dstValuesCopied { - dstValues = append([]reflect.Value(nil), dstValues...) + dstValues = slices.Clone(dstValues) dstValuesCopied = true } dstValues = append(dstValues, embeddedDstValue) From fca423d9e637bccd48938e93c131ba83880aab0a Mon Sep 17 00:00:00 2001 From: Colin Cross Date: Thu, 28 Mar 2024 13:21:54 -0700 Subject: [PATCH 2/6] Cache outgoing transitions TransitionMutators were calling OutgoingTransition and IncomingTransition twice, once to determine all the necessary variations for each module, and then again when creating the variations to resolve dependencies. Store the final variation needed for each dependency during the first computation, and use it to resolve the dependency variation in the second pass. Bug: 319288033 Test: all soong tests Change-Id: I2e42c0d69efbfcff915267c484c18554b857e0b6 --- context.go | 64 +++++++++++++++++++++++---------------------------- module_ctx.go | 10 +++----- 2 files changed, 32 insertions(+), 42 deletions(-) diff --git a/context.go b/context.go index a64250d..26c2ea2 100644 --- a/context.go +++ b/context.go @@ -351,6 +351,10 @@ type moduleInfo struct { currentTransitionMutator string requiredVariationsLock sync.Mutex + // outgoingTransitionCache stores the final variation for each dependency, indexed by the source variation + // index in transitionVariations and then by the index of the dependency in directDeps + outgoingTransitionCache [][]string + // set during PrepareBuildActions actionDefs localBuildActions @@ -828,12 +832,17 @@ func (t *transitionMutatorImpl) topDownMutator(mctx TopDownMutatorContext) { module.transitionVariations = addToStringListIfNotPresent(module.transitionVariations, mutatorSplits...) sort.Strings(module.transitionVariations) - for _, srcVariation := range module.transitionVariations { - for _, dep := range module.directDeps { + outgoingTransitionCache := make([][]string, len(module.transitionVariations)) + for srcVariationIndex, srcVariation := range module.transitionVariations { + srcVariationTransitionCache := make([]string, len(module.directDeps)) + for depIndex, dep := range module.directDeps { finalVariation := t.transition(mctx)(mctx.Module(), srcVariation, dep.module.logicModule, dep.tag) + srcVariationTransitionCache[depIndex] = finalVariation t.addRequiredVariation(dep.module, finalVariation) } + outgoingTransitionCache[srcVariationIndex] = srcVariationTransitionCache } + module.outgoingTransitionCache = outgoingTransitionCache } type transitionContextImpl struct { @@ -882,7 +891,9 @@ func (t *transitionMutatorImpl) bottomUpMutator(mctx BottomUpMutatorContext) { // only time interaction between multiple modules is required is during the // computation of the variations required by a given module. variations := mc.module.transitionVariations + outgoingTransitionCache := mc.module.outgoingTransitionCache mc.module.transitionVariations = nil + mc.module.outgoingTransitionCache = nil mc.module.currentTransitionMutator = "" if len(variations) < 1 { @@ -892,9 +903,10 @@ func (t *transitionMutatorImpl) bottomUpMutator(mctx BottomUpMutatorContext) { if len(variations) == 1 && variations[0] == "" { // Module is not split, just apply the transition - mc.applyTransition(t.transition(mctx)) + mc.context.convertDepsToVariation(mc.module, 0, + chooseDepByIndexes(mc.name, outgoingTransitionCache)) } else { - mc.createVariationsWithTransition(t.transition(mctx), variations...) + mc.createVariationsWithTransition(variations, outgoingTransitionCache) } } @@ -1742,7 +1754,7 @@ func (c *Context) createVariations(origModule *moduleInfo, mutatorName string, newModules = append(newModules, newModule) - newErrs := c.convertDepsToVariation(newModule, depChooser) + newErrs := c.convertDepsToVariation(newModule, i, depChooser) if len(newErrs) > 0 { errs = append(errs, newErrs...) } @@ -1758,38 +1770,13 @@ func (c *Context) createVariations(origModule *moduleInfo, mutatorName string, return newModules, errs } -type depChooser func(source *moduleInfo, dep depInfo) (*moduleInfo, string) +type depChooser func(source *moduleInfo, variationIndex, depIndex int, dep depInfo) (*moduleInfo, string) // This function is called for every dependency edge to determine which // variation of the dependency is needed. Its inputs are the depending module, // its variation, the dependency and the dependency tag. type Transition func(source Module, sourceVariation string, dep Module, depTag DependencyTag) string -func chooseDepByTransition(mutatorName string, transition Transition) depChooser { - return func(source *moduleInfo, dep depInfo) (*moduleInfo, string) { - sourceVariation := source.variant.variations[mutatorName] - depLogicModule := dep.module.logicModule - if depLogicModule == nil { - // This is really a lie because the original dependency before the split - // went away when it was split. We choose an arbitrary split module - // instead and hope that whatever information the transition wants from it - // is the same as in the original one - // TODO(lberki): this can be fixed by calling transition() once and saving - // its results somewhere - depLogicModule = dep.module.splitModules[0].moduleOrAliasTarget().logicModule - } - - desiredVariation := transition(source.logicModule, sourceVariation, depLogicModule, dep.tag) - for _, m := range dep.module.splitModules { - if m.moduleOrAliasVariant().variations[mutatorName] == desiredVariation { - return m.moduleOrAliasTarget(), "" - } - } - - return nil, desiredVariation - } -} - func chooseDep(candidates modulesOrAliases, mutatorName, variationName string, defaultVariationName *string) (*moduleInfo, string) { for _, m := range candidates { if m.moduleOrAliasVariant().variations[mutatorName] == variationName { @@ -1809,24 +1796,31 @@ func chooseDep(candidates modulesOrAliases, mutatorName, variationName string, d return nil, variationName } +func chooseDepByIndexes(mutatorName string, variations [][]string) depChooser { + return func(source *moduleInfo, variationIndex, depIndex int, dep depInfo) (*moduleInfo, string) { + desiredVariation := variations[variationIndex][depIndex] + return chooseDep(dep.module.splitModules, mutatorName, desiredVariation, nil) + } +} + func chooseDepExplicit(mutatorName string, variationName string, defaultVariationName *string) depChooser { - return func(source *moduleInfo, dep depInfo) (*moduleInfo, string) { + return func(source *moduleInfo, variationIndex, depIndex int, dep depInfo) (*moduleInfo, string) { return chooseDep(dep.module.splitModules, mutatorName, variationName, defaultVariationName) } } func chooseDepInherit(mutatorName string, defaultVariationName *string) depChooser { - return func(source *moduleInfo, dep depInfo) (*moduleInfo, string) { + return func(source *moduleInfo, variationIndex, depIndex int, dep depInfo) (*moduleInfo, string) { sourceVariation := source.variant.variations[mutatorName] return chooseDep(dep.module.splitModules, mutatorName, sourceVariation, defaultVariationName) } } -func (c *Context) convertDepsToVariation(module *moduleInfo, depChooser depChooser) (errs []error) { +func (c *Context) convertDepsToVariation(module *moduleInfo, variationIndex int, depChooser depChooser) (errs []error) { for i, dep := range module.directDeps { if dep.module.logicModule == nil { - newDep, missingVariation := depChooser(module, dep) + newDep, missingVariation := depChooser(module, variationIndex, i, dep) if newDep == nil { errs = append(errs, &BlueprintError{ Err: fmt.Errorf("failed to find variation %q for module %q needed by %q", diff --git a/module_ctx.go b/module_ctx.go index cb86f43..a8f68b8 100644 --- a/module_ctx.go +++ b/module_ctx.go @@ -1004,8 +1004,8 @@ func (mctx *mutatorContext) CreateVariations(variationNames ...string) []Module return mctx.createVariations(variationNames, depChooser, false) } -func (mctx *mutatorContext) createVariationsWithTransition(transition Transition, variationNames ...string) []Module { - return mctx.createVariations(variationNames, chooseDepByTransition(mctx.name, transition), false) +func (mctx *mutatorContext) createVariationsWithTransition(variationNames []string, outgoingTransitions [][]string) []Module { + return mctx.createVariations(variationNames, chooseDepByIndexes(mctx.name, outgoingTransitions), false) } func (mctx *mutatorContext) CreateLocalVariations(variationNames ...string) []Module { @@ -1106,12 +1106,8 @@ func (mctx *mutatorContext) CreateAliasVariation(aliasVariationName, targetVaria panic(fmt.Errorf("no %q variation in module variations %q", targetVariationName, foundVariations)) } -func (mctx *mutatorContext) applyTransition(transition Transition) { - mctx.context.convertDepsToVariation(mctx.module, chooseDepByTransition(mctx.name, transition)) -} - func (mctx *mutatorContext) SetDependencyVariation(variationName string) { - mctx.context.convertDepsToVariation(mctx.module, chooseDepExplicit( + mctx.context.convertDepsToVariation(mctx.module, 0, chooseDepExplicit( mctx.name, variationName, nil)) } From fab4866a685334a2450dc92183dcfc113f72743e Mon Sep 17 00:00:00 2001 From: Colin Cross Date: Thu, 28 Mar 2024 12:54:16 -0700 Subject: [PATCH 3/6] Add Provider to transition contexts Allow TransitionMutator OutgoingTransition and IncomingTransition methods to access Providers from the current and dependency module respectively. Bug: 319288033 Test: none Change-Id: If16ee7980ee0b8f4abaf3c112738fca3f13ab61c --- context.go | 56 +++++++++++++++++++++++++++++++++++++++++---------- module_ctx.go | 6 ++++++ 2 files changed, 51 insertions(+), 11 deletions(-) diff --git a/context.go b/context.go index 26c2ea2..6d539f3 100644 --- a/context.go +++ b/context.go @@ -688,6 +688,14 @@ type IncomingTransitionContext interface { // Config returns the config object that was passed to // Context.PrepareBuildActions. Config() interface{} + + // Provider returns the value for a provider for the target of the dependency edge for which the + // transition is being computed. If the value is not set it returns nil and false. It panics if + // called before the appropriate mutator or GenerateBuildActions pass for the provider. The value + // returned may be a deep copy of the value originally passed to SetProvider. + // + // This method shouldn't be used directly, prefer the type-safe android.ModuleProvider instead. + Provider(provider AnyProviderKey) (any, bool) } type OutgoingTransitionContext interface { @@ -702,6 +710,14 @@ type OutgoingTransitionContext interface { // Config returns the config object that was passed to // Context.PrepareBuildActions. Config() interface{} + + // Provider returns the value for a provider for the source of the dependency edge for which the + // transition is being computed. If the value is not set it returns nil and false. It panics if + // called before the appropriate mutator or GenerateBuildActions pass for the provider. The value + // returned may be a deep copy of the value originally passed to SetProvider. + // + // This method shouldn't be used directly, prefer the type-safe android.ModuleProvider instead. + Provider(provider AnyProviderKey) (any, bool) } // TransitionMutator implements a top-down mechanism where a module tells its @@ -836,7 +852,7 @@ func (t *transitionMutatorImpl) topDownMutator(mctx TopDownMutatorContext) { for srcVariationIndex, srcVariation := range module.transitionVariations { srcVariationTransitionCache := make([]string, len(module.directDeps)) for depIndex, dep := range module.directDeps { - finalVariation := t.transition(mctx)(mctx.Module(), srcVariation, dep.module.logicModule, dep.tag) + finalVariation := t.transition(mctx)(mctx.moduleInfo(), srcVariation, dep.module, dep.tag) srcVariationTransitionCache[depIndex] = finalVariation t.addRequiredVariation(dep.module, finalVariation) } @@ -846,10 +862,11 @@ func (t *transitionMutatorImpl) topDownMutator(mctx TopDownMutatorContext) { } type transitionContextImpl struct { - source Module - dep Module - depTag DependencyTag - config interface{} + context *Context + source *moduleInfo + dep *moduleInfo + depTag DependencyTag + config interface{} } func (c *transitionContextImpl) DepTag() DependencyTag { @@ -865,7 +882,11 @@ type outgoingTransitionContextImpl struct { } func (c *outgoingTransitionContextImpl) Module() Module { - return c.source + return c.source.logicModule +} + +func (c *outgoingTransitionContextImpl) Provider(provider AnyProviderKey) (any, bool) { + return c.context.provider(c.source, provider.provider()) } type incomingTransitionContextImpl struct { @@ -873,13 +894,26 @@ type incomingTransitionContextImpl struct { } func (c *incomingTransitionContextImpl) Module() Module { - return c.dep + return c.dep.logicModule } -func (t *transitionMutatorImpl) transition(mctx BaseMutatorContext) Transition { - return func(source Module, sourceVariation string, dep Module, depTag DependencyTag) string { - tc := transitionContextImpl{source: source, dep: dep, depTag: depTag, config: mctx.Config()} +func (c *incomingTransitionContextImpl) Provider(provider AnyProviderKey) (any, bool) { + return c.context.provider(c.dep, provider.provider()) +} + +func (t *transitionMutatorImpl) transition(mctx BaseModuleContext) Transition { + return func(source *moduleInfo, sourceVariation string, dep *moduleInfo, depTag DependencyTag) string { + tc := transitionContextImpl{ + context: mctx.base().context, + source: source, + dep: dep, + depTag: depTag, + config: mctx.Config(), + } outgoingVariation := t.mutator.OutgoingTransition(&outgoingTransitionContextImpl{tc}, sourceVariation) + if mctx.Failed() { + return outgoingVariation + } finalVariation := t.mutator.IncomingTransition(&incomingTransitionContextImpl{tc}, outgoingVariation) return finalVariation } @@ -1775,7 +1809,7 @@ type depChooser func(source *moduleInfo, variationIndex, depIndex int, dep depIn // This function is called for every dependency edge to determine which // variation of the dependency is needed. Its inputs are the depending module, // its variation, the dependency and the dependency tag. -type Transition func(source Module, sourceVariation string, dep Module, depTag DependencyTag) string +type Transition func(source *moduleInfo, sourceVariation string, dep *moduleInfo, depTag DependencyTag) string func chooseDep(candidates modulesOrAliases, mutatorName, variationName string, defaultVariationName *string) (*moduleInfo, string) { for _, m := range candidates { diff --git a/module_ctx.go b/module_ctx.go index a8f68b8..d71f053 100644 --- a/module_ctx.go +++ b/module_ctx.go @@ -357,6 +357,8 @@ type BaseModuleContext interface { SetProvider(provider AnyProviderKey, value any) EarlyGetMissingDependencies() []string + + base() *baseModuleContext } type DynamicDependerModuleContext BottomUpMutatorContext @@ -763,6 +765,10 @@ func (m *baseModuleContext) ModuleFactories() map[string]ModuleFactory { return ret } +func (m *baseModuleContext) base() *baseModuleContext { + return m +} + func (m *moduleContext) ModuleSubDir() string { return m.module.variant.name } From 7b5888a4f64ac157e22679b0e258aa5f659dbf88 Mon Sep 17 00:00:00 2001 From: Colin Cross Date: Thu, 28 Mar 2024 13:40:53 -0700 Subject: [PATCH 4/6] Keep logicModule for obsolete variants When splitting a module into variants the original *moduleInfo was left in place but with logicModule set to nil as a marker that the variant was obsolete and any incoming dependencies needed to be resolved onto one of the newly split variants. A change to allow TransitionMutator IncomingTransition calls when adding dependencies later will require keeping the obsolete *moduleInfo and logicModule around so that IncomingTransition can be called on it. so use an explicit boolean to mark the module as obsolete instead of clearing logicModule. Bug: 319288033 Test: go test ./... Change-Id: I5dc201f43442aa07ac1b858240c675bab3782b55 --- context.go | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/context.go b/context.go index 6d539f3..c748545 100644 --- a/context.go +++ b/context.go @@ -344,7 +344,8 @@ type moduleInfo struct { waitingCount int // set during each runMutator - splitModules modulesOrAliases + splitModules modulesOrAliases + obsoletedByNewVariants bool // Used by TransitionMutator implementations transitionVariations []string @@ -1796,7 +1797,7 @@ func (c *Context) createVariations(origModule *moduleInfo, mutatorName string, // Mark original variant as invalid. Modules that depend on this module will still // depend on origModule, but we'll fix it when the mutator is called on them. - origModule.logicModule = nil + origModule.obsoletedByNewVariants = true origModule.splitModules = newModules atomic.AddUint32(&c.depsModified, 1) @@ -1853,7 +1854,7 @@ func chooseDepInherit(mutatorName string, defaultVariationName *string) depChoos func (c *Context) convertDepsToVariation(module *moduleInfo, variationIndex int, depChooser depChooser) (errs []error) { for i, dep := range module.directDeps { - if dep.module.logicModule == nil { + if dep.module.obsoletedByNewVariants { newDep, missingVariation := depChooser(module, variationIndex, i, dep) if newDep == nil { errs = append(errs, &BlueprintError{ @@ -3218,12 +3219,12 @@ func (c *Context) runMutator(config interface{}, mutator *mutatorInfo, // Fix up any remaining dependencies on modules that were split into variants // by replacing them with the first variant for j, dep := range module.directDeps { - if dep.module.logicModule == nil { + if dep.module.obsoletedByNewVariants { module.directDeps[j].module = dep.module.splitModules.firstModule() } } - if module.createdBy != nil && module.createdBy.logicModule == nil { + if module.createdBy != nil && module.createdBy.obsoletedByNewVariants { module.createdBy = module.createdBy.splitModules.firstModule() } @@ -3248,7 +3249,7 @@ func (c *Context) runMutator(config interface{}, mutator *mutatorInfo, // change inside the loop for i := 0; i < len(group.modules); i++ { if alias := group.modules[i].alias(); alias != nil { - if alias.target.logicModule == nil { + if alias.target.obsoletedByNewVariants { newTarget := findAliasTarget(alias.target.variant) if newTarget != nil { alias.target = newTarget From 1d5e1a56febc27a5f204864b27464b4027d891d9 Mon Sep 17 00:00:00 2001 From: Colin Cross Date: Fri, 29 Mar 2024 16:00:42 -0700 Subject: [PATCH 5/6] Explicitly delete old logicModule entries from Context.moduleInfo Creating variants has an optimization to reuse the logicModule of the original variant as the logicModule of the first new variant. Using the same logicModule meant that updating the Context.moduleInfo map from logicModules to *moduleInfo would overwrite the old entry for the original variant with the new entry for the first variant. TransitionMutators will need to keep the original logicModule for later use, which will require disabling the optimization that reuses it. When the first variant is added to the map it no longer overwrites the original variant. Excplicitly track the original logicModule and remove it from the map if necessary. Bug: 319288033 Test: go test ./... Change-Id: I05ffdf6d7ce60508f6d9e9657c21032f2c4f5d9c --- context.go | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/context.go b/context.go index c748545..92a6962 100644 --- a/context.go +++ b/context.go @@ -3086,6 +3086,11 @@ func (c *Context) runMutator(config interface{}, mutator *mutatorInfo, deps []string } + type newVariationPair struct { + newVariations modulesOrAliases + origLogicModule Module + } + reverseDeps := make(map[*moduleInfo][]depInfo) var rename []rename var replace []replace @@ -3093,7 +3098,7 @@ func (c *Context) runMutator(config interface{}, mutator *mutatorInfo, errsCh := make(chan []error) globalStateCh := make(chan globalStateChange) - newVariationsCh := make(chan modulesOrAliases) + newVariationsCh := make(chan newVariationPair) done := make(chan bool) c.depsModified = 0 @@ -3113,6 +3118,8 @@ func (c *Context) runMutator(config interface{}, mutator *mutatorInfo, pauseCh: pause, } + origLogicModule := module.logicModule + module.startedMutator = mutator func() { @@ -3138,7 +3145,7 @@ func (c *Context) runMutator(config interface{}, mutator *mutatorInfo, } if len(mctx.newVariations) > 0 { - newVariationsCh <- mctx.newVariations + newVariationsCh <- newVariationPair{mctx.newVariations, origLogicModule} } if len(mctx.reverseDeps) > 0 || len(mctx.replace) > 0 || len(mctx.rename) > 0 || len(mctx.newModules) > 0 || len(mctx.ninjaFileDeps) > 0 { @@ -3154,6 +3161,8 @@ func (c *Context) runMutator(config interface{}, mutator *mutatorInfo, return false } + var obsoleteLogicModules []Module + // Process errs and reverseDeps in a single goroutine go func() { for { @@ -3169,7 +3178,10 @@ func (c *Context) runMutator(config interface{}, mutator *mutatorInfo, newModules = append(newModules, globalStateChange.newModules...) deps = append(deps, globalStateChange.deps...) case newVariations := <-newVariationsCh: - for _, moduleOrAlias := range newVariations { + if newVariations.origLogicModule != newVariations.newVariations[0].module().logicModule { + obsoleteLogicModules = append(obsoleteLogicModules, newVariations.origLogicModule) + } + for _, moduleOrAlias := range newVariations.newVariations { if m := moduleOrAlias.module(); m != nil { newModuleInfo[m.logicModule] = m } @@ -3201,6 +3213,10 @@ func (c *Context) runMutator(config interface{}, mutator *mutatorInfo, return nil, errs } + for _, obsoleteLogicModule := range obsoleteLogicModules { + delete(newModuleInfo, obsoleteLogicModule) + } + c.moduleInfo = newModuleInfo for _, group := range c.moduleGroups { From d5d55e5b260c59bfad2aa1d1ebb6b3e1d92624f6 Mon Sep 17 00:00:00 2001 From: Colin Cross Date: Fri, 29 Mar 2024 12:06:25 -0700 Subject: [PATCH 6/6] Store mutator in mutatorContext instead of name TransitionMutators will need access to the original *mutatorInfo, store it in mutatorContext instead of the name, and update all accesses to the name to go through the *mutatorInfo. Bug: 319288033 Test: go test ./... Change-Id: I4bb1a17f44e55b23dd70708c9a5fde2ae80ce154 --- context.go | 10 +++++----- module_ctx.go | 24 ++++++++++++------------ 2 files changed, 17 insertions(+), 17 deletions(-) diff --git a/context.go b/context.go index 92a6962..730894a 100644 --- a/context.go +++ b/context.go @@ -939,7 +939,7 @@ func (t *transitionMutatorImpl) bottomUpMutator(mctx BottomUpMutatorContext) { if len(variations) == 1 && variations[0] == "" { // Module is not split, just apply the transition mc.context.convertDepsToVariation(mc.module, 0, - chooseDepByIndexes(mc.name, outgoingTransitionCache)) + chooseDepByIndexes(mc.mutator.name, outgoingTransitionCache)) } else { mc.createVariationsWithTransition(variations, outgoingTransitionCache) } @@ -1751,12 +1751,12 @@ func newVariant(module *moduleInfo, mutatorName string, variationName string, return variant{newVariantName, newVariations, newDependencyVariations} } -func (c *Context) createVariations(origModule *moduleInfo, mutatorName string, +func (c *Context) createVariations(origModule *moduleInfo, mutator *mutatorInfo, depChooser depChooser, variationNames []string, local bool) (modulesOrAliases, []error) { if len(variationNames) == 0 { panic(fmt.Errorf("mutator %q passed zero-length variation list for module %q", - mutatorName, origModule.Name())) + mutator.name, origModule.Name())) } var newModules modulesOrAliases @@ -1782,7 +1782,7 @@ func (c *Context) createVariations(origModule *moduleInfo, mutatorName string, newModule.reverseDeps = nil newModule.forwardDeps = nil newModule.logicModule = newLogicModule - newModule.variant = newVariant(origModule, mutatorName, variationName, local) + newModule.variant = newVariant(origModule, mutator.name, variationName, local) newModule.properties = newProperties newModule.providers = slices.Clone(origModule.providers) newModule.providerInitialValueHashes = slices.Clone(origModule.providerInitialValueHashes) @@ -3114,7 +3114,7 @@ func (c *Context) runMutator(config interface{}, mutator *mutatorInfo, config: config, module: module, }, - name: mutator.name, + mutator: mutator, pauseCh: pause, } diff --git a/module_ctx.go b/module_ctx.go index d71f053..4fc949b 100644 --- a/module_ctx.go +++ b/module_ctx.go @@ -825,7 +825,7 @@ func (m *baseModuleContext) EarlyGetMissingDependencies() []string { type mutatorContext struct { baseModuleContext - name string + mutator *mutatorInfo reverseDeps []reverseDep rename []rename replace []replace @@ -1002,20 +1002,20 @@ func (BaseDependencyTag) dependencyTag(DependencyTag) { var _ DependencyTag = BaseDependencyTag{} func (mctx *mutatorContext) MutatorName() string { - return mctx.name + return mctx.mutator.name } func (mctx *mutatorContext) CreateVariations(variationNames ...string) []Module { - depChooser := chooseDepInherit(mctx.name, mctx.defaultVariation) + depChooser := chooseDepInherit(mctx.mutator.name, mctx.defaultVariation) return mctx.createVariations(variationNames, depChooser, false) } func (mctx *mutatorContext) createVariationsWithTransition(variationNames []string, outgoingTransitions [][]string) []Module { - return mctx.createVariations(variationNames, chooseDepByIndexes(mctx.name, outgoingTransitions), false) + return mctx.createVariations(variationNames, chooseDepByIndexes(mctx.mutator.name, outgoingTransitions), false) } func (mctx *mutatorContext) CreateLocalVariations(variationNames ...string) []Module { - depChooser := chooseDepInherit(mctx.name, mctx.defaultVariation) + depChooser := chooseDepInherit(mctx.mutator.name, mctx.defaultVariation) return mctx.createVariations(variationNames, depChooser, true) } @@ -1031,7 +1031,7 @@ func (mctx *mutatorContext) SetVariationProvider(module Module, provider AnyProv func (mctx *mutatorContext) createVariations(variationNames []string, depChooser depChooser, local bool) []Module { var ret []Module - modules, errs := mctx.context.createVariations(mctx.module, mctx.name, depChooser, variationNames, local) + modules, errs := mctx.context.createVariations(mctx.module, mctx.mutator, depChooser, variationNames, local) if len(errs) > 0 { mctx.errs = append(mctx.errs, errs...) } @@ -1062,7 +1062,7 @@ func (mctx *mutatorContext) AliasVariation(variationName string) { } for _, variant := range mctx.newVariations { - if variant.moduleOrAliasVariant().variations[mctx.name] == variationName { + if variant.moduleOrAliasVariant().variations[mctx.mutator.name] == variationName { alias := &moduleAlias{ variant: mctx.module.variant, target: variant.moduleOrAliasTarget(), @@ -1076,13 +1076,13 @@ func (mctx *mutatorContext) AliasVariation(variationName string) { var foundVariations []string for _, variant := range mctx.newVariations { - foundVariations = append(foundVariations, variant.moduleOrAliasVariant().variations[mctx.name]) + foundVariations = append(foundVariations, variant.moduleOrAliasVariant().variations[mctx.mutator.name]) } panic(fmt.Errorf("no %q variation in module variations %q", variationName, foundVariations)) } func (mctx *mutatorContext) CreateAliasVariation(aliasVariationName, targetVariationName string) { - newVariant := newVariant(mctx.module, mctx.name, aliasVariationName, false) + newVariant := newVariant(mctx.module, mctx.mutator.name, aliasVariationName, false) for _, moduleOrAlias := range mctx.module.splitModules { if moduleOrAlias.moduleOrAliasVariant().variations.equal(newVariant.variations) { @@ -1095,7 +1095,7 @@ func (mctx *mutatorContext) CreateAliasVariation(aliasVariationName, targetVaria } for _, variant := range mctx.newVariations { - if variant.moduleOrAliasVariant().variations[mctx.name] == targetVariationName { + if variant.moduleOrAliasVariant().variations[mctx.mutator.name] == targetVariationName { // Append the alias here so that it comes after any aliases created by AliasVariation. mctx.module.splitModules = append(mctx.module.splitModules, &moduleAlias{ variant: newVariant, @@ -1107,14 +1107,14 @@ func (mctx *mutatorContext) CreateAliasVariation(aliasVariationName, targetVaria var foundVariations []string for _, variant := range mctx.newVariations { - foundVariations = append(foundVariations, variant.moduleOrAliasVariant().variations[mctx.name]) + foundVariations = append(foundVariations, variant.moduleOrAliasVariant().variations[mctx.mutator.name]) } panic(fmt.Errorf("no %q variation in module variations %q", targetVariationName, foundVariations)) } func (mctx *mutatorContext) SetDependencyVariation(variationName string) { mctx.context.convertDepsToVariation(mctx.module, 0, chooseDepExplicit( - mctx.name, variationName, nil)) + mctx.mutator.name, variationName, nil)) } func (mctx *mutatorContext) SetDefaultDependencyVariation(variationName *string) {