fix(profiler): five native profiler bug fixes#8533
Merged
Conversation
BenchmarksBenchmark execution time: 2026-04-29 16:33:57 Comparing candidate commit 3149a14 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 27 metrics, 0 unstable metrics, 59 known flaky benchmarks, 28 flaky benchmarks without significant changes.
|
Execution-Time Benchmarks Report ⏱️Execution-time results for samples comparing This PR (8533) and master. ✅ No regressions detected - check the details below Full Metrics ComparisonFakeDbCommand
HttpMessageHandler
Comparison explanationExecution-time benchmarks measure the whole time it takes to execute a program, and are intended to measure the one-off costs. Cases where the execution time results for the PR are worse than latest master results are highlighted in **red**. The following thresholds were used for comparing the execution times:
Note that these results are based on a single point-in-time result for each branch. For full results, see the dashboard. Graphs show the p99 interval based on the mean and StdDev of the test run, as well as the mean value of the run (shown as a diamond below the graph). Duration chartsFakeDbCommand (.NET Framework 4.8)gantt
title Execution time (ms) FakeDbCommand (.NET Framework 4.8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8533) - mean (76ms) : 71, 80
master - mean (76ms) : 72, 79
section Bailout
This PR (8533) - mean (79ms) : 75, 83
master - mean (79ms) : 75, 82
section CallTarget+Inlining+NGEN
This PR (8533) - mean (1,089ms) : 1016, 1162
master - mean (1,081ms) : 1024, 1138
FakeDbCommand (.NET Core 3.1)gantt
title Execution time (ms) FakeDbCommand (.NET Core 3.1)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8533) - mean (116ms) : 108, 123
master - mean (118ms) : 112, 125
section Bailout
This PR (8533) - mean (114ms) : 112, 117
master - mean (115ms) : 112, 118
section CallTarget+Inlining+NGEN
This PR (8533) - mean (780ms) : 757, 804
master - mean (780ms) : 755, 805
FakeDbCommand (.NET 6)gantt
title Execution time (ms) FakeDbCommand (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8533) - mean (105ms) : 98, 111
master - mean (104ms) : 98, 111
section Bailout
This PR (8533) - mean (104ms) : 100, 108
master - mean (103ms) : 98, 108
section CallTarget+Inlining+NGEN
This PR (8533) - mean (942ms) : 910, 974
master - mean (939ms) : 900, 977
FakeDbCommand (.NET 8)gantt
title Execution time (ms) FakeDbCommand (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8533) - mean (102ms) : 97, 107
master - mean (101ms) : 96, 105
section Bailout
This PR (8533) - mean (106ms) : 101, 111
master - mean (105ms) : 99, 110
section CallTarget+Inlining+NGEN
This PR (8533) - mean (825ms) : 778, 872
master - mean (824ms) : 783, 864
HttpMessageHandler (.NET Framework 4.8)gantt
title Execution time (ms) HttpMessageHandler (.NET Framework 4.8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8533) - mean (205ms) : 193, 218
master - mean (204ms) : 194, 214
section Bailout
This PR (8533) - mean (208ms) : 198, 218
master - mean (207ms) : 199, 215
section CallTarget+Inlining+NGEN
This PR (8533) - mean (1,222ms) : 1172, 1273
master - mean (1,208ms) : 1156, 1259
HttpMessageHandler (.NET Core 3.1)gantt
title Execution time (ms) HttpMessageHandler (.NET Core 3.1)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8533) - mean (295ms) : 276, 313
master - mean (293ms) : 280, 306
section Bailout
This PR (8533) - mean (295ms) : 278, 312
master - mean (294ms) : 283, 305
section CallTarget+Inlining+NGEN
This PR (8533) - mean (974ms) : 943, 1005
master - mean (966ms) : 929, 1002
HttpMessageHandler (.NET 6)gantt
title Execution time (ms) HttpMessageHandler (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8533) - mean (289ms) : 274, 304
master - mean (286ms) : 272, 299
section Bailout
This PR (8533) - mean (293ms) : 275, 310
master - mean (286ms) : 273, 299
section CallTarget+Inlining+NGEN
This PR (8533) - mean (1,166ms) : 1128, 1204
master - mean (1,158ms) : 1114, 1202
HttpMessageHandler (.NET 8)gantt
title Execution time (ms) HttpMessageHandler (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8533) - mean (287ms) : 273, 301
master - mean (283ms) : 273, 292
section Bailout
This PR (8533) - mean (289ms) : 272, 306
master - mean (281ms) : 273, 289
section CallTarget+Inlining+NGEN
This PR (8533) - mean (1,055ms) : 987, 1124
master - mean (1,040ms) : 991, 1089
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
r1viollet
marked this pull request as draft
April 28, 2026 14:17
r1viollet
force-pushed
the
r1viollet/profiler-bug-fixes-followup
branch
from
April 28, 2026 14:33
69b640a to
b0e719b
Compare
r1viollet
commented
Apr 28, 2026
| bool _canReplaceSignalHandler; | ||
| int32_t _signalToSend; | ||
| HandlerFn_t _handler; | ||
| std::atomic<HandlerFn_t> _handler; |
Collaborator
Author
There was a problem hiding this comment.
@gleocadie I realize that this was not an atomic, so re-reading the current had no effect (compiler optimized it away)
r1viollet
marked this pull request as ready for review
April 28, 2026 15:00
r1viollet
marked this pull request as draft
April 28, 2026 15:02
_probability (double) and _samplesBudget (int64_t) were declared volatile but written by the timer thread in RollWindow() while being read concurrently by Sample() on arbitrary CLR threads. C++ volatile provides no memory-model guarantees: no happens-before, no atomicity, no ordering. The compiler is free to reorder or cache these accesses, and on ARM64 a 64-bit non-atomic store is not guaranteed to be read-consistent by other cores. TSAN detects this as a data race. Replace both fields with std::atomic<> using relaxed ordering. The existing comment in RollWindow() acknowledges intentional looseness in the count/budget window accounting; relaxed atomics preserve that intent while eliminating the formal undefined behaviour and ARM64 correctness risk. Add ConcurrentSampleAndRollWindow_NoDataRace test: 4 threads calling Sample() while a writer thread calls RollWindow() in a tight loop. With the volatile code this reliably triggers a TSAN report; with the fix it is clean. Co-Authored-By: Claude Sonnet 4.6 (1M context) <[email protected]>
…ager
RegisterHandler() used a classic DCL pattern but the inner guard (after
acquiring the mutex) checked the stale pre-lock local `current` instead of
re-reading `_handler`:
HandlerFn_t current = _handler; // read without lock
if (current != nullptr) return;
lock.acquire();
if (current != nullptr) return; // BUG: stale, always nullptr here
Two threads that both observe _handler==nullptr before the lock race to
acquire the mutex sequentially. The second caller's inner check still sees
the pre-lock value (nullptr) and calls SetupSignalHandler() a second time.
That second sigaction() call saves the profiler's own handler into
_previousAction; on teardown UnRegisterHandler() then restores the profiler
handler instead of the original one, creating an infinite signal chain.
Fix: read _handler again inside the mutex.
Co-Authored-By: Claude Sonnet 4.6 (1M context) <[email protected]>
AddContentionSample() declared `static uint64_t failureCount` and then both read and incremented it from multiple concurrent CLR callback threads (the synchronous ICorProfilerCallback path and the async ETW path run on different threads simultaneously). This is a plain C++ data race. Replace with `static std::atomic<uint64_t>` and use fetch_add so that the threshold check and increment are a single atomic operation, avoiding the secondary race between the modulo check and the increment. Co-Authored-By: Claude Sonnet 4.6 (1M context) <[email protected]>
GetCodeInfo2 can theoretically return a code range with size==0 (e.g. during
tier-transition races or for degenerate JIT stubs). GetCodeRanges computed:
endAddress = startAddress + size - 1
Without a size==0 guard this underflows. For startAddress==0 it produces
endAddress==UINTPTR_MAX, and the subsequent page-insertion loop:
for (page = startPage; page <= endPage; ++page)
iterates from 0 to ~UINTPTR_MAX/pageSize (billions of iterations), hanging
the background worker thread permanently and starving IsManaged() callers.
For non-zero startAddress the loop is silently a no-op (endPage < startPage)
but the degenerate CodeRange still pollutes the sorted range vector.
Fix: skip entries where size==0 before constructing the CodeRange.
Add two regression tests:
- ZeroSizeRange_DoesNotPolluteCacheNonZeroBase: verifies IsManaged returns
false for all IPs near a zero-size registration.
- ZeroSizeRangeAtAddressZero_WorkerDoesNotHang: verifies the background
worker completes within the normal timeout (detects the hang path).
The previous test AddFunction_ZeroSizeRange_HandledGracefully did `(void)result`
with no assertion — it was replaced by the two tests above.
Co-Authored-By: Claude Sonnet 4.6 (1M context) <[email protected]>
…RegisterHandler Restyle the DCL fix to keep `current` as the sole variable checked throughout the function, making the intent clearer: after acquiring the mutex, refresh `current` from `_handler` so the inner guard sees any write that a racing thread may have completed between the outer check and the lock acquisition. Co-Authored-By: Claude Sonnet 4.6 (1M context) <[email protected]>
_handler is written by RegisterHandler() (main thread, under mutex) and read concurrently from the signal handler via CallCustomHandler() and from the pre-lock fast path in RegisterHandler() — both without any lock. A plain pointer gives no memory-model guarantees: the compiler may cache it in a register and the CPU may observe a stale value on ARM64. Make _handler std::atomic<HandlerFn_t>: - load(memory_order_acquire) at every read point, including the pre-lock fast path and the re-read after acquiring the mutex. - store(memory_order_release) at every write point. The acquire/release pair ensures that Thread B's post-lock re-read is guaranteed to observe Thread A's store, giving the DCL its required happens-before edge without relying on the mutex flush as a side effect. Co-Authored-By: Claude Sonnet 4.6 (1M context) <[email protected]>
r1viollet
force-pushed
the
r1viollet/profiler-bug-fixes-followup
branch
from
April 28, 2026 15:20
b0e719b to
35c3174
Compare
r1viollet
marked this pull request as ready for review
April 29, 2026 14:51
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 35c31742d4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| { | ||
| // avoid the case where the ClrStackWalk event has been missed for the ContentionStart | ||
| if ((stack.size() == 0) && (failureCount % 1000 == 0)) | ||
| if ((stack.size() == 0) && (failureCount.fetch_add(1) % 1000 == 0)) |
There was a problem hiding this comment.
When a CLR contention event arrives without a stack, this condition increments the counter but only returns on failures where the previous count is divisible by 1000. The other 999 failures fall through and call Add() with an empty rawSample.Stack, even though this block is meant to drop missed stack walks; the same fetch_add() % 1000 pattern above has the same effect for synchronous stack-walk failures. Keep the log rate-limited, but return on every empty-stack failure.
Useful? React with 👍 / 👎.
The previous condition combined the empty-stack check with the rate-limited log check using &&, so the early return only fired on every 1000th failure. The other 999 calls fell through and passed an empty rawSample.Stack to Add(), emitting a corrupt sample with no frames. Separate the two concerns: always return when the stack is empty, and only log every 1000 occurrences. Applies to both the synchronous stack-walk path and the async ETW path. Co-Authored-By: Claude Sonnet 4.6 (1M context) <[email protected]>
r1viollet
force-pushed
the
r1viollet/profiler-bug-fixes-followup
branch
from
April 29, 2026 15:22
6272369 to
3149a14
Compare
r1viollet
commented
Apr 29, 2026
| uint32_t hrCollectStack = E_FAIL; | ||
| const auto result = pStackFramesCollector->CollectStackSample(threadInfo.get(), &hrCollectStack); | ||
| if ((result->GetFramesCount() == 0) && (failureCount % 1000 == 0)) | ||
| if (result->GetFramesCount() == 0) |
Collaborator
Author
There was a problem hiding this comment.
@gleocadie I changed this part (from feedback of codex), I think it is better ?
r1viollet
changed the title
r1viollet
changed the title
r1viollet
enabled auto-merge (squash)
April 29, 2026 15:35
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
3 participants
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.
Five correctness bugs found by static analysis of the native profiler.
AdaptiveSampler:volatile→std::atomic_probabilityand_samplesBudgetwerevolatilebut written by the timer thread and read concurrently bySample(). No memory-model guarantees; TSAN-visible data race, torn reads on ARM64.ProfilerSignalManager: atomic_handler+ DCL fix_handlerwas a plain pointer read from the signal handler thread (inCallCustomHandler) without any lock — a data race. Madestd::atomic<HandlerFn_t>with acquire/release ordering.Also fixed the double-checked locking: the inner guard after acquiring the mutex was checking a stale pre-lock local instead of re-reading
_handler, allowing two racing threads to both callSetupSignalHandler()and corrupt_previousAction.ContentionProvider: atomicfailureCount+ always drop empty-stack samplesfailureCountwas a non-atomic static local incremented from concurrent CLR callback threads. Also: the empty-stack early-return was gated onfailureCount % 1000 == 0, so 999 out of every 1000 empty-stack failures fell through and emitted a corrupt zero-frame sample. The return is now unconditional; only the log is rate-limited.ManagedCodeCache: skip zero-size code rangesGetCodeInfo2entries withsize == 0producedendAddress = startAddress - 1(underflow). ForstartAddress == 0this givesendAddress = UINTPTR_MAXand the page-insertion loop runs forever, hanging the background worker. Guard added; JIT callbacks also now checkSUCCEEDED(hrStatus)before callingAddFunction.