fix: address ivoanjo-ghost review findings

- ThreadContextBenchmark: replace removed getContext() with getSpanId()
- thread.cpp: use __atomic_store_n(RELEASE) for _otel_ctx_initialized
- thread.h: document naturally-aligned u64 atomicity assumption
- ThreadContext: document clearContextDirect valid=0 OTEP reader trade-off
- context_api.h: move LOCAL_ROOT_SPAN_ATTR_INDEX to context_api.cpp (file-local)
- perf report: remove stale getContext results, note replacement by direct buffer reads

Author: jbachorik

ddprof-lib/src/main/cpp/context_api.cpp

@@ -23,6 +23,9 @@
 #include "thread.h"
 #include <cstring>
 
+// Reserved attribute index for local root span ID in OTEL attrs_data.
+// Only used within this translation unit; not part of the public ContextApi header.
+static const uint8_t LOCAL_ROOT_SPAN_ATTR_INDEX = 0;
 
 /**
  * Initialize context TLS for the current thread on first use.

ddprof-lib/src/main/cpp/context_api.h

@@ -78,8 +78,6 @@ class ContextApi {
      */
     static void registerAttributeKeys(const char** keys, int count);
 
-    // Reserved attribute index for local root span ID in OTEL attrs_data.
-    static const uint8_t LOCAL_ROOT_SPAN_ATTR_INDEX = 0;
 };
 
 #endif /* _CONTEXT_API_H */

ddprof-lib/src/main/cpp/thread.cpp

@@ -110,7 +110,9 @@ void ProfiledThread::releaseFromBuffer() {
     // short-circuit before reading partially-zeroed data during the memset below.
     // (The valid flag is zeroed by memset too, but _otel_ctx_initialized guards
     // the isContextInitialized() check which runs before any record access.)
-    _otel_ctx_initialized = false;
+    // Use __ATOMIC_RELEASE so the compiler cannot reorder this store after the
+    // memset on ARM with aggressive optimizations.
+    __atomic_store_n(&_otel_ctx_initialized, false, __ATOMIC_RELEASE);
     clearOtelSidecar();
     memset(&_otel_ctx_record, 0, sizeof(_otel_ctx_record));
 

ddprof-lib/src/main/cpp/thread.h

@@ -129,6 +129,9 @@ class ProfiledThread : public ThreadLocalData {
     // between detach() and attach() in Java, ContextApi::get returns valid=0 with
     // root_span_id=0; writing that would clobber the value Java just stored.
     if (context_valid) {
+      // Plain store is safe: naturally-aligned u64 stores/loads are atomic on
+      // x86-64 and aarch64 (the only supported targets). The Java writer uses
+      // sidecarBuffer.putLong() which is a single aligned 8-byte store.
       _otel_local_root_span_id = root_span_id;
     }
     _recording_epoch = recording_epoch;

ddprof-lib/src/main/java/com/datadoghq/profiler/ThreadContext.java

@@ -293,6 +293,12 @@ private void setContextDirect(long localRootSpanId, long spanId, long trHi, long
      * attrs_data_size is not reset here; the next non-zero setContext call will reset it
      * before attach(). This is safe because valid remains 0 after clear, so no reader will
      * observe the stale attrs_data_size.
+     *
+     * <p>External OTEP readers see valid=0 and skip the record — they cannot distinguish
+     * "cleared" from "being mutated". This is intentional: without also resetting
+     * attrs_data_size here, publishing valid=1 with a stale attrs_data_size would expose
+     * a partially-valid record. The cleared state is effectively invisible to external
+     * readers until the next non-zero setContext call publishes it.
      */
     private void clearContextDirect() {
         recordBuffer.putLong(traceIdOffset, 0);

ddprof-stresstest/src/jmh/java/com/datadoghq/profiler/stresstest/scenarios/throughput/ThreadContextBenchmark.java

@@ -126,8 +126,8 @@ public void spanLifecycle_4t(ThreadState ts) {
     }
 
     @Benchmark
-    public long[] getContext(ThreadState ts) {
-        return ts.ctx.getContext();
+    public long getSpanId(ThreadState ts) {
+        return ts.ctx.getSpanId();
     }
 
     @Benchmark

doc/performance/reports/thread-context-benchmark-2026-03-21.md

@@ -42,7 +42,6 @@ of 1.
 ```
 Benchmark                                 Mode  Cnt   Score   Error  Units
 ThreadContextBenchmark.clearContext       avgt    9   5.011 ± 0.039  ns/op
-ThreadContextBenchmark.getContext         avgt    9  71.557 ± 1.162  ns/op
 ThreadContextBenchmark.setAttrCacheHit   avgt    9  10.707 ± 0.077  ns/op
 ThreadContextBenchmark.setContextFull    avgt    9  11.104 ± 0.338  ns/op
 ThreadContextBenchmark.setContextFull_2t avgt    9  11.081 ± 0.105  ns/op
@@ -60,16 +59,17 @@ ThreadContextBenchmark.spanLifecycle_4t  avgt    9  32.203 ± 0.309  ns/op
 | `setContextFull` | 11.104 | ± 0.338 | Write localRootSpanId, spanId, traceIdHigh, traceIdLow (4-arg put) |
 | `setAttrCacheHit`| 10.707 | ± 0.077 | Set string attribute (dictionary cache hit)    |
 | `spanLifecycle`  | 30.430 | ± 0.129 | `setContextFull` + `setAttrCacheHit` combined  |
-| `getContext`     | 71.557 | ± 1.162 | Read context into a `long[]` (allocates)       |
 
 `spanLifecycle` at ~30.4 ns is consistent with the sum of its parts:
 `setContextFull` (~11.1 ns) + `setAttrCacheHit` (~10.7 ns) + loop and
 call overhead (~8.6 ns). All error bars are under 2% of the score,
 indicating stable measurements.
 
-`getContext` is the most expensive operation at ~71.6 ns, dominated by
-the `long[]` allocation on the return path. This is a read-side
-operation, not on the span start/end hot path.
+Note: the original `getContext` benchmark (71.6 ns, dominated by `long[]` allocation)
+was removed when `getContext()` was replaced by direct `DirectByteBuffer` reads in
+`getSpanId()` and `getRootSpanId()`, eliminating the JNI call and the array allocation.
+The `getSpanId` benchmark was added as a replacement but was not run at the time of
+this report.
 
 ### False Sharing Analysis
 
@@ -124,7 +124,6 @@ divergences; the vendor difference does not materially affect the comparison.
 | `setContextFull` |        20.0 |             11.104 |   1.8x  |
 | `setAttrCacheHit`|       114.8 |             10.707 |  10.7x  |
 | `spanLifecycle`  |       146.3 |             30.430 |   4.8x  |
-| `getContext`     |        68.8 |             71.557 |   0.96x |
 
 The span lifecycle hot path improved by **4.8x** (146 ns to 30 ns).
 The dominant contributor is `setContextAttribute`, which improved by
@@ -273,11 +272,10 @@ represents less than 0.004% of available CPU time.
    allocation strategy (one heap allocation per thread) provides
    sufficient cache-line isolation.
 
-2. **`getContext` allocation.** The `long[]` allocation in `getContext`
-   (~71.6 ns) could be eliminated by passing a reusable buffer, but
-   this is a read-side operation not on the span start/end critical
-   path. Only worth optimizing if profiling shows GC pressure from
-   high-frequency context reads.
+2. **`getContext` allocation (resolved).** The original `getContext` method
+   allocated a `long[]` per call (~71.6 ns). This was resolved by replacing
+   `getContext()` with direct `DirectByteBuffer` reads in `getSpanId()` and
+   `getRootSpanId()`, eliminating both the JNI call and the array allocation.
 
 3. **Higher thread counts.** To stress-test on server hardware, add
    `@Threads(8)` and `@Threads(16)` variants and run on a machine with