perf(telemetry): serialize batches on shared thread pool - #1946
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1946 +/- ##
==========================================
+ Coverage 74.31% 74.62% +0.31%
==========================================
Files 104 104
Lines 25738 26179 +441
Branches 4648 4742 +94
==========================================
+ Hits 19126 19535 +409
- Misses 5305 5308 +3
- Partials 1307 1336 +29 🚀 New features to boost your workflow:
|
ae3b36a to
53a8c3e
Compare
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
53a8c3e to
e8103ca
Compare
e8103ca to
af6d19c
Compare
7e20632 to
ccf9950
Compare
ccf9950 to
a5e9fef
Compare
a5e9fef to
bd10425
Compare
bd10425 to
9cb4261
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 9cb4261. Configure here.
261cfe1 to
1134ee2
Compare
1134ee2 to
ba1a0c2
Compare
jpnurmi
changed the title
c068550 to
f908079
Compare
|
Update: I'm investigating why TSAN jobs started hanging in the CI after the latest rebase and |
3d7e429 to
089103a
Compare
Revert 219d934, which promoted the page allocator spinlock into a shared primitive. The page allocator and telemetry batcher require different locking semantics and should not share this implementation.
Let the telemetry lifecycle own a shared serialization pool for enabled telemetry batchers. Use the pool for log and metric batch serialization while keeping completion ordered through the batcher flush lifecycle.
089103a to
2f2e296
Compare
The problem turned out to be the shared spinlock promoted in #1964. 😢 Apparently, the Unix page allocator and telemetry batcher require different locking semantics and should not share the same implementation. Thus, I've taken a step back and reverted it in c64b850. The CI is back to green. |
limbonaut
left a comment
limbonaut
left a comment
There was a problem hiding this comment.
Super well-polished. This should increase batching throughput quite noticeably. As far as I understood the workings, I didn't spot any issues except one narrow race between closing and crashing paths (see comments). Looks good otherwise!
| lock_tasks(batcher); | ||
| const bool done = batcher->tasks == NULL; | ||
| unlock_tasks(batcher); | ||
| if (done) { | ||
| return; | ||
| } | ||
| sentry__cpu_relax(); |
There was a problem hiding this comment.
Q: Should we wait on a condvar here or use crash_safe_sleep_ms? Or do we expect this to be a relatively short duration? This part is not on a crash path if I'm not mistaken.
There was a problem hiding this comment.
Resulted in a small side quest:
Wake task waiters when the final in-flight task is removed instead of continuously polling the task list. Document the crash-flush race that can leave a worker with no task to execute.
Keep lock-free crash flush access alive until shutdown detaches the batcher and all active pins are released.
3 participants
Let the telemetry module own a shared thread pool for telemetry batchers, and use it for log and metric batch serialization.
Before: on the left, the batcher thread is unable to keep up with serializing incoming logs before handing them off to the transport thread. Serialization takes too much time, queues fill up, logs overflow, and get discarded. The batcher thread continues working for a good while after logs stop coming in.
After: on the right, the batcher thread only acts as a gateway for incoming logs. It has no problem keeping up with the flood because they are only quickly organized into batches and immediately handed over to the telemetry pool, which then distributes the heavy serialization work to its pooled threads, resulting in no discarded logs.
before-vs-after.ftrace.zip
A partial, batcher-side fix to: