Skip to content

Conversation

@kakkoyun
Copy link
Member

@kakkoyun kakkoyun commented Nov 20, 2025

What does this PR do?

Replace regex-based symbol parsing with string operations to eliminate
per-frame allocations in stack capture.

Motivation

Improve the performance and reduce the allocations.

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!

Copy link
Member Author

kakkoyun commented Nov 20, 2025

@pr-commenter
Copy link

pr-commenter bot commented Nov 20, 2025

Benchmarks

Benchmark execution time: 2025-11-25 14:32:14

Comparing candidate commit 2e0365e in PR branch kakkoyun/11-20-docs-document_testunit_make_target with baseline commit 87e83e4 in branch main.

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

@kakkoyun kakkoyun changed the base branch from kakkoyun/11-20-stacktrace-use_internalstacktrace_package_where_possible to graphite-base/4148 November 20, 2025 12:56
@kakkoyun kakkoyun changed the base branch from graphite-base/4148 to kakkoyun/11-20-stacktrace-use_internalstacktrace_package_where_possible November 20, 2025 12:56
@datadog-official
Copy link
Contributor

datadog-official bot commented Nov 20, 2025

✅ Tests

🎉 All green!

❄️ No new flaky tests detected
🧪 All tests passed

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: 2e0365e | Docs | Datadog PR Page | Was this helpful? Give us feedback!

@kakkoyun kakkoyun changed the base branch from kakkoyun/11-20-stacktrace-use_internalstacktrace_package_where_possible to graphite-base/4148 November 20, 2025 12:59
@kakkoyun kakkoyun changed the base branch from graphite-base/4148 to kakkoyun/11-20-stacktrace-use_internalstacktrace_package_where_possible November 20, 2025 12:59
@kakkoyun kakkoyun force-pushed the kakkoyun/11-20-docs-document_testunit_make_target branch from e673b82 to 19fa72c Compare November 20, 2025 13:00
@kakkoyun kakkoyun changed the base branch from kakkoyun/11-20-stacktrace-use_internalstacktrace_package_where_possible to graphite-base/4148 November 20, 2025 13:00
@kakkoyun kakkoyun changed the base branch from graphite-base/4148 to main November 20, 2025 13:01
@kakkoyun kakkoyun changed the base branch from main to kakkoyun/use_stacktrace_package November 20, 2025 13:19
@kakkoyun kakkoyun force-pushed the kakkoyun/11-20-docs-document_testunit_make_target branch 2 times, most recently from fe173f5 to 89e0450 Compare November 20, 2025 13:39
@kakkoyun kakkoyun marked this pull request as ready for review November 20, 2025 13:55
@kakkoyun kakkoyun requested a review from a team as a code owner November 20, 2025 13:55
@kakkoyun kakkoyun requested a review from eliottness November 24, 2025 15:26
Copy link
Contributor

eliottness commented Nov 24, 2025

Found 0 performance improvements and 0 performance regressions!

Interesting

@kakkoyun kakkoyun marked this pull request as draft November 24, 2025 15:40
@kakkoyun
Copy link
Member Author

Found 0 performance improvements and 0 performance regressions!

Interesting

This is because we haven't had that benchmark in the main before.
I will split this PR into two and merge the benchmarking PR first so that we can have a baseline.

Great catch!

…tation

    Replace regex-based symbol parsing with string operations to eliminate
    per-frame allocations in stack capture.

    Performance improvements:
    - parseSymbol: 0 allocs/op (was ~2 allocs/op)
    - BenchmarkCaptureWithRedaction/depth_10: 38→6 allocs/op (84% reduction)
    - BenchmarkCaptureWithRedaction/depth_50: 118→6 allocs/op (95% reduction)
    - Speed: 19.2µs→4.1µs @ depth=10 (79% faster)
    - Memory: 8966B→5296B @ depth=10 (41% reduction)

    Allocations are now constant regardless of stack depth.

Signed-off-by: Kemal Akkoyun <[email protected]>
@kakkoyun kakkoyun force-pushed the kakkoyun/11-20-docs-document_testunit_make_target branch from 89e0450 to 0cbb439 Compare November 24, 2025 15:51
@kakkoyun
Copy link
Member Author

Found 0 performance improvements and 0 performance regressions!

Interesting

This is because we haven't had that benchmark in the main before. I will split this PR into two and merge the benchmarking PR first so that we can have a baseline.

Great catch!

#4169

Copy link
Contributor

