Skip to content

feat: add clone string to handle memory in cm2 - #4395

Merged
cgrinds merged 3 commits into
mainfrom
rg2-fix-cm-leak
Aug 7, 2026
Merged

feat: add clone string to handle memory in cm2#4395
cgrinds merged 3 commits into
mainfrom
rg2-fix-cm-leak

Conversation

@rahulguptajss

Copy link
Copy Markdown
Contributor

No description provided.

Copilot AI lite review requested due to automatic review settings August 7, 2026 14:45
@cla-bot cla-bot Bot added the cla-signed label Aug 7, 2026

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

This PR updates the CM performance metrics protobuf parsing to clone strings extracted via easyproto.FieldContext.String(), preventing returned structs from retaining references to the underlying parse buffer and reducing long-lived memory retention when callers keep parsed records.

Changes:

  • Added a clonedString(*easyproto.FieldContext) helper that wraps fc.String() and returns strings.Clone(...).
  • Replaced all direct fc.String() usages in parsing logic with clonedString(&fc) so stored strings no longer reference the input buffer.

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

Copilot AI review requested due to automatic review settings August 7, 2026 14:50

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

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Suppressed comments (1)

cmd/collectors/cmperf/cmmetrics/metrics.go:290

  • The comment has a grammatical error and is missing the subject, which makes the safety rationale unclear. It should explicitly say that the parser clones retained strings.
			// buf is reused across messages, which is safe because the
			// every string it keeps and never stores a slice of buf.

Comment thread cmd/collectors/cmperf/cmmetrics/metrics.go Outdated
Copilot AI review requested due to automatic review settings August 7, 2026 15:11
@cgrinds
cgrinds merged commit 7a7ba3d into main Aug 7, 2026
12 checks passed
@cgrinds
cgrinds deleted the rg2-fix-cm-leak branch August 7, 2026 15:11

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

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Suppressed comments (1)

cmd/collectors/cmperf/cmmetrics/metrics.go:290

  • The comment explaining why reusing buf is safe is grammatically incorrect ("because the / every string...") and reads like it’s missing the subject (the parser). This makes the rationale harder to understand for future maintainers.
			// buf is reused across messages, which is safe because the
			// every string it keeps and never stores a slice of buf.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants