Optimize SafeAccess:safeCopy() implementation in assembly#660
Conversation
There was a problem hiding this comment.
Pull request overview
This PR reimplements SafeAccess::safeCopy() in architecture-specific assembly to perform byte-granular copying guarded by the existing handle_safefetch signal redirection mechanism, eliminating sentinel-based fault heuristics and avoiding over-reads past src+len.
Changes:
- Replace the C++/safefetch32-based
safeCopy()implementation with an in-assemblysafecopy_implloop and asafecopy_contfallback. - Extend
SafeAccess::handle_safefetch()to redirect faults occurring anywhere inside thesafecopy_implcode range. - Update
safefetch_ut.cppcomments to reflect the new safeCopy mechanism and semantics.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| ddprof-lib/src/main/cpp/safeAccess.cpp | Adds safecopy_impl/safecopy_cont assembly stubs, switches safeCopy() to them, and updates fault redirection logic in handle_safefetch(). |
| ddprof-lib/src/main/cpp/safeAccess.h | Updates the safeCopy() contract comment to match the new byte-granular, no-overread behavior. |
| ddprof-lib/src/test/cpp/safefetch_ut.cpp | Adjusts test documentation/comments for the new safeCopy() behavior (no functional test logic changes). |
Comments suppressed due to low confidence (1)
ddprof-lib/src/main/cpp/safeAccess.cpp:256
- handle_safefetch() dereferences the ucontext pointer (uc->current_pc) before checking whether the provided context is null. Even if the signal handler currently always supplies a valid ucontext, this ordering makes the null-check ineffective and can crash if handle_safefetch is ever called with a null context.
bool SafeAccess::handle_safefetch(int sig, void* context) {
ucontext_t* uc = (ucontext_t*)context;
uintptr_t pc = uc->current_pc;
if ((sig == SIGSEGV || sig == SIGBUS) && uc != nullptr) {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
CI Test ResultsRun: #29366855474 | Commit:
Status Overview
Legend: ✅ passed | ❌ failed | ⚪ skipped | 🚫 cancelled Summary: Total: 32 | Passed: 32 | Failed: 0 Updated: 2026-07-14 21:02:48 UTC |
Benchmark Results (commit ec57fa1)Pipeline: https://gitlab.ddbuild.io/DataDog/apm-reliability/benchmarking-platform/-/pipelines/124571803 Commit:
|
| Benchmark | JDK | Latest | Dev | Δ (dev vs latest) | Issues L/D |
|---|---|---|---|---|---|
| akka-uct | 21 | ✅ 10392 ms (21 iters) | ✅ 10379 ms (21 iters) | ≈ -0.1% (±11.5%) | — / — |
| akka-uct | 25 | ✅ 8885 ms (24 iters) | ✅ 8878 ms (24 iters) | ≈ -0.1% (±10.7%) | — / — |
| finagle-chirper | 21 | ✅ 6057 ms (33 iters) | ✅ 6030 ms (33 iters) | ≈ -0.4% (±25.2%) | |
| finagle-chirper | 25 | ✅ 5426 ms (36 iters) | ✅ 5520 ms (36 iters) | ≈ +1.7% (±24.6%) | |
| fj-kmeans | 21 | ✅ 2650 ms (71 iters) | ✅ 2777 ms (67 iters) | 🔴 +4.8% | — / — |
| fj-kmeans | 25 | ✅ 2768 ms (68 iters) | ✅ 2864 ms (66 iters) | 🔴 +3.5% | — / — |
| future-genetic | 21 | ✅ 2071 ms (89 iters) | ✅ 2032 ms (91 iters) | ≈ -1.9% (±2.6%) | — / — |
| future-genetic | 25 | ✅ 2024 ms (92 iters) | ✅ 2108 ms (88 iters) | 🔴 +4.2% | — / — |
| naive-bayes | 21 | ✅ 1260 ms (136 iters) | ✅ 1251 ms (136 iters) | ≈ -0.7% (±32.7%) | — / — |
| naive-bayes | 25 | ✅ 1020 ms (167 iters) | ✅ 1015 ms (169 iters) | ≈ -0.5% (±31.7%) | — / — |
| reactors | 21 | ✅ 15716 ms (15 iters) | ✅ 16511 ms (15 iters) | ≈ +5.1% (±6.8%) | — / — |
| reactors | 25 | ✅ 17928 ms (15 iters) | ✅ 18515 ms (15 iters) | ≈ +3.3% (±4.8%) | — / — |
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 / 1 | 2067 / 1978 | ✅ / ✅ | ✅ / ✅ |
| akka-uct | 25 | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | 2343 / 2212 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 21 | ✅ / ✅ | ✅ / ✅ | 5 / 3 | 8525 / 8819 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 25 | ✅ / ✅ | ✅ / ✅ | 2 / 2 | 8375 / 8755 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 21 | ✅ / ✅ | ✅ / ✅ | 2 / 1 | 1285 / 1260 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 25 | ✅ / ✅ | ✅ / ✅ | 2 / 4 | 1272 / 1300 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 21 | ✅ / ✅ | ✅ / ✅ | 1 / ✅ | 2894 / 2979 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 25 | ✅ / ✅ | ✅ / ✅ | 6 / ✅ | 2898 / 2911 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 21 | ✅ / ✅ | ✅ / ✅ | 1 / 1 | 3528 / 3530 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 25 | ✅ / ✅ | ✅ / ✅ | 3 / 2 | 3510 / 3463 | ✅ / ✅ | ✅ / ✅ |
| reactors | 21 | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | 1560 / 1630 | ✅ / ✅ | ✅ / ✅ |
| reactors | 25 | ✅ / ✅ | ✅ / ✅ | 1 / ✅ | 1799 / 1812 | ✅ / ✅ | ✅ / ✅ |
![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: bf4f024eac
ℹ️ 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".
Reliability & Chaos Results✅ All reliability & chaos checks passed Pipeline: https://gitlab.ddbuild.io/DataDog/java-profiler/-/pipelines/124571756 |
Benchmark Results (commit bf4f024)Pipeline: https://gitlab.ddbuild.io/DataDog/apm-reliability/benchmarking-platform/-/pipelines/124580944 Commit:
|
| Benchmark | JDK | Latest | Dev | Δ (dev vs latest) | Issues L/D |
|---|---|---|---|---|---|
| akka-uct | 25 | ✅ 8909 ms (24 iters) | ✅ 8938 ms (24 iters) | ≈ +0.3% (±10.4%) | — / — |
| finagle-chirper | 21 | ✅ 5976 ms (33 iters) | ✅ 6031 ms (33 iters) | ≈ +0.9% (±26.3%) | |
| finagle-chirper | 25 | ✅ 5462 ms (36 iters) | ✅ 5500 ms (36 iters) | ≈ +0.7% (±24.6%) | |
| fj-kmeans | 21 | ✅ 2774 ms (67 iters) | ✅ 2720 ms (69 iters) | ≈ -1.9% (±2.7%) | — / — |
| fj-kmeans | 25 | ✅ 2763 ms (67 iters) | ✅ 2819 ms (66 iters) | ≈ +2% (±2.7%) | — / — |
| future-genetic | 21 | ✅ 2043 ms (90 iters) | ✅ 2103 ms (89 iters) | 🔴 +2.9% | — / — |
| future-genetic | 25 | ✅ 2013 ms (92 iters) | ✅ 2032 ms (91 iters) | ≈ +0.9% (±2.8%) | — / — |
| naive-bayes | 21 | ✅ 1246 ms (137 iters) | ✅ 1238 ms (138 iters) | ≈ -0.6% (±32.5%) | — / — |
| naive-bayes | 25 | ✅ 1018 ms (168 iters) | ✅ 1017 ms (168 iters) | ≈ -0.1% (±31.2%) | — / — |
| reactors | 21 | ✅ 16906 ms (15 iters) | ✅ 15625 ms (15 iters) | ≈ -7.6% (±7.8%) | — / — |
| reactors | 25 | ✅ 18263 ms (15 iters) | ✅ 18459 ms (15 iters) | ≈ +1.1% (±4.6%) | — / — |
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 / 1 | 1955 / 2136 | ✅ / ✅ | ✅ / ✅ |
| akka-uct | 25 | ✅ / ✅ | ✅ / ✅ | ✅ / 1 | 2209 / 2287 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 21 | ✅ / ✅ | ✅ / ✅ | 3 / 2 | 8586 / 8531 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 25 | ✅ / ✅ | ✅ / ✅ | 3 / 1 | 8309 / 8821 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 21 | ✅ / ✅ | ✅ / ✅ | ✅ / 3 | 1272 / 1292 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 25 | ✅ / ✅ | ✅ / ✅ | ✅ / 6 | 1259 / 1262 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 21 | ✅ / ✅ | ✅ / ✅ | 1 / 1 | 2888 / 2955 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 25 | ✅ / ✅ | ✅ / ✅ | 5 / 5 | 2784 / 2853 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 21 | ✅ / ✅ | ✅ / ✅ | 2 / 1 | 3533 / 3517 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 25 | ✅ / ✅ | ✅ / ✅ | 4 / 4 | 3476 / 3466 | ✅ / ✅ | ✅ / ✅ |
| reactors | 21 | ✅ / ✅ | ✅ / ✅ | 1 / 1 | 1571 / 1511 | ✅ / ✅ | ✅ / ✅ |
| reactors | 25 | ✅ / ✅ | ✅ / ✅ | ✅ / 1 | 1898 / 1855 | ✅ / ✅ | ✅ / ✅ |
|
🗿 🤖 🔴 File: [Sphinx Review — MEDIUM] The range-based fault redirect for safecopy_impl depends on link-time symbol adjacency (safecopy_impl immediately followed by safecopy_cont) with no static or build-time check enforcing that layout, unlike the exact-address match used for safefetch32/64. Suggestion: Add a one-time startup assertion (e.g. in profiler init or a static constructor) that safecopy_cont's address is strictly greater than safecopy_impl's, so any future toolchain regression fails loudly at startup rather than silently losing fault protection. |
|
🗿 🤖 🔴 File: [Sphinx Review — LOW] No test exercises a fault partway through a multi-byte safeCopy (a readable prefix followed by an unreadable byte), so the newly-documented 'up to len-1 bytes already written to dst' partial-write behavior is unverified. Suggestion: Add a test that maps two adjacent pages, unmaps the second, places src straddling the boundary with a known prefix length, and asserts both that safeCopy returns false and that dst[0..prefix) matches the readable bytes. |
jbachorik
left a comment
jbachorik
left a comment
There was a problem hiding this comment.
LGTM, assuming the small nits in the review are addressed.
Benchmark Results (commit f0493f4)Pipeline: https://gitlab.ddbuild.io/DataDog/apm-reliability/benchmarking-platform/-/pipelines/124621753 Commit:
|
| Benchmark | JDK | Latest | Dev | Δ (dev vs latest) | Issues L/D |
|---|---|---|---|---|---|
| akka-uct | 21 | ✅ 10228 ms (21 iters) | ✅ 10317 ms (21 iters) | ≈ +0.9% (±11%) | — / — |
| akka-uct | 25 | ✅ 8871 ms (24 iters) | ✅ 8862 ms (24 iters) | ≈ -0.1% (±10.1%) | — / — |
| finagle-chirper | 21 | ✅ 5986 ms (33 iters) | ✅ 5990 ms (33 iters) | ≈ +0.1% (±24.9%) | |
| finagle-chirper | 25 | ✅ 5451 ms (36 iters) | ✅ 5517 ms (36 iters) | ≈ +1.2% (±24.3%) | |
| fj-kmeans | 21 | ✅ 2752 ms (68 iters) | ✅ 2753 ms (68 iters) | ≈ +0% (±2.7%) | — / — |
| fj-kmeans | 25 | ✅ 2830 ms (66 iters) | ✅ 2842 ms (66 iters) | ≈ +0.4% (±2.6%) | — / — |
| future-genetic | 21 | ✅ 2065 ms (89 iters) | ✅ 2080 ms (89 iters) | ≈ +0.7% (±2.6%) | — / — |
| future-genetic | 25 | ✅ 2125 ms (87 iters) | ✅ 2055 ms (91 iters) | 🟢 -3.3% | — / — |
| naive-bayes | 21 | ✅ 1238 ms (138 iters) | ✅ 1247 ms (137 iters) | ≈ +0.7% (±32.9%) | — / — |
| reactors | 21 | ✅ 16926 ms (15 iters) | ✅ 15566 ms (15 iters) | 🟢 -8% | — / — |
| reactors | 25 | ✅ 18660 ms (15 iters) | ✅ 18823 ms (15 iters) | ≈ +0.9% (±5.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 | ✅ / ✅ | ✅ / ✅ | 5 / 5 | 2031 / 2000 | ✅ / ✅ | ✅ / ✅ |
| akka-uct | 25 | ✅ / ✅ | ✅ / ✅ | 1 / 2 | 2291 / 2319 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 21 | ✅ / ✅ | ✅ / ✅ | 3 / 2 | 8608 / 8441 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 25 | ✅ / ✅ | ✅ / ✅ | 3 / 2 | 8390 / 8522 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 21 | ✅ / ✅ | ✅ / ✅ | 1 / 2 | 1291 / 1284 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 25 | ✅ / ✅ | ✅ / ✅ | 3 / 3 | 1295 / 1294 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 21 | ✅ / ✅ | ✅ / ✅ | 1 / 3 | 2936 / 2974 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 25 | ✅ / ✅ | ✅ / ✅ | ✅ / 1 | 2899 / 2922 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 21 | ✅ / ✅ | ✅ / ✅ | 3 / 8 | 3471 / 3513 | ✅ / ✅ | ✅ / ✅ |
| reactors | 21 | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | 1583 / 1561 | ✅ / ✅ | ✅ / ✅ |
| reactors | 25 | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | 1887 / 1939 | ✅ / ✅ | ✅ / ✅ |
gh-worker-dd-devflow-36fce6
Bot
added
mergequeue-status: queued
mergequeue-status: in_progress
and removed
mergequeue-status: queued
labels
gh-worker-dd-mergequeue-cf854d
Bot
merged commit 0c8985a
into
main
gh-worker-dd-devflow-36fce6
Bot
added
mergequeue-status: done
and removed
mergequeue-status: in_progress
labels
What does this PR do?:
Reimplement
SafeAccess::safeCopy()in assembly.Motivation:
Current implementation of SafeAccess::safeCopy() is extremely expensive:
Double reads to detect fault
No reads can inline
Optimized the implementation in assembly, similar to SafeAccess::safefetch()
No false detection. The old code inferred faults from two sentinel values matching — a heuristic. The assembly version catches the actual hardware fault via the signal handler, so it's exact.
No over-read. The byte-granular copy only ever touches [src, src+len). The old code did 4-byte aligned loads with front-fixup and up-to-3-byte over-reads, requiring careful page-boundary reasoning (all now unnecessary).
Sanitizer-clean. Assembly loads aren't ASan-instrumented, so faulting reads on unmapped/PROT_NONE pages don't trip the sanitizer.
Additional Notes:
How to test the change?:
Passed existing tests for old implementation
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!