Use sigsetjmp/siglongjmp instead of setjmp/longjmp#686
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR aims to make HotSpot crash-protection jumps signal-safe by migrating from setjmp/longjmp to sigsetjmp/siglongjmp in the native stack-walking fault-recovery path.
Changes:
- Switched the per-thread stored jump context type from
jmp_buf*tosigjmp_buf*. - Updated HotSpot fault recovery to use
siglongjmp()when unwinding from the SEGV handler.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
ddprof-lib/src/main/cpp/threadLocalData.h |
Stores the crash-protection jump context as sigjmp_buf* instead of jmp_buf*. |
ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp |
Uses sigjmp_buf for the crash-protection context and siglongjmp() for recovery. |
Comments suppressed due to low confidence (1)
ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp:258
- This comment references setjmp/longjmp, but the code now uses sigjmp_buf/siglongjmp. Update the wording to match the new control-flow mechanism.
// Should be preserved across setjmp/longjmp
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
CI Test ResultsRun: #30256437751 | Commit:
Status Overview
Legend: ✅ passed | ❌ failed | ⚪ skipped | 🚫 cancelled Summary: Total: 32 | Passed: 32 | Failed: 0 Updated: 2026-07-27 10:42:44 UTC |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (4)
ddprof-lib/src/test/cpp/hotspot_crash_protection_ut.cpp:17
- The header comment still refers to
jmp_buf*andlongjmp()even though the implementation has been switched tosigsetjmp/siglongjmp. This is misleading (and suggests the wrong buffer type is stored on ProfiledThread).
This issue also appears in the following locations of the same file:
- line 283
- line 294
* Crash recovery inside walkVM relies on sigsetjmp/siglongjmp:
* 1. walkVM stores a jmp_buf* on ProfiledThread (setJmpCtx/getJmpCtx),
* chaining it with whatever context was already installed so a
* signal-based sampler interrupting a non-signal-based sampler's own
* in-flight walkVM() call doesn't clobber the outer call's context.
ddprof-lib/src/test/cpp/hotspot_crash_protection_ut.cpp:298
- This test now uses
sigsetjmp/siglongjmp, but the jump buffers are still declared asjmp_buf/jmp_buf*. Usesigjmp_bufconsistently to avoid relying on platform typedef quirks and to match the production API (ProfiledThread::setJmpCtx(sigjmp_buf*)). Also update the nearby comments/FAIL message to mentionsiglongjmp.
if (sigsetjmp(outer_ctx, 1) != 0) {
outer_landed++;
} else {
_pt->setJmpCtx(&outer_ctx);
ddprof-lib/src/test/cpp/faultInjection_ut.cpp:150
- The test still uses
jmp_buf+setjmp, but the crash-protection path has moved tosigsetjmp/siglongjmp. Mixingsetjmpwithsiglongjmpis undefined/implementation-specific. Update the buffer type and call tosigjmp_buf/sigsetjmp, and rename the test to avoid implying the non-signal variant is used.
// (c2) walkVM path: a raw dereference of an injected poison pointer must be
// caught by the sigsetjmp/siglongjmp crash protection, returning control to setjmp.
TEST_F(FaultInjectionTest, WalkVmSetjmpRecoversFromInjectedFault) {
ProfiledThread* t = ProfiledThread::current();
ASSERT_NE(t, nullptr);
ddprof-lib/src/test/cpp/hotspot_crash_protection_ut.cpp:286
- Comment still says
checkFault()"longjmps" even though the code path is nowsiglongjmp. Update wording to match the behavior being tested.
// End-to-end with real sigsetjmp/siglongjmp: a fault inside the inner frame must
// land in the inner frame's own recovery branch — checkFault() always
// longjmps through whatever is currently installed — and once the inner
// frame has recovered and restored the outer's context, the outer frame must
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (5)
ddprof-lib/src/test/cpp/hotspot_crash_protection_ut.cpp:17
- The documentation here still references
jmp_buf/longjmp, butProfiledThread::setJmpCtx/getJmpCtxnow usesigjmp_bufand recovery usessiglongjmp. This is misleading and also risks type mismatches on platforms wherejmp_bufandsigjmp_bufdiffer.
This issue also appears in the following locations of the same file:
- line 14
- line 283
- line 309
* Crash recovery inside walkVM relies on sigsetjmp/siglongjmp:
* 1. walkVM stores a jmp_buf* on ProfiledThread (setJmpCtx/getJmpCtx),
* chaining it with whatever context was already installed so a
* signal-based sampler interrupting a non-signal-based sampler's own
* in-flight walkVM() call doesn't clobber the outer call's context.
ddprof-lib/src/test/cpp/hotspot_crash_protection_ut.cpp:302
jmp_buf/jmp_buf*are still used here withsigsetjmp/siglongjmp. SinceProfiledThread::setJmpCtx/getJmpCtxnow usesigjmp_buf, this can break compilation/portability on platforms wherejmp_bufandsigjmp_bufare distinct types.
// End-to-end with real sigsetjmp/siglongjmp: a fault inside the inner frame must
// land in the inner frame's own recovery branch — checkFault() always
// longjmps through whatever is currently installed — and once the inner
// frame has recovered and restored the outer's context, the outer frame must
// be left exactly as it was, never having been unwound itself.
TEST_F(JmpCtxChainingTest, FaultInInnerFrameDoesNotDisturbOuterFrame) {
jmp_buf outer_ctx;
jmp_buf* outer_prev = _pt->getJmpCtx();
int outer_landed = 0;
int inner_landed = 0;
if (sigsetjmp(outer_ctx, 1) != 0) {
outer_landed++;
} else {
_pt->setJmpCtx(&outer_ctx);
// --- inner "walkVM" call, interrupted mid-flight by a fault ---
jmp_buf inner_ctx;
jmp_buf* inner_prev = _pt->getJmpCtx();
ASSERT_EQ(&outer_ctx, inner_prev);
ddprof-lib/src/test/cpp/hotspot_crash_protection_ut.cpp:312
- This block still refers to
longjmpin comments/error text, but the code path usessiglongjmp. Keeping wording consistent makes failures easier to interpret.
// Simulate checkFault(): longjmp through whatever is currently
// installed — this must hit the inner frame, not the outer.
siglongjmp(*_pt->getJmpCtx(), 1);
FAIL() << "unreachable: longjmp does not return";
ddprof-lib/src/test/cpp/faultInjection_ut.cpp:162
- Later in this test (outside the diff hunk) the comments and assertion message still say setjmp/longjmp. Since the code path now uses sigsetjmp/siglongjmp, update that wording to avoid confusion when diagnosing failures.
sigjmp_buf ctx;
if (sigsetjmp(ctx) != 0) {
recovered = true; // returned here via checkFault -> siglongjmp
faults++;
}
ddprof-lib/src/test/cpp/hotspot_crash_protection_ut.cpp:16
ProfiledThread::setJmpCtx/getJmpCtxnow take/returnsigjmp_buf*, but several tests earlier in this file still declarejmp_bufand passjmp_buf*to these APIs (e.g., SetAndGetRoundTrip / SingleFrameRestoresPreviousOnExit / NestedFrames…). That can fail to compile on platforms wherejmp_bufandsigjmp_bufare distinct types; consider switching those remainingjmp_bufdeclarations/comments tosigjmp_buffor consistency/portability.
* 1. walkVM stores a jmp_buf* on ProfiledThread (setJmpCtx/getJmpCtx),
* chaining it with whatever context was already installed so a
* signal-based sampler interrupting a non-signal-based sampler's own
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (3)
ddprof-lib/src/test/cpp/faultInjection_ut.cpp:162
- sigsetjmp requires the
savesigsargument (typically 1). As written, this call will not compile (or will call the wrong overload/macro) on most platforms.
sigjmp_buf ctx;
if (sigsetjmp(ctx, 1) != 0) {
recovered = true; // returned here via checkFault -> siglongjmp
faults++;
}
ddprof-lib/src/test/cpp/faultInjection_ut.cpp:149
- The test name and comment still refer to setjmp, but the implementation now uses sigsetjmp/siglongjmp. Renaming keeps intent and failure output accurate.
// (c2) walkVM path: a raw dereference of an injected poison pointer must be
// caught by the sigsetjmp/siglongjmp crash protection, returning control to setjmp.
TEST_F(FaultInjectionTest, WalkVmSetjmpRecoversFromInjectedFault) {
ProfiledThread* t = ProfiledThread::current();
ddprof-lib/src/test/cpp/hotspot_crash_protection_ut.cpp:302
- Same issue for the inner frame: use
sigjmp_bufconsistently so sigsetjmp/siglongjmp and setJmpCtx/getJmpCtx all agree on the buffer type.
jmp_buf inner_ctx;
jmp_buf* inner_prev = _pt->getJmpCtx();
ASSERT_EQ(&outer_ctx, inner_prev);
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (5)
ddprof-lib/src/test/cpp/hotspot_crash_protection_ut.cpp:20
- The header comment was updated to mention sigsetjmp/siglongjmp, but it still refers to storing a jmp_buf* and calling longjmp(). This is now inconsistent with the implementation (ProfiledThread uses sigjmp_buf* and checkFault uses siglongjmp), which makes the documentation misleading.
* Crash recovery inside walkVM relies on sigsetjmp/siglongjmp:
* 1. walkVM stores a jmp_buf* on ProfiledThread (setJmpCtx/getJmpCtx),
* chaining it with whatever context was already installed so a
* signal-based sampler interrupting a non-signal-based sampler's own
* in-flight walkVM() call doesn't clobber the outer call's context.
* 2. If a fault fires during the walk, checkFault() detects the live
* context via ProfiledThread::isProtected() and calls longjmp() to
* unwind through whatever context is currently installed.
ddprof-lib/src/test/cpp/hotspot_crash_protection_ut.cpp:312
- This comment and failure message still refer to longjmp, but the code now uses siglongjmp(). Updating them keeps the test intent clear and consistent with the new crash-protection mechanism.
// Simulate checkFault(): longjmp through whatever is currently
// installed — this must hit the inner frame, not the outer.
siglongjmp(*_pt->getJmpCtx(), 1);
FAIL() << "unreachable: longjmp does not return";
ddprof-lib/src/test/cpp/faultInjection_ut.cpp:147
- The comment says control returns to setjmp, but this test now uses sigsetjmp/siglongjmp. Update wording to match the actual mechanism being tested.
// (c2) walkVM path: a raw dereference of an injected poison pointer must be
// caught by the sigsetjmp/siglongjmp crash protection, returning control to setjmp.
ddprof-lib/src/test/cpp/hotspot_crash_protection_ut.cpp:294
- outer_ctx/outer_prev are declared as jmp_buf/jmp_buf*, but this path now uses sigsetjmp(…, 1) and ProfiledThread::setJmpCtx/getJmpCtx are sigjmp_buf*-based. On libcs where jmp_buf and sigjmp_buf differ, passing the wrong buffer type can be incorrect (and could even corrupt the stack if sigsetjmp writes a larger env). Prefer using sigjmp_buf consistently here.
if (sigsetjmp(outer_ctx, 1) != 0) {
ddprof-lib/src/test/cpp/hotspot_crash_protection_ut.cpp:304
- inner_ctx/inner_prev are declared as jmp_buf/jmp_buf* but are used with sigsetjmp(…, 1) and stored via ProfiledThread::setJmpCtx(sigjmp_buf*). Use sigjmp_buf consistently to match the sigsetjmp/siglongjmp mechanism and avoid libc-dependent type mismatches.
if (sigsetjmp(inner_ctx, 1) != 0) {
Contributor
Benchmark Results (commit 75d514d)Pipeline: https://gitlab.ddbuild.io/DataDog/apm-reliability/benchmarking-platform/-/pipelines/126927848 Commit: ✅ Within expected boundariesNo significant runtime deltas (all within run-to-run noise) and no internal-counter outliers. Runtime details (per benchmark × JDK)
Internal counter details (ddprof)ddprof internal counters, latest / dev (✅ = 0, · = unavailable):
|
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (6)
ddprof-lib/src/test/cpp/hotspot_crash_protection_ut.cpp:17
- The file header comment was partially updated to sigsetjmp/siglongjmp but still refers to
jmp_buf*andlongjmp(). This is now inconsistent with the code/API (sigjmp_buf*+siglongjmp()) and may confuse future maintenance.
* Crash recovery inside walkVM relies on sigsetjmp/siglongjmp:
* 1. walkVM stores a jmp_buf* on ProfiledThread (setJmpCtx/getJmpCtx),
* chaining it with whatever context was already installed so a
* signal-based sampler interrupting a non-signal-based sampler's own
* in-flight walkVM() call doesn't clobber the outer call's context.
ddprof-lib/src/test/cpp/hotspot_crash_protection_ut.cpp:287
- This comment still says
checkFault()"longjmps" even though the recovery mechanism is nowsiglongjmp(). Update the wording to match the implementation.
// End-to-end with real sigsetjmp/siglongjmp: a fault inside the inner frame must
// land in the inner frame's own recovery branch — checkFault() always
// longjmps through whatever is currently installed — and once the inner
// frame has recovered and restored the outer's context, the outer frame must
// be left exactly as it was, never having been unwound itself.
ddprof-lib/src/test/cpp/hotspot_crash_protection_ut.cpp:310
- The inline comment describes the simulated jump as
longjmp, but the code now callssiglongjmp(). Keeping the comment aligned helps avoid confusion when debugging these tests.
// Simulate checkFault(): longjmp through whatever is currently
// installed — this must hit the inner frame, not the outer.
ddprof-lib/src/test/cpp/hotspot_crash_protection_ut.cpp:292
ProfiledThread::getJmpCtx()now returnssigjmp_buf*, but this test file still contains earlier tests usingjmp_buf/jmp_buf*withsetJmpCtx/getJmpCtx(e.g., around theSetAndGetRoundTripandNestedFramesChainAndUnwindInOrdertests). Those will no longer compile cleanly and should be updated tosigjmp_buf/sigjmp_buf*consistently throughout the file.
sigjmp_buf outer_ctx;
sigjmp_buf* outer_prev = _pt->getJmpCtx();
int outer_landed = 0;
int inner_landed = 0;
ddprof-lib/src/test/cpp/faultInjection_ut.cpp:147
- This comment still says control returns to
setjmp, but the test now usessigsetjmp/siglongjmp. Update the wording for accuracy.
// caught by the sigsetjmp/siglongjmp crash protection, returning control to setjmp.
ddprof-lib/src/test/cpp/faultInjection_ut.cpp:163
- The test body now uses
sigsetjmp/siglongjmp, but the later comment and assertion message still saysetjmp/longjmp. Since this shows up directly in test output, it should be kept consistent with the updated recovery mechanism.
sigjmp_buf ctx;
if (sigsetjmp(ctx, 1) != 0) {
recovered = true; // returned here via checkFault -> siglongjmp
faults++;
}
t->setJmpCtx(&ctx);
Contributor
Benchmark Results (commit d2773a4)Pipeline: https://gitlab.ddbuild.io/DataDog/apm-reliability/benchmarking-platform/-/pipelines/126937456 Commit:
|
| Benchmark | JDK | Latest | Dev | Δ (dev vs latest) | Issues L/D |
|---|---|---|---|---|---|
| akka-uct | 21 | ✅ 10522 ms (21 iters) | ✅ 10247 ms (21 iters) | ≈ -2.6% (±11.3%) | — / — |
| akka-uct | 25 | ✅ 9035 ms (24 iters) | ✅ 8862 ms (24 iters) | ≈ -1.9% (±10.7%) | — / — |
| finagle-chirper | 21 | ✅ 5998 ms (33 iters) | ✅ 5954 ms (33 iters) | ≈ -0.7% (±24.5%) | |
| finagle-chirper | 25 | ✅ 5490 ms (36 iters) | ✅ 5511 ms (36 iters) | ≈ +0.4% (±24.7%) | |
| fj-kmeans | 21 | ✅ 2787 ms (67 iters) | ✅ 2853 ms (66 iters) | ≈ +2.4% (±2.6%) | — / — |
| fj-kmeans | 25 | ✅ 2813 ms (66 iters) | ✅ 2807 ms (66 iters) | ≈ -0.2% (±2.6%) | — / — |
| future-genetic | 21 | ✅ 2133 ms (87 iters) | ✅ 2077 ms (89 iters) | 🟢 -2.6% | — / — |
| future-genetic | 25 | ✅ 2080 ms (90 iters) | ✅ 1972 ms (95 iters) | 🟢 -5.2% | — / — |
| naive-bayes | 21 | ✅ 1245 ms (137 iters) | ✅ 1275 ms (134 iters) | ≈ +2.4% (±33.2%) | — / — |
| naive-bayes | 25 | ✅ 979 ms (174 iters) | ✅ 1013 ms (169 iters) | ≈ +3.5% (±32.2%) | — / — |
| reactors | 21 | ✅ 16261 ms (15 iters) | ✅ 16277 ms (15 iters) | ≈ +0.1% (±8.2%) | — / — |
| reactors | 25 | ✅ 18391 ms (15 iters) | ✅ 18398 ms (15 iters) | ≈ +0% (±3%) | — / — |
Internal counter details (ddprof)
ddprof internal counters, latest / dev (✅ = 0, · = unavailable):
| Benchmark | JDK | Dropped rec | Dropped jvmti | Dropped trace | Skipped WC | AGCT fail | Unwind fail |
|---|---|---|---|---|---|---|---|
| akka-uct | 21 | ✅ / ✅ | ✅ / ✅ | ✅ / 2 | 1977 / 2008 | ✅ / ✅ | ✅ / ✅ |
| akka-uct | 25 | ✅ / ✅ | ✅ / ✅ | ✅ / 1 | 2312 / 2189 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 21 | ✅ / ✅ | ✅ / ✅ | 3 / 2 | 8725 / 8783 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 25 | ✅ / ✅ | ✅ / ✅ | 2 / 2 | 8724 / 8493 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 21 | ✅ / ✅ | ✅ / ✅ | ✅ / 2 | 1277 / 1278 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 25 | ✅ / ✅ | ✅ / ✅ | 1 / 1 | 1274 / 1274 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 21 | ✅ / ✅ | ✅ / ✅ | 2 / 2 | 2916 / 2861 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 25 | ✅ / ✅ | ✅ / ✅ | 3 / 1 | 2887 / 2863 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 21 | ✅ / ✅ | ✅ / ✅ | 1 / 1 | 3465 / 3536 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 25 | ✅ / ✅ | ✅ / ✅ | 5 / 1 | 3492 / 3486 | ✅ / ✅ | ✅ / ✅ |
| reactors | 21 | ✅ / ✅ | ✅ / ✅ | 2 / 2 | 1826 / 1586 | ✅ / ✅ | ✅ / ✅ |
| reactors | 25 | ✅ / ✅ | ✅ / ✅ | ✅ / 2 | 1859 / 1858 | ✅ / ✅ | ✅ / ✅ |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (4)
ddprof-lib/src/main/cpp/threadLocalData.h:251
- _jmp_buf is a std::atomic, but these helpers rely on implicit atomic conversions/assignments. Using explicit load/store with acquire/release makes the publish/observe intent clearer (and avoids depending on implicit operator overloads), which is particularly important since this is read from an async signal handler path.
inline void setJmpCtx(sigjmp_buf* buf) {
_jmp_buf = buf;
}
inline sigjmp_buf* getJmpCtx() const {
return _jmp_buf;
}
inline bool isProtected() const {
return _jmp_buf != nullptr;
}
ddprof-lib/src/test/cpp/faultInjection_ut.cpp:148
- This comment still says control returns to
setjmp, but the test now usessigsetjmp/siglongjmp. Updating the wording avoids confusion when grepping for the intended mechanism.
// (c2) walkVM path: a raw dereference of an injected poison pointer must be
// caught by the sigsetjmp/siglongjmp crash protection, returning control to setjmp.
TEST_F(FaultInjectionTest, WalkVmSetjmpRecoversFromInjectedFault) {
ddprof-lib/src/test/cpp/hotspot_crash_protection_ut.cpp:287
- These comments still describe the recovery as
longjmpeven though the test has been switched tosigsetjmp/siglongjmp. Keeping terminology consistent will make the chaining behavior easier to follow.
// End-to-end with real sigsetjmp/siglongjmp: a fault inside the inner frame must
// land in the inner frame's own recovery branch — checkFault() always
// longjmps through whatever is currently installed — and once the inner
// frame has recovered and restored the outer's context, the outer frame must
// be left exactly as it was, never having been unwound itself.
ddprof-lib/src/test/cpp/hotspot_crash_protection_ut.cpp:312
- The inline comment says "Simulate checkFault(): longjmp" but the code calls
siglongjmpnow.
_pt->setJmpCtx(&inner_ctx);
// Simulate checkFault(): longjmp through whatever is currently
// installed — this must hit the inner frame, not the outer.
siglongjmp(*_pt->getJmpCtx(), 1);
FAIL() << "unreachable: siglongjmp does not return";
zhengyu123
marked this pull request as ready for review
July 25, 2026 21:15
Contributor
Benchmark Results (commit adad103)Pipeline: https://gitlab.ddbuild.io/DataDog/apm-reliability/benchmarking-platform/-/pipelines/126983712 Commit:
|
| Benchmark | JDK | Latest | Dev | Δ (dev vs latest) | Issues L/D |
|---|---|---|---|---|---|
| akka-uct | 21 | ✅ 10337 ms (21 iters) | ✅ 10307 ms (21 iters) | ≈ -0.3% (±11.6%) | — / — |
| akka-uct | 25 | ✅ 8961 ms (24 iters) | ✅ 8800 ms (24 iters) | ≈ -1.8% (±10.8%) | — / — |
| finagle-chirper | 21 | ✅ 6006 ms (33 iters) | ✅ 5958 ms (33 iters) | ≈ -0.8% (±26.1%) | |
| finagle-chirper | 25 | ✅ 5464 ms (36 iters) | ✅ 5489 ms (36 iters) | ≈ +0.5% (±24.6%) | |
| fj-kmeans | 21 | ✅ 2721 ms (68 iters) | ✅ 2826 ms (66 iters) | 🔴 +3.9% | — / — |
| fj-kmeans | 25 | ✅ 2812 ms (66 iters) | ✅ 2795 ms (67 iters) | ≈ -0.6% (±2.5%) | — / — |
| future-genetic | 21 | ✅ 2055 ms (90 iters) | ✅ 2109 ms (88 iters) | 🔴 +2.6% | — / — |
| future-genetic | 25 | ✅ 2021 ms (92 iters) | ✅ 2110 ms (88 iters) | 🔴 +4.4% | — / — |
| naive-bayes | 21 | ✅ 1251 ms (137 iters) | ✅ 1313 ms (131 iters) | ≈ +5% (±33.5%) | — / — |
| naive-bayes | 25 | ✅ 1032 ms (166 iters) | ✅ 1013 ms (168 iters) | ≈ -1.8% (±31.2%) | — / — |
| reactors | 21 | ✅ 16767 ms (15 iters) | ✅ 15964 ms (15 iters) | ≈ -4.8% (±7%) | — / — |
| reactors | 25 | ✅ 18801 ms (15 iters) | ✅ 18428 ms (15 iters) | ≈ -2% (±5%) | — / — |
Internal counter details (ddprof)
ddprof internal counters, latest / dev (✅ = 0, · = unavailable):
| Benchmark | JDK | Dropped rec | Dropped jvmti | Dropped trace | Skipped WC | AGCT fail | Unwind fail |
|---|---|---|---|---|---|---|---|
| akka-uct | 21 | ✅ / ✅ | ✅ / ✅ | ✅ / 5 | 1966 / 2025 | ✅ / ✅ | ✅ / ✅ |
| akka-uct | 25 | ✅ / ✅ | ✅ / ✅ | 1 / 3 | 2400 / 2261 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 21 | ✅ / ✅ | ✅ / ✅ | 7 / 3 | 8658 / 8467 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 25 | ✅ / ✅ | ✅ / ✅ | ✅ / 1 | 8288 / 8713 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 21 | ✅ / ✅ | ✅ / ✅ | 1 / 4 | 1278 / 1251 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 25 | ✅ / ✅ | ✅ / ✅ | 1 / 3 | 1295 / 1247 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 21 | ✅ / ✅ | ✅ / ✅ | 1 / 1 | 3067 / 2997 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 25 | ✅ / ✅ | ✅ / ✅ | ✅ / 2 | 2913 / 2848 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 21 | ✅ / ✅ | ✅ / ✅ | 5 / 1 | 3505 / 3571 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 25 | ✅ / ✅ | ✅ / ✅ | 3 / 9 | 3509 / 3461 | ✅ / ✅ | ✅ / ✅ |
| reactors | 21 | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ |
| reactors | 25 | ✅ / ✅ | ✅ / ✅ | 1 / 1 | 1932 / 1816 | ✅ / ✅ | ✅ / ✅ |
kaahos
approved these changes
Jul 27, 2026
kaahos
left a comment
kaahos
left a comment
Contributor
There was a problem hiding this comment.
Thanks for the work! Overall this looks good to me. I've made some suggestions to make some tests and comments more consistent with this switch.
jbachorik
approved these changes
Jul 27, 2026
Contributor
Benchmark Results (commit b4a0284)Pipeline: https://gitlab.ddbuild.io/DataDog/apm-reliability/benchmarking-platform/-/pipelines/127106698 Commit:
|
| Benchmark | JDK | Latest | Dev | Δ (dev vs latest) | Issues L/D |
|---|---|---|---|---|---|
| akka-uct | 21 | ✅ 10199 ms (21 iters) | ✅ 10390 ms (21 iters) | ≈ +1.9% (±11.4%) | — / — |
| akka-uct | 25 | ✅ 8878 ms (24 iters) | ✅ 8830 ms (24 iters) | ≈ -0.5% (±10.3%) | — / — |
| finagle-chirper | 21 | ✅ 5992 ms (33 iters) | ✅ 5986 ms (33 iters) | ≈ -0.1% (±25.2%) | |
| finagle-chirper | 25 | ✅ 5505 ms (36 iters) | ✅ 5456 ms (36 iters) | ≈ -0.9% (±25%) | |
| fj-kmeans | 21 | ✅ 2704 ms (70 iters) | ✅ 2749 ms (68 iters) | ≈ +1.7% (±2.7%) | — / — |
| fj-kmeans | 25 | ✅ 2819 ms (66 iters) | ✅ 2811 ms (66 iters) | ≈ -0.3% (±2.6%) | — / — |
| future-genetic | 21 | ✅ 2033 ms (92 iters) | ✅ 2106 ms (88 iters) | 🔴 +3.6% | — / — |
| future-genetic | 25 | ✅ 2075 ms (89 iters) | ✅ 2005 ms (93 iters) | 🟢 -3.4% | — / — |
| naive-bayes | 21 | ✅ 1340 ms (128 iters) | ✅ 1273 ms (134 iters) | ≈ -5% (±32.2%) | — / — |
| naive-bayes | 25 | ✅ 1007 ms (169 iters) | ✅ 983 ms (173 iters) | ≈ -2.4% (±31.3%) | — / — |
| reactors | 21 | ✅ 16201 ms (15 iters) | ✅ 16155 ms (15 iters) | ≈ -0.3% (±7.9%) | — / — |
| reactors | 25 | ✅ 18074 ms (15 iters) | ✅ 18767 ms (15 iters) | ≈ +3.8% (±5.2%) | — / — |
Internal counter details (ddprof)
ddprof internal counters, latest / dev (✅ = 0, · = unavailable):
| Benchmark | JDK | Dropped rec | Dropped jvmti | Dropped trace | Skipped WC | AGCT fail | Unwind fail |
|---|---|---|---|---|---|---|---|
| akka-uct | 21 | ✅ / ✅ | ✅ / ✅ | 1 / 2 | 1910 / 2009 | ✅ / ✅ | ✅ / ✅ |
| akka-uct | 25 | ✅ / ✅ | ✅ / ✅ | 1 / 3 | 2492 / 2064 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 21 | ✅ / ✅ | ✅ / ✅ | 6 / 5 | 8735 / 8297 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 25 | ✅ / ✅ | ✅ / ✅ | 1 / ✅ | 8462 / 8466 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 21 | ✅ / ✅ | ✅ / ✅ | 3 / ✅ | 1286 / 1280 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 25 | ✅ / ✅ | ✅ / ✅ | 2 / 3 | 1262 / 1259 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 21 | ✅ / ✅ | ✅ / ✅ | 2 / 1 | 3016 / 2973 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 25 | ✅ / ✅ | ✅ / ✅ | 2 / 6 | 2886 / 2947 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 21 | ✅ / ✅ | ✅ / ✅ | 4 / 4 | 3496 / 3485 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 25 | ✅ / ✅ | ✅ / ✅ | 4 / 1 | 3482 / 3478 | ✅ / ✅ | ✅ / ✅ |
| reactors | 21 | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ |
| reactors | 25 | ✅ / ✅ | ✅ / ✅ | 1 / 1 | 1817 / 1887 | ✅ / ✅ | ✅ / ✅ |
Contributor
|
A couple of things worth addressing before merging: Performance
Could you run a before/after comparison on a stack-walk-heavy benchmark to quantify the overhead, and add a note in the PR description on the correctness-vs-perf tradeoff (and why it's acceptable)? Test coverage None of the updated tests ( Nit The "How to test the change?" section in the PR description is empty — worth filling in given this is a signal-safety fix that regular CI likely won't exercise end-to-end. 🤖 Generated with Claude Code |
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?:
To perform a non-local jump out of a signal handler in POSIX C, you should use sigsetjmp() and siglongjmp() rather than standard setjmp() and longjmp(). [1, 2]
While longjmp() safely unwinds the execution stack, it does not consistently restore the process's signal mask. This means the signal that triggered your handler could remain blocked indefinitely. Using siglongjmp() ensures that your signal mask returns to its exact state prior to the signal delivery. [1, 2, 3, 4, 5]
Motivation:
Improve stability
Additional Notes:
-Regular CI tests
How to test the change?:
For Datadog employees:
credentials of any kind, I've requested a security review (run the
dd:platform-security-reviewskill, or file a request via the PSEC review form).
bewairealso runs automatically on every PR.Unsure? Have a question? Request a review!