From 83605c630554f83f49fa6c593f91f22e3a6e8e98 Mon Sep 17 00:00:00 2001 From: Binu Philip Date: Mon, 1 Jun 2026 14:03:21 -0700 Subject: [PATCH] perf(comp list): skip lock file reads (non-mutating) Component resolution was doing thousands of unnecessary lock file reads for commands that do not consume lock data. On large repos checked out on slow filesystems this dominated runtime for read-only workflows. This change addresses that through a user override to not read component lock file information available only for list action. - Introduce LockMode in component filtering/resolution to make lock behavior explicit. - Enforce the invariant that lock population can only be skipped when validation is also skipped. - Keep update/build-style flows on skip-validation+populate behavior, while allowing skip-both only for safe read-only paths. - Make lock-population skipping a deliberate CLI choice for 'component list' via --skip-lock-population. - Keep 'component changed' on skip-both since it reads lock data from git commits directly. Performance impact (measured on Azure Linux repo with ~7k components): - 'component list -a' reduced from ~38s to ~8s (lock reads were the dominant I/O cost at ~30s) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../reference/cli/azldev_component_build.md | 1 - .../cli/azldev_component_diff-sources.md | 1 - .../reference/cli/azldev_component_list.md | 1 + .../cli/azldev_component_prepare-sources.md | 1 - .../reference/cli/azldev_component_query.md | 1 - .../reference/cli/azldev_component_render.md | 1 - internal/app/azldev/cmds/component/changed.go | 8 ++- internal/app/azldev/cmds/component/list.go | 25 +++++--- internal/app/azldev/cmds/component/update.go | 10 ++-- internal/app/azldev/core/components/filter.go | 42 +++++++++++--- .../app/azldev/core/components/resolver.go | 58 +++++++++++++++---- .../azldev/core/components/resolver_test.go | 48 +++++++++++++-- internal/projectconfig/component.go | 4 +- 13 files changed, 155 insertions(+), 46 deletions(-) diff --git a/docs/user/reference/cli/azldev_component_build.md b/docs/user/reference/cli/azldev_component_build.md index 2086f683..16a30f7f 100644 --- a/docs/user/reference/cli/azldev_component_build.md +++ b/docs/user/reference/cli/azldev_component_build.md @@ -57,7 +57,6 @@ azldev component build [flags] --mock-config-opt stringToString Pass a configuration option through to mock (key=value, can be specified multiple times) (default []) --no-check Skip package %check tests --preserve-buildenv policy Preserve build environment {on-failure, always, never} (default on-failure) - --skip-lock-validation skip lock file consistency checks -s, --spec-path stringArray Spec path --srpm-only Build SRPM (source RPM) *only* --without-git Skip creating a dist-git repository with synthetic commit history diff --git a/docs/user/reference/cli/azldev_component_diff-sources.md b/docs/user/reference/cli/azldev_component_diff-sources.md index afd471ac..34969640 100644 --- a/docs/user/reference/cli/azldev_component_diff-sources.md +++ b/docs/user/reference/cli/azldev_component_diff-sources.md @@ -22,7 +22,6 @@ azldev component diff-sources [flags] -g, --component-group stringArray Component group name -h, --help help for diff-sources --output-file string write the diff output to a file instead of stdout - --skip-lock-validation skip lock file consistency checks -s, --spec-path stringArray Spec path ``` diff --git a/docs/user/reference/cli/azldev_component_list.md b/docs/user/reference/cli/azldev_component_list.md index 2900def0..ab8dc5d9 100644 --- a/docs/user/reference/cli/azldev_component_list.md +++ b/docs/user/reference/cli/azldev_component_list.md @@ -38,6 +38,7 @@ azldev component list [flags] -p, --component stringArray Component name pattern -g, --component-group stringArray Component group name -h, --help help for list + --skip-lock-population skip lock file population (default: true; set to false to read lock data) (default true) -s, --spec-path stringArray Spec path ``` diff --git a/docs/user/reference/cli/azldev_component_prepare-sources.md b/docs/user/reference/cli/azldev_component_prepare-sources.md index 590286e7..7055caa9 100644 --- a/docs/user/reference/cli/azldev_component_prepare-sources.md +++ b/docs/user/reference/cli/azldev_component_prepare-sources.md @@ -39,7 +39,6 @@ azldev component prepare-sources [flags] --force delete and recreate the output directory if it already exists -h, --help help for prepare-sources -o, --output-dir string output directory - --skip-lock-validation skip lock file consistency checks --skip-overlays skip applying overlays to prepared sources --skip-sources skip downloading fetched sources when preparing the package (useful to extract dist-git metadata when source files are not needed) -s, --spec-path stringArray Spec path diff --git a/docs/user/reference/cli/azldev_component_query.md b/docs/user/reference/cli/azldev_component_query.md index 03a49fe1..26bd9750 100644 --- a/docs/user/reference/cli/azldev_component_query.md +++ b/docs/user/reference/cli/azldev_component_query.md @@ -34,7 +34,6 @@ azldev component query [flags] -p, --component stringArray Component name pattern -g, --component-group stringArray Component group name -h, --help help for query - --skip-lock-validation skip lock file consistency checks -s, --spec-path stringArray Spec path ``` diff --git a/docs/user/reference/cli/azldev_component_render.md b/docs/user/reference/cli/azldev_component_render.md index 1ded12de..297ed8f3 100644 --- a/docs/user/reference/cli/azldev_component_render.md +++ b/docs/user/reference/cli/azldev_component_render.md @@ -62,7 +62,6 @@ azldev component render [flags] -f, --force allow overwriting existing rendered component directories -h, --help help for render -o, --output-dir string output directory for rendered specs (overrides rendered-specs-dir from config) - --skip-lock-validation skip lock file consistency checks -s, --spec-path stringArray Spec path ``` diff --git a/internal/app/azldev/cmds/component/changed.go b/internal/app/azldev/cmds/component/changed.go index 63a1b896..40128896 100644 --- a/internal/app/azldev/cmds/component/changed.go +++ b/internal/app/azldev/cmds/component/changed.go @@ -119,11 +119,13 @@ const ( func ChangedComponents( env *azldev.Env, options *ChangedComponentOptions, ) ([]ChangedResult, error) { - // Changed compares lock files between git refs — skip validation since - // the current working-tree locks may legitimately be stale. - options.ComponentFilter.SkipLockValidation = true + // Changed compares lock files between git refs — skip both validation and population + // since it reads locks directly from git commits (ReadAllAtCommit). + options.ComponentFilter.LockMode = components.LockModeSkipBoth resolver := components.NewResolver(env) + // Skip lock population since changed reads locks directly from git commits + resolver.SkipLockPopulation = true comps, err := resolver.FindComponents(&options.ComponentFilter) if err != nil { diff --git a/internal/app/azldev/cmds/component/list.go b/internal/app/azldev/cmds/component/list.go index ac348c81..ba8e3236 100644 --- a/internal/app/azldev/cmds/component/list.go +++ b/internal/app/azldev/cmds/component/list.go @@ -17,6 +17,9 @@ import ( type ListComponentOptions struct { // Standard filter for selecting components. ComponentFilter components.ComponentFilter + // SkipLockPopulation controls whether lock files are read during resolution. + // Only applicable when component list is explicitly requested by the user. + SkipLockPopulation bool } func listOnAppInit(_ *azldev.App, parentCmd *cobra.Command) { @@ -57,26 +60,32 @@ Component name patterns support glob syntax (*, ?, []).`, components.AddComponentFilterOptionsToCommand(cmd, &options.ComponentFilter) - // List always skips lock validation (read-only), so the flag is - // meaningless here. Hide it to avoid confusion. - _ = cmd.Flags().MarkHidden("skip-lock-validation") + // Add skip-lock-population flag for list command + cmd.Flags().BoolVar(&options.SkipLockPopulation, "skip-lock-population", + true, + "skip lock file population (default: true; set to false to read lock data)") return cmd } // ListComponentConfigs lists components in the env, in accordance with options. -// Lock validation is always skipped regardless of the caller's SkipLockValidation -// value — list is read-only. +// Lock validation is always skipped since list is read-only. func ListComponentConfigs( env *azldev.Env, options *ListComponentOptions, ) (results []projectconfig.ComponentConfig, err error) { var comps *components.ComponentSet - // List is read-only — skip lock validation so it works even when - // locks are missing or stale. - options.ComponentFilter.SkipLockValidation = true + // List is read-only — always skip lock validation. + // Determine lock mode based on user preference for population. + if options.SkipLockPopulation { + options.ComponentFilter.LockMode = components.LockModeSkipBoth + } else { + options.ComponentFilter.LockMode = components.LockModeSkipValidationPopulate + } resolver := components.NewResolver(env) + // Set the resolver's SkipLockPopulation to match the filter's LockMode + resolver.SkipLockPopulation = options.SkipLockPopulation comps, err = resolver.FindComponents(&options.ComponentFilter) if err != nil { diff --git a/internal/app/azldev/cmds/component/update.go b/internal/app/azldev/cmds/component/update.go index 906162fa..ee177bbe 100644 --- a/internal/app/azldev/cmds/component/update.go +++ b/internal/app/azldev/cmds/component/update.go @@ -150,8 +150,7 @@ type UpdateResult struct { // UpdateComponents resolves source identities for all selected components and // writes the results to per-component lock files under locks/. -// Lock validation is always skipped regardless of the caller's SkipLockValidation -// value — update is the lock writer. +// Lock validation is always skipped — update is the lock writer. func UpdateComponents(env *azldev.Env, options *UpdateComponentOptions) ([]UpdateResult, error) { if options.Bump && options.CheckOnly { return nil, fmt.Errorf("%w: --bump and --check-only are mutually exclusive", azldev.ErrInvalidUsage) @@ -161,9 +160,10 @@ func UpdateComponents(env *azldev.Env, options *UpdateComponentOptions) ([]Updat // Suppress staleness warnings — we're about to refresh the locks ourselves, // so warning the user to "run component update" would be self-referential noise. resolver.SuppressLockWarnings = true - // Skip lock validation — update is the lock file writer, so missing or - // stale locks are expected and will be fixed by this command. - options.ComponentFilter.SkipLockValidation = true + // Skip lock validation but keep population — update is the lock file writer, + // so missing or stale locks are expected and will be fixed by this command. + // We still need to populate locks to compute freshness. + options.ComponentFilter.LockMode = components.LockModeSkipValidationPopulate // Enable freshness checking so the resolver computes FreshnessStatus for // each component. This lets resolveSourceIdentitiesParallel skip // re-resolution for components whose resolution inputs haven't changed. diff --git a/internal/app/azldev/core/components/filter.go b/internal/app/azldev/core/components/filter.go index 95c9b582..d1416ca8 100644 --- a/internal/app/azldev/core/components/filter.go +++ b/internal/app/azldev/core/components/filter.go @@ -10,6 +10,35 @@ import ( "github.com/spf13/cobra" ) +// LockMode defines how lock files should be handled during component resolution. +type LockMode int + +const ( + // LockModeValidateAndPopulate validates lock file consistency and populates lock data on components (default). + LockModeValidateAndPopulate LockMode = iota + // LockModeSkipValidationPopulate skips lock file validation but still populates lock data on components. + // Used by commands that build/render components and consume lock data as-is. + LockModeSkipValidationPopulate + // LockModeSkipBoth skips both lock file validation and population of lock data. + // Lock population can only be skipped if validation is also skipped. + // Used by read-only commands (list, changed) that don't consume lock data. + LockModeSkipBoth +) + +// String returns a string representation of the LockMode. +func (m LockMode) String() string { + switch m { + case LockModeValidateAndPopulate: + return "validateAndPopulate" + case LockModeSkipValidationPopulate: + return "skipValidationPopulate" + case LockModeSkipBoth: + return "skipBoth" + default: + return "unknown" + } +} + // Describes a filter that selects a subset of components known within the environment's // loaded configuration. type ComponentFilter struct { @@ -22,10 +51,10 @@ type ComponentFilter struct { SpecPaths []string // If true, then *all* known components are included in the result set. IncludeAllComponents bool - // SkipLockValidation disables lock file consistency checks for this - // filter's resolution. Commands that write lock files (update) or are - // read-only (list) set this to true. - SkipLockValidation bool + // LockMode defines how lock file validation and population should be handled during resolution. + // Default is LockModeValidateAndPopulate. + // Lock population can only be skipped if validation is also skipped (LockModeSkipBoth). + LockMode LockMode } // HasNoCriteria returns true if the filter has no criteria set, meaning that it will never @@ -53,9 +82,8 @@ func AddComponentFilterOptionsToCommand(cmd *cobra.Command, filter *ComponentFil cmd.Flags().StringArrayVarP(&filter.SpecPaths, "spec-path", "s", []string{}, "Spec path") _ = cmd.MarkFlagFilename("spec-path", ".spec") - cmd.Flags().BoolVar(&filter.SkipLockValidation, "skip-lock-validation", - false, - "skip lock file consistency checks") + // Note: skip-lock-validation flag is added per-command, not here, to allow customization + // (e.g., hide for commands that always skip, or pair with skip-lock-population for list) } // Function suitable for use as a [cobra.ValidArgsFunction] in a [cobra.Command]. Intended for use diff --git a/internal/app/azldev/core/components/resolver.go b/internal/app/azldev/core/components/resolver.go index 5001ac1b..33908c69 100644 --- a/internal/app/azldev/core/components/resolver.go +++ b/internal/app/azldev/core/components/resolver.go @@ -4,6 +4,7 @@ package components import ( + "context" "errors" "fmt" "log/slog" @@ -16,6 +17,7 @@ import ( "github.com/microsoft/azure-linux-dev-tools/internal/fingerprint" "github.com/microsoft/azure-linux-dev-tools/internal/projectconfig" "github.com/microsoft/azure-linux-dev-tools/internal/utils/fileutils" + "github.com/microsoft/azure-linux-dev-tools/internal/utils/parmap" ) // Well-known errors. @@ -41,6 +43,11 @@ type Resolver struct { // Only 'component update' enables this — it uses the freshness status to // decide which components need re-resolution and which can be skipped. CheckFreshness bool + // SkipLockPopulation skips reading lock files and attaching lock data to + // resolved components. When true, component Locked fields remain nil. + // Set this when the resolver's lock mode requires skipping population + // (e.g., when called from a command with LockModeSkipBoth). + SkipLockPopulation bool } // NewResolver constructs a new [Resolver] for the given environment. @@ -52,11 +59,20 @@ func NewResolver(env *azldev.Env) *Resolver { // Given a component filter, finds all components defined in the environment that match the filter. func (r *Resolver) FindComponents(filter *ComponentFilter) (components *ComponentSet, err error) { - // The filter's SkipLockValidation field is the primary control. When - // created via AddComponentFilterOptionsToCommand, its default is false - // (validation on). Commands that write locks (update) or are read-only - // (list) explicitly set it to true. - skipValidation := filter.SkipLockValidation + // Determine whether to skip lock validation based on the filter's LockMode. + skipValidation := filter.LockMode != LockModeValidateAndPopulate + // Also check the resolver's SkipLockPopulation field (set programmatically by callers) + skipPopulation := filter.LockMode == LockModeSkipBoth || r.SkipLockPopulation + + // Temporarily set r.SkipLockPopulation during this resolution to ensure + // FindAllComponents and other methods respect the skip requirement. + oldSkipLockPopulation := r.SkipLockPopulation + if skipPopulation { + r.SkipLockPopulation = true + } + defer func() { + r.SkipLockPopulation = oldSkipLockPopulation + }() // For usability's sake, detect if the caller/user forgot to specify *any* criteria. if filter.HasNoCriteria() { @@ -72,7 +88,13 @@ func (r *Resolver) FindComponents(filter *ComponentFilter) (components *Componen return allComps, findErr } - r.warnOnLockDrift(allComps) + if !skipPopulation { + r.populateLocksParallel(allComps) + } + + if !r.SuppressLockWarnings { + r.warnOnLockDrift(allComps) + } return allComps, r.validateLockFiles(allComps, true, skipValidation) } @@ -103,6 +125,10 @@ func (r *Resolver) FindComponents(filter *ComponentFilter) (components *Componen } } + if !skipPopulation { + r.populateLocksParallel(components) + } + r.warnOnLockDrift(components) return components, r.validateLockFiles(components, false, skipValidation) @@ -162,6 +188,10 @@ func (r *Resolver) FindAllComponents() (components *ComponentSet, err error) { components.Add(comp) } + if !r.SkipLockPopulation { + r.populateLocksParallel(components) + } + return components, nil } @@ -497,17 +527,23 @@ func (r *Resolver) createComponentFromConfig(componentConfig *projectconfig.Comp componentConfig.Release.Calculation = projectconfig.ReleaseCalculationAuto } - // Populate locked state onto the component config. This makes lock data - // available to all downstream consumers (render, build, prepare-sources, - // diff-sources) without each needing lock-file awareness. - r.populateFromLock(componentConfig) - return &resolvedComponent{ env: r.env, config: *componentConfig, }, nil } +func (r *Resolver) populateLocksParallel(set *ComponentSet) { + comps := set.Components() + + _ = parmap.Map(r.env, r.env.IOBoundConcurrency(), comps, nil, + func(_ context.Context, c Component) error { + r.populateFromLock(c.GetConfig()) + + return nil + }) +} + // populateFromLock reads lock file data and attaches it to the component config // as [projectconfig.ComponentLockData]. This centralizes lock-file consumption so // all downstream commands (render, build, prepare-sources, diff-sources) get diff --git a/internal/app/azldev/core/components/resolver_test.go b/internal/app/azldev/core/components/resolver_test.go index dd734667..6b0daae1 100644 --- a/internal/app/azldev/core/components/resolver_test.go +++ b/internal/app/azldev/core/components/resolver_test.go @@ -769,7 +769,7 @@ func TestFindComponents_ErrorWhenNoLockFile(t *testing.T) { assert.Contains(t, err.Error(), "lock file") } -// When SkipLockValidation is set, a missing lock file is tolerated — +// When LockMode is set to skip validation, a missing lock file is tolerated — // the component's Locked field is nil instead of causing an error. func TestFindComponents_LockedNilWhenValidationSkipped(t *testing.T) { env := testutils.NewTestEnv(t) @@ -784,7 +784,7 @@ func TestFindComponents_LockedNilWhenValidationSkipped(t *testing.T) { filter := &components.ComponentFilter{ IncludeAllComponents: true, - SkipLockValidation: true, + LockMode: components.LockModeSkipValidationPopulate, } resolved, err := components.NewResolver(env.Env).FindComponents(filter) @@ -820,7 +820,7 @@ func TestFindComponents_ExplicitPinPreserved(t *testing.T) { filter := &components.ComponentFilter{ IncludeAllComponents: true, - SkipLockValidation: true, + LockMode: components.LockModeSkipValidationPopulate, } resolved, err := components.NewResolver(env.Env).FindComponents(filter) @@ -921,7 +921,7 @@ func TestFindComponents_CorruptLockSurfaces(t *testing.T) { filter := &components.ComponentFilter{ IncludeAllComponents: true, - SkipLockValidation: true, + LockMode: components.LockModeSkipValidationPopulate, } resolved, err := components.NewResolver(env.Env).FindComponents(filter) @@ -955,7 +955,7 @@ func TestFindComponents_PinDiffersFromLock(t *testing.T) { filter := &components.ComponentFilter{ IncludeAllComponents: true, - SkipLockValidation: true, + LockMode: components.LockModeSkipValidationPopulate, } resolved, err := components.NewResolver(env.Env).FindComponents(filter) @@ -1011,3 +1011,41 @@ func TestComponentFilter_HasNoCriteria(t *testing.T) { assert.False(t, filter.HasNoCriteria()) }) } + +// When LockMode is set to LockModeSkipBoth, lock files are not read +// and Locked remains nil — even when a valid lock file exists on disk. +func TestFindComponents_SkipLockPopulation(t *testing.T) { + env := testutils.NewTestEnv(t) + + env.Config.Components["curl"] = projectconfig.ComponentConfig{ + Name: "curl", + Spec: projectconfig.SpecSource{ + SourceType: projectconfig.SpecSourceTypeUpstream, + UpstreamCommit: "config-pin-aaa", + }, + } + + // Write a valid lock file that would normally be read. + lock := lockfile.New() + lock.UpstreamCommit = "lock-commit-bbb" + + env.WriteLock(t, "curl", lock) + + filter := &components.ComponentFilter{ + IncludeAllComponents: true, + LockMode: components.LockModeSkipBoth, + } + + resolver := components.NewResolver(env.Env) + + resolved, err := resolver.FindComponents(filter) + require.NoError(t, err) + + comp, ok := resolved.TryGet("curl") + require.True(t, ok) + + assert.Nil(t, comp.GetConfig().Locked, + "Locked must be nil when LockModeSkipBoth is set, even with a valid lock on disk") + assert.Equal(t, "config-pin-aaa", comp.GetConfig().Spec.UpstreamCommit, + "spec pin must be preserved regardless") +} diff --git a/internal/projectconfig/component.go b/internal/projectconfig/component.go index 9d45a2b4..4d75e301 100644 --- a/internal/projectconfig/component.go +++ b/internal/projectconfig/component.go @@ -314,8 +314,8 @@ func (c *ComponentConfig) MergeUpdatesFrom(other *ComponentConfig) error { // EffectiveUpstreamCommit returns the commit to use for upstream operations. // Prefers the locked commit (resolved reality) over the config pin (user intent). -// Falls back to Spec.UpstreamCommit for SkipLockValidation paths (update, list, -// changed) where Locked may be nil. Returns empty string when neither is set. +// Falls back to Spec.UpstreamCommit for LockModeSkipBoth paths (list, changed) +// where Locked may be nil. Returns empty string when neither is set. func (c *ComponentConfig) EffectiveUpstreamCommit() string { if c.Locked != nil && c.Locked.UpstreamCommit != "" { return c.Locked.UpstreamCommit