Skip to content

fix: reduce memory consumption by eliminating redundant string copies #965

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
yxxhero merged 5 commits into from
Mar 29, 2026
+221 −57
Merged

fix: reduce memory consumption by eliminating redundant string copies #965

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
229bf9e
fix: reduce memory consumption by eliminating redundant string copies
yxxhero Mar 28, 2026
c8b3679
fix: remove ineffectual nil assignments flagged by linter
yxxhero Mar 28, 2026
30b3e80
fix: address PR review comments for reduce memory consumption
yxxhero Mar 28, 2026
a8fed47
fix: address all PR review comments
Copilot Mar 29, 2026
fe9603d
fix: suppress ineffassign warnings on GC-hint nil assignments
Copilot Mar 29, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions cmd/release.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,9 +109,14 @@ func (d *release) differentiateHelm3() error {
}

if releaseChart1 == releaseChart2 {
oldSpecs := manifest.Parse(releaseResponse1, namespace1, d.normalizeManifests, excludes...)
newSpecs := manifest.Parse(releaseResponse2, namespace2, d.normalizeManifests, excludes...)
releaseResponse1 = nil //nolint:ineffassign // nil to allow GC to reclaim raw bytes before diff computation
releaseResponse2 = nil //nolint:ineffassign // nil to allow GC to reclaim raw bytes before diff computation

Copy link

Copilot AI Mar 28, 2026

Choose a reason for hiding this comment

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

Likewise, releaseResponse1/releaseResponse2 remain referenced after parsing into oldSpecs/newSpecs. If these raw manifest bytes aren’t used later, consider setting them to nil after parsing to reduce peak memory during diff.Releases(...) for large releases.

Suggested change
// Release raw manifest responses to reduce peak memory usage before diffing.
releaseResponse1 = nil
releaseResponse2 = nil

Copilot uses AI. Check for mistakes.
seenAnyChanges := diff.Releases(
manifest.Parse(string(releaseResponse1), namespace1, d.normalizeManifests, excludes...),
manifest.Parse(string(releaseResponse2), namespace2, d.normalizeManifests, excludes...),
oldSpecs,
newSpecs,
&d.Options,
Comment on lines 111 to 120
Copy link

Copilot AI Mar 28, 2026

Choose a reason for hiding this comment

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

The raw releaseResponse1 / releaseResponse2 byte slices stay live for the duration of diff.Releases. If you want the earlier GC reclamation described in the PR, parse into locals first and then set the raw byte slices to nil before running the diff.

Copilot uses AI. Check for mistakes.
os.Stdout)

Expand Down
18 changes: 14 additions & 4 deletions cmd/revision.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,9 +99,14 @@ func (d *revision) differentiateHelm3() error {
return err
}

oldSpecs := manifest.Parse(revisionResponse, namespace, d.normalizeManifests, excludes...)
newSpecs := manifest.Parse(releaseResponse, namespace, d.normalizeManifests, excludes...)
revisionResponse = nil //nolint:ineffassign // nil to allow GC to reclaim raw bytes before diff computation
releaseResponse = nil //nolint:ineffassign // nil to allow GC to reclaim raw bytes before diff computation

Comment on lines 101 to +106
Copy link

Copilot AI Mar 28, 2026

Choose a reason for hiding this comment

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

In this command path the raw []byte responses (releaseResponse, revisionResponse*) remain referenced after parsing, so they can’t be GC’d until the function returns. If the goal is to reduce peak memory during diff computation, consider setting these byte slices to nil after manifest.Parse(...) once you no longer need the raw manifest bytes.

Copilot uses AI. Check for mistakes.
diff.Manifests(
manifest.Parse(string(revisionResponse), namespace, d.normalizeManifests, excludes...),
manifest.Parse(string(releaseResponse), namespace, d.normalizeManifests, excludes...),
oldSpecs,
newSpecs,
&d.Options,
os.Stdout)
Comment on lines 107 to 111
Copy link

Copilot AI Mar 28, 2026

Choose a reason for hiding this comment

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

In this code path, the raw releaseResponse / revisionResponse manifest byte slices remain referenced while diff.Manifests runs. If the goal is to reduce peak memory, consider parsing into locals and then setting these byte slices to nil before starting the diff.

Copilot uses AI. Check for mistakes.

Expand All @@ -122,9 +127,14 @@ func (d *revision) differentiateHelm3() error {
return err
}

oldSpecs := manifest.Parse(revisionResponse1, namespace, d.normalizeManifests, excludes...)
newSpecs := manifest.Parse(revisionResponse2, namespace, d.normalizeManifests, excludes...)
revisionResponse1 = nil //nolint:ineffassign // nil to allow GC to reclaim raw bytes before diff computation
revisionResponse2 = nil //nolint:ineffassign // nil to allow GC to reclaim raw bytes before diff computation

seenAnyChanges := diff.Manifests(
manifest.Parse(string(revisionResponse1), namespace, d.normalizeManifests, excludes...),
manifest.Parse(string(revisionResponse2), namespace, d.normalizeManifests, excludes...),
oldSpecs,
newSpecs,
&d.Options,
os.Stdout)

Expand Down
9 changes: 7 additions & 2 deletions cmd/rollback.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,9 +90,14 @@ func (d *rollback) backcastHelm3() error {
}

// create a diff between the current manifest and the version of the manifest that a user is intended to rollback
oldSpecs := manifest.Parse(releaseResponse, namespace, d.normalizeManifests, excludes...)
newSpecs := manifest.Parse(revisionResponse, namespace, d.normalizeManifests, excludes...)
releaseResponse = nil //nolint:ineffassign // nil to allow GC to reclaim raw bytes before diff computation
revisionResponse = nil //nolint:ineffassign // nil to allow GC to reclaim raw bytes before diff computation

Copy link

Copilot AI Mar 28, 2026

Choose a reason for hiding this comment

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

If the intent is to allow GC reclamation of the raw manifest bytes before diffing (as described in the PR summary), releaseResponse/revisionResponse are still held for the remainder of backcastHelm3. After oldSpecs/newSpecs are built, consider setting those []byte variables to nil to let the GC drop the backing arrays earlier during potentially expensive diff processing.

Suggested change
// Allow GC to reclaim raw manifest bytes before potentially expensive diff processing.
releaseResponse = nil
revisionResponse = nil

Copilot uses AI. Check for mistakes.
seenAnyChanges := diff.Manifests(
manifest.Parse(string(releaseResponse), namespace, d.normalizeManifests, excludes...),
manifest.Parse(string(revisionResponse), namespace, d.normalizeManifests, excludes...),
oldSpecs,
newSpecs,
&d.Options,
Comment on lines 92 to 101
Copy link

Copilot AI Mar 28, 2026

Choose a reason for hiding this comment

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

To reduce peak memory during the diff, consider dropping references to the large releaseResponse / revisionResponse byte slices after parsing (e.g., parse into locals, then set the byte slices to nil before calling diff.Manifests). As written, both raw buffers remain live for the duration of the diff.

Copilot uses AI. Check for mistakes.
os.Stdout)

Expand Down
10 changes: 6 additions & 4 deletions cmd/upgrade.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -327,11 +327,12 @@ func (d *diffCmd) runHelm3() error {
releaseManifest = append(releaseManifest, hooks...)
}
if d.includeTests {
currentSpecs = manifest.Parse(string(releaseManifest), d.namespace, d.normalizeManifests)
currentSpecs = manifest.Parse(releaseManifest, d.namespace, d.normalizeManifests)
} else {
currentSpecs = manifest.Parse(string(releaseManifest), d.namespace, d.normalizeManifests, manifest.Helm3TestHook, manifest.Helm2TestSuccessHook)
currentSpecs = manifest.Parse(releaseManifest, d.namespace, d.normalizeManifests, manifest.Helm3TestHook, manifest.Helm2TestSuccessHook)
}
Comment on lines 329 to 333
Copy link

Copilot AI Mar 28, 2026

Choose a reason for hiding this comment

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

The PR description mentions setting releaseManifest to nil after parsing so the large raw buffer can be reclaimed before diff computation. Here releaseManifest stays referenced after manifest.Parse(...). Consider setting it to nil once currentSpecs is built and the raw bytes are no longer needed.

Copilot uses AI. Check for mistakes.
Comment on lines 327 to 333
Copy link

Copilot AI Mar 28, 2026

Choose a reason for hiding this comment

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

PR description mentions setting raw manifest byte slices to nil after parsing so GC can reclaim them before diff computation. In this code path, releaseManifest remains referenced after manifest.Parse(...), so the underlying bytes can’t be reclaimed until runHelm3 returns. If the slice isn’t needed after parsing (and after any three-way merge/hook appends), consider setting releaseManifest = nil once currentSpecs is built.

Copilot uses AI. Check for mistakes.
}
releaseManifest = nil //nolint:ineffassign // nil to allow GC to reclaim raw bytes before diff computation

