-
Notifications
You must be signed in to change notification settings - Fork 10
Implement 64-bit trace ID system with double-buffered storage and liveness tracking #262
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.
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
Conversation
55f7fa6 to
98d82be
Compare
98d82be to
0e22d49
Compare
0e22d49 to
0ff5e12
Compare
0ff5e12 to
233747e
Compare
233747e to
32c8306
Compare
32c8306 to
ed00356
Compare
360fcb4 to
9bad2a2
Compare
9bad2a2 to
3957819
Compare
3957819 to
55050db
Compare
55050db to
c96dc57
Compare
1 similar comment
da6a866 to
f36b049
Compare
1 similar comment
c314d52 to
53be30c
Compare
bb1865b to
f3cd240
Compare
There was a problem hiding this 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 implements a comprehensive 64-bit trace ID system with double-buffered storage and liveness tracking for call traces. The changes enhance call trace management by adding liveness-aware preservation across JFR dumps, contention handling with visibility into dropped traces, and a modular hash table architecture with instance-based collision avoidance.
- Adds liveness-aware double-buffered storage with selective trace preservation
- Implements 64-bit trace ID system with instance-based collision avoidance
- Introduces contention handling with dropped trace visibility in JFR output
- Extracts dedicated CallTraceHashTable class for improved modularity
Reviewed Changes
Copilot reviewed 21 out of 22 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| LivenessTrackingTest.java | Comprehensive Java integration test for end-to-end liveness tracking validation |
| TagContextTest.java | Enhanced test with dropped sample tracking and counter validation improvements |
| ContendedCallTraceStorageTest.java | New test for measuring and validating contention in CallTraceStorage operations |
| test_callTraceStorage.cpp | Extensive C++ unit tests covering liveness scenarios and concurrent operations |
| wallClock.h/cpp | Updated copyright headers and 64-bit trace ID migration |
| thread.h | Updated trace ID fields and methods to use 64-bit values |
| profiler.h/cpp | Major refactoring for 64-bit trace IDs and liveness checker integration |
| objectSampler.cpp | Updated copyright and 64-bit trace ID adoption |
| livenessTracker.h/cpp | Enhanced with 64-bit trace IDs and self-registration with profiler |
| flightRecorder.h/cpp | Updated for 64-bit trace ID support and improved trace processing |
| counters.h | Added new counter for tracking dropped traces |
| callTraceStorage.h/cpp | Major refactoring with double-buffering and liveness integration |
| callTraceHashTable.h/cpp | New dedicated hash table implementation extracted from storage |
| arch_dd.h | Added COMMA macro consolidation |
| CLAUDE.md | New documentation file with project guidance and architecture details |
Comments suppressed due to low confidence (1)
ddprof-lib/src/main/cpp/callTraceStorage.cpp:125
- Copying the entire unordered_set could be expensive for large sets. Consider using std::move or passing the set by reference to avoid the copy overhead, especially since this is in the critical processTraces path.
preserve_set = _preserve_set; // Copy the set for lock-free processing
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
ddprof-test/src/test/java/com/datadoghq/profiler/memleak/LivenessTrackingTest.java
Show resolved
Hide resolved
ddprof-test/src/test/java/com/datadoghq/profiler/ContendedCallTraceStorageTest.java
Outdated
Show resolved
Hide resolved
2 similar comments
435eb08 to
2e8ab12
Compare
zhengyu123
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the second parameters for __atomic_compare_exchange_n() were used correctly. They should be values, not addresses to the value. Please check
2e8ab12 to
45eef79
Compare
Major architectural changes: - Replace monolithic CallTraceStorage with double-buffered hash table design - Add CallTraceHashTable with lock-free concurrent access and instance-based trace IDs - Implement liveness tracking system to preserve active traces across JFR dumps - Add dropped trace handling for lock contention with proper JFR integration Key features: - 64-bit trace IDs combining instance ID and slot for collision avoidance - Split-lock strategy minimizing exclusive lock time during trace collection - Platform-specific ASGCT_CallFrame alignment using LP64_ONLY macro - Comprehensive test coverage including contention and liveness scenarios 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
…essTrackingTest.java Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…TraceStorageTest.java Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
45eef79 to
56d1e55
Compare
What does this PR do?:
This PR implements a liveness-aware double-buffered call trace storage system with several key improvements:
CallTraceHashTableclassMotivation:
The changes address critical issues in call trace management:
Additional Notes:
Key Features:
Liveness-Aware Storage:
processTraces()methodContention Handling:
<dropped due to contention>shown in JFR stack traces instead of null entriesDouble-Buffered Architecture:
(instance_id << 32) | slotpreventing collisionsHash Table Improvements:
CallTraceHashTableclass (441 lines)Implementation Details:
Core Storage Refactoring:
CallTraceStoragereduced from 265→142 lines through hash table extraction64-bit Trace ID Migration:
recordJVMTISample(),recordSample(),recordDeferredSample()LivenessTrackerfor 64-bit trace ID handlingPlatform Compatibility:
arch_dd.hfor consistent designated initializer syntaxNew Files:
callTraceHashTable.{h,cpp}- Dedicated hash table implementation (441 lines)test_callTraceStorage.cpp- Comprehensive unit tests with liveness scenarios (387 lines)LivenessTrackingTest.java- Java integration test for end-to-end validation (246 lines)ContendedCallTraceStorageTest.java- Contention measurement and validation test (249 lines)Modified Files:
profiler.{h,cpp},objectSampler.cpp,wallClock.{h,cpp}- 64-bit trace ID adoptioncallTraceStorage.{h,cpp}- major refactoring with liveness integrationflightRecorder.{h,cpp}- 64-bit trace ID support and dropped trace handlinglivenessTracker.{h,cpp}- 64-bit trace ID migrationarch_dd.h- COMMA macro consolidationHow to test the change?:
The implementation includes extensive test coverage:
CallTraceStorageliveness scenariosFor Datadog employees:
@DataDog/security-design-and-guidance.Summary: +1689 lines, -447 lines (net +1239 lines)
This implementation provides a robust foundation for liveness-aware profiling with clear visibility into contention issues while maintaining high performance through lock-free operations and efficient storage management.