Fix WallClockASGCT::timerLoop skipping every other thread (PROF-14332)#493
Merged
Fix WallClockASGCT::timerLoop skipping every other thread (PROF-14332)#493
Conversation
Remove the spurious extra ThreadList::next() call in the no-filter fallback branch of collectThreads. The loop already reads one tid per iteration via the hasNext()/next() pattern; the trailing next() advanced the cursor a second time, so tids was populated with only the 1st, 3rd, 5th, ... entries. Impact: when threadFilter->enabled() is false, wallclock ticks sampled only half of eligible threads. The context-filtered path is unaffected. JIRA: PROF-14332 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes an iteration bug in WallClockASGCT::timerLoop()’s “no thread filter” fallback path where ThreadList::next() was being called twice per loop iteration, causing every other thread to be skipped when collecting tids for wallclock sampling.
Changes:
- Remove a redundant
thread_list->next()call inside thethreadFilter->enabled() == falsebranch ofWallClockASGCT::timerLoop(). - Ensure the fallback
while (thread_list->hasNext()) { int tid = thread_list->next(); ... }pattern consumes exactly one thread id per iteration.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
CI Test ResultsRun: #24785852534 | Commit:
Status Overview
Legend: ✅ passed | ❌ failed | ⚪ skipped | 🚫 cancelled Summary: Total: 32 | Passed: 32 | Failed: 0 Updated: 2026-04-23 15:05:12 UTC |
![dd-octo-sts[bot]](https://avatars.githubusercontent.com/in/1157446?s=60&v=4){kind=small}
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?:
Fixes a bug in
WallClockASGCT::timerLoopwhere thecollectThreadslambda's no-filter fallback advances theThreadListiterator twice per loop body, silently skipping every other thread when the wallclock thread filter is disabled.The fix removes one spurious
tid = thread_list->next();call at the tail of the loop inddprof-lib/src/main/cpp/wallClock.cpp. Thewhile (thread_list->hasNext()) { tid = thread_list->next(); ... }pattern now correctly reads one tid per iteration.Motivation:
When
threadFilter->enabled()is false (no context-based wallclock filtering), every wallclock tick was sampling only half the eligible threads, producing under-sampled wallclock profiles. The typical context-filtered path is unaffected; this only bit on the fallback.Flagged by a muse chorus review as an unrelated pre-existing bug and filed separately so it can land on its own small PR.
Additional Notes:
Pre-existing bug — not introduced by any recent change. Scope is intentionally narrow: only the redundant
next()call is removed. Related concerns (e.g.ThreadList::next()lacking an internal bounds check for misuse) are pre-existing API issues and out of scope for this PR.How to test the change?:
Manual validation: run the wallclock profiler with the context-based thread filter disabled and confirm all eligible threads appear in the sampled output rather than every other one. The change is a 1-line deletion inside the
threadFilter->enabled() == falsebranch; the filter-enabled path is untouched.For Datadog employees:
@DataDog/security-design-and-guidance.🤖 Generated with Claude Code via muse implement