var newOwnedReleases map[string]diff.OwnershipDiff
if d.takeOwnership {
Expand All @@ -347,10 +348,11 @@ func (d *diffCmd) runHelm3() error {

var newSpecs map[string]*manifest.MappingResult
if d.includeTests {
newSpecs = manifest.Parse(string(installManifest), d.namespace, d.normalizeManifests)
newSpecs = manifest.Parse(installManifest, d.namespace, d.normalizeManifests)
} else {
newSpecs = manifest.Parse(string(installManifest), d.namespace, d.normalizeManifests, manifest.Helm3TestHook, manifest.Helm2TestSuccessHook)
newSpecs = manifest.Parse(installManifest, d.namespace, d.normalizeManifests, manifest.Helm3TestHook, manifest.Helm2TestSuccessHook)
}
Comment on lines 350 to 354
Copy link

Copilot AI Mar 28, 2026

Choose a reason for hiding this comment

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

If the intent is to reduce peak memory by allowing GC to reclaim the rendered manifest early, consider setting installManifest to nil after newSpecs is computed (before invoking the diff). As written, the raw buffer remains live for the rest of runHelm3.

Copilot uses AI. Check for mistakes.
Comment on lines 349 to 354
Copy link

Copilot AI Mar 28, 2026

Choose a reason for hiding this comment

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

Similarly, installManifest remains referenced after parsing into newSpecs, so the raw rendered bytes won’t be eligible for GC until the end of runHelm3. If the manifest bytes aren’t used after this point (including the takeOwnership branch), consider setting installManifest = nil after newSpecs is created to match the PR’s stated memory-reclamation behavior.

Copilot uses AI. Check for mistakes.
installManifest = nil //nolint:ineffassign // nil to allow GC to reclaim raw bytes before diff computation

seenAnyChanges := diff.ManifestsOwnership(currentSpecs, newSpecs, newOwnedReleases, &d.Options, os.Stdout)

Expand Down
22 changes: 21 additions & 1 deletion diff/diff.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,11 @@ func actualChanges(diff []difflib.DiffRecord) int {
return changes
}

const (
renameDetectionMinLengthRatio float32 = 0.1
renameDetectionMaxLengthRatio float32 = 10.0
)

func contentSearch(report *Report, possiblyRemoved []string, oldIndex map[string]*manifest.MappingResult, possiblyAdded []string, newIndex map[string]*manifest.MappingResult, options *Options) ([]string, []string) {
if options.FindRenames <= 0 {
return possiblyRemoved, possiblyAdded
Expand All @@ -198,6 +203,21 @@ func contentSearch(report *Report, possiblyRemoved []string, oldIndex map[string
continue
}

oldLen := len(oldContent.Content)
newLen := len(newContent.Content)
if oldLen == 0 || newLen == 0 {
continue
}
// Skip the length-ratio filter for Secrets: their raw content length can
// differ greatly from the post-processed (redacted/decoded) length, so the
// ratio would be an unreliable predictor of content similarity.
if oldContent.Kind != "Secret" {
ratio := float32(oldLen) / float32(newLen)
if ratio < renameDetectionMinLengthRatio || ratio > renameDetectionMaxLengthRatio {
continue
}
}
Comment on lines +206 to +219
Copy link

Copilot AI Mar 28, 2026

Choose a reason for hiding this comment

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

The new length-ratio short-circuit runs before secrets are redacted/decoded. When ShowSecrets is false (or decoding is enabled), this can skip rename detection for Secrets whose raw YAML lengths differ a lot even though the post-processed (redacted/decoded) content would be comparable. Consider moving the ratio check to after the secret pre-processing step (or basing the ratio on the same content that diffMappingResults compares).

Copilot uses AI. Check for mistakes.

switch {
case options.ShowSecretsDecoded:
decodeSecrets(oldContent, newContent)
Expand All @@ -208,7 +228,7 @@ func contentSearch(report *Report, possiblyRemoved []string, oldIndex map[string
diff := diffMappingResults(oldContent, newContent, options.StripTrailingCR)
delta := actualChanges(diff)
if delta == 0 || len(diff) == 0 {
continue // Should never happen, but better safe than sorry
continue
}
fraction := float32(delta) / float32(len(diff))
if fraction > 0 && fraction < smallestFraction {
Expand Down
155 changes: 140 additions & 15 deletions diff/diff_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -617,8 +617,8 @@ spec:
- name: app
image: demo:v2
`
oldIndex := manifest.Parse(oldManifest, "prod", true)
newIndex := manifest.Parse(newManifest, "prod", true)
oldIndex := manifest.Parse([]byte(oldManifest), "prod", true)
newIndex := manifest.Parse([]byte(newManifest), "prod", true)

var buf bytes.Buffer
changed := Manifests(oldIndex, newIndex, opts, &buf)
Expand Down Expand Up @@ -656,7 +656,7 @@ metadata:
namespace: ops
spec: {}
`
newIndex := manifest.Parse(newManifest, "ops", true)
newIndex := manifest.Parse([]byte(newManifest), "ops", true)

var buf bytes.Buffer
changed := Manifests(map[string]*manifest.MappingResult{}, newIndex, opts, &buf)
Expand Down Expand Up @@ -702,8 +702,8 @@ metadata:
data:
password: Zm9v
`
oldIndex := manifest.Parse(oldManifest, "default", true)
newIndex := manifest.Parse(newManifest, "default", true)
oldIndex := manifest.Parse([]byte(oldManifest), "default", true)
newIndex := manifest.Parse([]byte(newManifest), "default", true)

var buf bytes.Buffer
changed := Manifests(oldIndex, newIndex, opts, &buf)
Expand Down Expand Up @@ -822,8 +822,8 @@ metadata:
name: test
namespace: default
`
oldIndex := manifest.Parse(emptyManifest, "default", true)
newIndex := manifest.Parse(validManifest, "default", true)
oldIndex := manifest.Parse([]byte(emptyManifest), "default", true)
newIndex := manifest.Parse([]byte(validManifest), "default", true)

var buf bytes.Buffer
changed := Manifests(oldIndex, newIndex, opts, &buf)
Expand All @@ -846,8 +846,8 @@ metadata:
name: test
namespace: default
`
oldIndex := manifest.Parse(nullManifest, "default", true)
newIndex := manifest.Parse(validManifest, "default", true)
oldIndex := manifest.Parse([]byte(nullManifest), "default", true)
newIndex := manifest.Parse([]byte(validManifest), "default", true)

var buf bytes.Buffer
changed := Manifests(oldIndex, newIndex, opts, &buf)
Expand Down Expand Up @@ -890,8 +890,8 @@ data:
deeply:
value: new
`
oldIndex := manifest.Parse(oldManifest, "default", true)
newIndex := manifest.Parse(newManifest, "default", true)
oldIndex := manifest.Parse([]byte(oldManifest), "default", true)
newIndex := manifest.Parse([]byte(newManifest), "default", true)

var buf bytes.Buffer
changed := Manifests(oldIndex, newIndex, opts, &buf)
Expand Down Expand Up @@ -944,8 +944,8 @@ spec:
- name: KEY2
value: val2
`
oldIndex := manifest.Parse(oldManifest, "prod", true)
newIndex := manifest.Parse(newManifest, "prod", true)
oldIndex := manifest.Parse([]byte(oldManifest), "prod", true)
newIndex := manifest.Parse([]byte(newManifest), "prod", true)

var buf bytes.Buffer
changed := Manifests(oldIndex, newIndex, opts, &buf)
Expand All @@ -967,8 +967,8 @@ spec:
emptyManifest1 := ``
emptyManifest2 := ``

oldIndex := manifest.Parse(emptyManifest1, "default", true)
newIndex := manifest.Parse(emptyManifest2, "default", true)
oldIndex := manifest.Parse([]byte(emptyManifest1), "default", true)
newIndex := manifest.Parse([]byte(emptyManifest2), "default", true)

var buf bytes.Buffer
changed := Manifests(oldIndex, newIndex, opts, &buf)
Expand Down Expand Up @@ -1593,3 +1593,128 @@ data:
require.Contains(t, new.Content, "key1: '++++++++ # (6 bytes)'")
})
}

func TestRenameDetectionLengthRatio(t *testing.T) {
ansi.DisableColors(true)

makeSpec := func(name string, content string) map[string]*manifest.MappingResult {
return map[string]*manifest.MappingResult{
name: {
Name: name,
Kind: "Deployment",
Content: content,
},
}
}

shortContent := `
apiVersion: apps/v1
kind: Deployment
metadata:
name: short
spec:
replicas: 1
`

shortContentRenamed := `
apiVersion: apps/v1
kind: Deployment
metadata:
name: short-renamed
spec:
replicas: 1
`

longContent := `
apiVersion: apps/v1
kind: Deployment
metadata:
name: very-long
spec:
replicas: 1
template:
spec:
containers:
- name: app
image: myapp:v1
ports:
- containerPort: 8080
env:
- name: VAR1
value: "hello"
- name: VAR2
value: "world"
- name: VAR3
value: "foo"
- name: VAR4
value: "bar"
- name: VAR5
value: "baz"
resources:
limits:
cpu: "1"
memory: "1Gi"
`

t.Run("similar length detects rename", func(t *testing.T) {
var buf bytes.Buffer
opts := &Options{OutputFormat: "diff", OutputContext: 10, ShowSecrets: true, FindRenames: 0.5}

oldSpec := makeSpec("default, short, Deployment (apps)", shortContent)
newSpec := makeSpec("default, short-renamed, Deployment (apps)", shortContentRenamed)

changed := Manifests(oldSpec, newSpec, opts, &buf)
require.True(t, changed)
require.Contains(t, buf.String(), "default, short, Deployment (apps) has changed")
})

t.Run("very different length skips rename", func(t *testing.T) {
var buf bytes.Buffer
opts := &Options{OutputFormat: "diff", OutputContext: 10, ShowSecrets: true, FindRenames: 0.5}

oldSpec := makeSpec("default, short, Deployment (apps)", shortContent)
newSpec := makeSpec("default, very-long, Deployment (apps)", longContent)

changed := Manifests(oldSpec, newSpec, opts, &buf)
require.True(t, changed)
require.Contains(t, buf.String(), "default, short, Deployment (apps) has been removed")
require.Contains(t, buf.String(), "default, very-long, Deployment (apps) has been added")
})

t.Run("empty content skipped", func(t *testing.T) {
var buf bytes.Buffer
opts := &Options{OutputFormat: "diff", OutputContext: 10, ShowSecrets: true, FindRenames: 0.5}

oldSpec := map[string]*manifest.MappingResult{
"default, empty, Deployment (apps)": {
Name: "default, empty, Deployment (apps)",
Kind: "Deployment",
Content: "",
},
}
newSpec := makeSpec("default, short-renamed, Deployment (apps)", shortContentRenamed)

changed := Manifests(oldSpec, newSpec, opts, &buf)
require.True(t, changed)
require.Contains(t, buf.String(), "has been added")
})

t.Run("different kind skipped", func(t *testing.T) {
var buf bytes.Buffer
opts := &Options{OutputFormat: "diff", OutputContext: 10, ShowSecrets: true, FindRenames: 0.5}

oldSpec := map[string]*manifest.MappingResult{
"default, svc, Service (v1)": {
Name: "default, svc, Service (v1)",
Kind: "Service",
Content: shortContent,
},
}
newSpec := makeSpec("default, svc-renamed, Deployment (apps)", shortContentRenamed)

changed := Manifests(oldSpec, newSpec, opts, &buf)
require.True(t, changed)
require.Contains(t, buf.String(), "has been removed")
require.Contains(t, buf.String(), "has been added")
})
}
Loading
Loading