Skip to content

Conversation

@felixge
Copy link
Member

@felixge felixge commented Oct 27, 2025

What does this PR do?

Refactor the zstd encoder sharing implemented in #4058 by assigning acquire/release semantics to the compressor interface Reset / Close methods.

Motivation

Hide the implementation details of the shared encoder from the profile type implementations. Also properly support multiple zstd levels for different profile types. This is not needed for now, but the current interfaces expect this ability to work. If we want to constrain this in the future, we should do it on the interface level first.

Reviewer's Checklist

  • Changed code has unit tests for its functionality at or near 100% coverage.
  • System-Tests covering this feature have been added and enabled with the va.b.c-dev version tag.
  • There is a benchmark for any new code, or changes to existing code.
  • If this interacts with the agent in a new way, a system test has been added.
  • New code is free of linting errors. You can check this by running ./scripts/lint.sh locally.
  • Add an appropriate team label so this PR gets put in the right place for the release notes.
  • Non-trivial go.mod changes, e.g. adding new modules, are reviewed by @DataDog/dd-trace-go-guild.

Unsure? Have a question? Request a review!

This benchmark seems to have been broken since it was added in #3555.
@pr-commenter
Copy link

pr-commenter bot commented Oct 27, 2025

Benchmarks

Benchmark execution time: 2025-10-28 09:34:28

Comparing candidate commit 1079a3d in PR branch felix.geisendoerfer/alternative-zstd-encoder-reuse-approach with baseline commit a6be279 in branch PROF-12641-shared-zstd-encoder.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 24 metrics, 0 unstable metrics.

@felixge felixge force-pushed the felix.geisendoerfer/alternative-zstd-encoder-reuse-approach branch from e6f96f4 to 4c9cf96 Compare October 28, 2025 05:06
@datadog-datadog-prod-us1
Copy link

datadog-datadog-prod-us1 bot commented Oct 28, 2025

✅ Tests

🎉 All green!

❄️ No new flaky tests detected
🧪 All tests passed

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: 25c00e5 | Docs | Was this helpful? Give us feedback!

@felixge felixge changed the title fix(profiler): BenchmarkRecompression panic @felixge refactor(profiler): alternative zstd encoder reuse approach Oct 28, 2025
@felixge felixge changed the title @felixge refactor(profiler): alternative zstd encoder reuse approach refactor(profiler): alternative zstd encoder reuse approach Oct 28, 2025
@felixge felixge requested a review from Copilot October 28, 2025 09:03
Copy link

Copilot AI left a comment

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 refactors the zstd encoder sharing mechanism by replacing a global mutex-based approach with a builder pattern that assigns acquire/release semantics to the compressor interface's Reset/Close methods.

Key Changes:

  • Introduced compressionPipelineBuilder to manage shared zstd encoders per compression level
  • Replaced global compressionMux with per-encoder semaphores embedded in sharedZstdEncoder
  • Updated all compression usage sites to call Close() after Reset() to properly release encoder resources

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.

File Description
profiler/compression.go Implements new compressionPipelineBuilder and sharedZstdEncoder types with semaphore-based encoder sharing
profiler/profiler.go Uses compressionPipelineBuilder instance to build compression pipelines
profiler/profile.go Removes global mutex locking and adds deferred Close() calls for resource release
profiler/compression_test.go Updates tests to use new builder pattern

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

@felixge felixge force-pushed the felix.geisendoerfer/alternative-zstd-encoder-reuse-approach branch from 59d5391 to 280fb17 Compare October 28, 2025 09:17
@felixge felixge requested a review from Copilot October 28, 2025 09:18
Copy link

Copilot AI left a comment

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 4 out of 4 changed files in this pull request and generated 2 comments.


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

@felixge felixge marked this pull request as ready for review October 28, 2025 09:40
@felixge felixge requested a review from a team as a code owner October 28, 2025 09:40
Copy link
Contributor

@nsrip-dd nsrip-dd left a comment

Choose a reason for hiding this comment

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

Nice refactor. Thanks!

@nsrip-dd nsrip-dd merged commit a37f42c into PROF-12641-shared-zstd-encoder Oct 28, 2025
242 checks passed
@nsrip-dd nsrip-dd deleted the felix.geisendoerfer/alternative-zstd-encoder-reuse-approach branch October 28, 2025 11:57
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.

3 participants