@kakkoyun Actually I was not thinking about this but about the other benchmarks in the package that should still be impacted by this. all the better if you separated this benchmark

@kakkoyun kakkoyun changed the title feat(stacktrace): optimize parseSymbol with zero-allocation implementation perf(stacktrace): optimize parseSymbol with zero-allocation implementation Nov 24, 2025
@kakkoyun
Copy link
Member Author

@kakkoyun Actually I was not thinking about this but about the other benchmarks in the package that should still be impacted by this. all the better if you separated this benchmark

We don't have this method called in any of the other benchmarks, AFAICT.

@kakkoyun kakkoyun marked this pull request as ready for review November 24, 2025 16:25
@kakkoyun kakkoyun marked this pull request as draft November 24, 2025 16:25
Copy link
Member

@darccio darccio left a comment

Choose a reason for hiding this comment

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

I like when we drop regexps 😁 LGTM. I assume this is covered by the current tests.

Base automatically changed from kakkoyun/use_stacktrace_package to main November 25, 2025 11:54
@kakkoyun kakkoyun marked this pull request as ready for review November 25, 2025 13:57
@kakkoyun
Copy link
Member Author

/merge

@dd-devflow-routing-codex
Copy link

dd-devflow-routing-codex bot commented Nov 25, 2025

View all feedbacks in Devflow UI.

2025-11-25 14:17:34 UTC ℹ️ Start processing command /merge


2025-11-25 14:17:43 UTC ℹ️ MergeQueue: waiting for PR to be ready

This pull request is not mergeable according to GitHub. Common reasons include pending required checks, missing approvals, or merge conflicts — but it could also be blocked by other repository rules or settings.
It will be added to the queue as soon as checks pass and/or get approvals.
Note: if you pushed new commits since the last approval, you may need additional approval.
You can remove it from the waiting list with /remove command.


2025-11-25 15:07:10 UTC ℹ️ MergeQueue: merge request added to the queue

The expected merge time in main is approximately 42m (p90).


2025-11-25 15:35:48 UTC ℹ️ MergeQueue: This merge request was merged

@kakkoyun
Copy link
Member Author

Benchmarks

Benchmark execution time: 2025-11-25 14:12:22

Comparing candidate commit 9843ec9 in PR branch kakkoyun/11-20-docs-document_testunit_make_target with baseline commit fa7d051 in branch main.

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

scenario:BenchmarkCaptureStackTrace/10-25

  • 🟩 allocated_mem [-834 bytes; -827 bytes] or [-13.621%; -13.505%]
  • 🟩 allocations [-6; -6] or [-50.000%; -50.000%]
  • 🟩 execution_time [-2.184µs; -2.159µs] or [-19.577%; -19.352%]

scenario:BenchmarkCaptureStackTrace/100-25

  • 🟩 allocated_mem [-1.836KB; -1.797KB] or [-3.849%; -3.768%]
  • 🟩 allocations [-6; -6] or [-50.000%; -50.000%]
  • 🟩 execution_time [-2.820µs; -2.753µs] or [-7.205%; -7.033%]

scenario:BenchmarkCaptureStackTrace/20-25

  • 🟩 allocated_mem [-928 bytes; -921 bytes] or [-8.626%; -8.561%]
  • 🟩 allocations [-6; -6] or [-50.000%; -50.000%]
  • 🟩 execution_time [-2.327µs; -2.299µs] or [-16.160%; -15.963%]

scenario:BenchmarkCaptureStackTrace/200-25

  • 🟩 allocated_mem [-2.560KB; -2.483KB] or [-2.621%; -2.542%]
  • 🟩 allocations [-6; -6] or [-50.000%; -50.000%]
  • 🟩 execution_time [-3.367µs; -3.224µs] or [-4.830%; -4.625%]

scenario:BenchmarkCaptureStackTrace/50-25

  • 🟩 allocated_mem [-1.342KB; -1.321KB] or [-5.494%; -5.410%]
  • 🟩 allocations [-6; -6] or [-50.000%; -50.000%]
  • 🟩 execution_time [-2.520µs; -2.470µs] or [-10.529%; -10.318%]

cc @eliottness

@dd-mergequeue dd-mergequeue bot merged commit c0d4c16 into main Nov 25, 2025
314 of 318 checks passed
@dd-mergequeue dd-mergequeue bot deleted the kakkoyun/11-20-docs-document_testunit_make_target branch November 25, 2025 15:35
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.

4 participants