Skip to content

perf(workflow): cache manifest baseline lookup in CompileWorkflow path#31584

Merged
pelikhan merged 9 commits into
mainfrom
copilot/fix-compile-complex-workflow-performance
May 11, 2026
Merged

perf(workflow): cache manifest baseline lookup in CompileWorkflow path#31584
pelikhan merged 9 commits into
mainfrom
copilot/fix-compile-complex-workflow-performance

Conversation

Copilot AI commented May 11, 2026

Copy link
Copy Markdown
Contributor

✨ Enhancement

BenchmarkCompileComplexWorkflow regressed by ~23.5% (3.39ms vs 2.74ms historical), indicating extra work in the compile pipeline. This change removes repeated baseline-manifest resolution during repeated compiles on the same Compiler instance.

  • What does this improve?

    • Reduces repeated HEAD/filesystem manifest reads in CompileWorkflowData for the same lock file.
    • Lowers per-compile overhead in complex workflow compilation loops.
    • Avoids persisting nil baseline state from legacy lock files, allowing later compiles to pick up a manifest when one becomes available.
  • Why is this valuable?

    • The regression signal came from a hot benchmark path (CompileComplexWorkflow), where repeated compiles amplify avoidable I/O/lookup work.
    • Keeps safe-update baseline semantics stable across repeated compiles in a single compiler lifecycle.
    • Prevents a nil baseline from being cached and unintentionally suppressing enforcement behavior for the remainder of the compiler instance.
  • Implementation approach:

    • Cache resolved prior manifest baselines in Compiler.priorManifests keyed by lock file path.
    • Initialize priorManifests in NewCompiler and normalize nil input in SetPriorManifests.
    • Populate cache on first non-nil resolution and reuse on subsequent compiles (intentional first-baseline semantics).
    • Add focused unit coverage to assert:
      • cache population and persistence across repeated compile calls
      • no-overwrite behavior for the first cached baseline
      • nil legacy baselines are not cached
if oldManifest != nil {
    if _, ok := c.priorManifests[lockFile]; !ok {
        c.priorManifests[lockFile] = oldManifest
    }
}

Copilot AI and others added 4 commits May 11, 2026 21:12
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix regression in CompileComplexWorkflow performance perf(workflow): cache manifest baseline lookup in CompileWorkflow path May 11, 2026
Copilot AI requested a review from pelikhan May 11, 2026 21:17
@pelikhan pelikhan marked this pull request as ready for review May 11, 2026 21:40
Copilot AI review requested due to automatic review settings May 11, 2026 21:40

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Caches resolved safe-update baseline manifests per lockfile path on the Compiler instance to avoid repeated HEAD/filesystem baseline reads across repeated compiles in the same process, addressing a benchmark regression in CompileWorkflow hot paths.

Changes:

  • Cache the first resolved baseline manifest in Compiler.priorManifests during CompileWorkflowData.
  • Initialize priorManifests in NewCompiler and normalize nil input in SetPriorManifests.
  • Add a unit test asserting that baseline manifest resolution populates the cache across repeated compiles.
Show a summary per file
File Description
pkg/workflow/compiler.go Adds first-baseline caching into priorManifests during baseline resolution.
pkg/workflow/compiler_types.go Initializes priorManifests and makes SetPriorManifests(nil) safe.
pkg/workflow/compiler_test.go Adds coverage for baseline cache population across repeated compiles.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

  • Files reviewed: 3/3 changed files
  • Comments generated: 2

Comment thread pkg/workflow/compiler.go Outdated
// This intentionally does not overwrite an existing cache entry so repeated
// compiles in the same process continue to compare against the same trusted
// baseline rather than a just-generated local lock file.
if _, ok := c.priorManifests[lockFile]; !ok {
require.NoError(t, compiler.CompileWorkflow(testFile))
secondBaseline, ok := manifestCache[lockFile]
require.True(t, ok, "cached baseline should remain available after second compile")
require.NotNil(t, secondBaseline, "cached baseline should be non-nil")
@pelikhan

Copy link
Copy Markdown
Collaborator

@copilot review all comments

Copilot AI and others added 3 commits May 11, 2026 21:56
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
@pelikhan

Copy link
Copy Markdown
Collaborator

@copilot review all comments

Copilot AI commented May 11, 2026

Copy link
Copy Markdown
Contributor Author

@copilot review all comments

Addressed the actionable review threads in commit 0c1c879.

  • Cache now stores only non-nil baselines, so legacy nil baselines are not persisted.
  • Added/updated tests to verify first-baseline no-overwrite behavior and the nil-legacy non-caching case.
  • Targeted tests pass for the updated compiler cache behavior.

Copilot AI commented May 11, 2026

Copy link
Copy Markdown
Contributor Author

@copilot review all comments

Reviewed and addressed the actionable review threads in 0c1c879.

  • nil prior manifests are no longer cached
  • cache keeps the first resolved baseline (no overwrite)
  • added coverage for the nil-legacy non-caching case

No additional changes are needed at this point.

@pelikhan pelikhan merged commit 95f1809 into main May 11, 2026
22 of 23 checks passed
@pelikhan pelikhan deleted the copilot/fix-compile-complex-workflow-performance branch May 11, 2026 23:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[performance] Regression in CompileComplexWorkflow: 23.5% slower

3 participants