fix fd leak#1
Closed
yanglong1010 wants to merge 1 commit into
Closed
Conversation
Contributor
|
Hi @yanglong1010 it's interesting that you're running the wallclock profiler without the tracer-guided thread filter - we were considering removing the fallback mode. Are you running the profiler without dd-java-agent, or are you setting |
Contributor
Author
|
Yes, we did not use dd-java-agent, but a java agent written by ourselves. I found that you recorded the span id in the wall clock sample event. This is a very good idea. I ported this part of the code into our project to achieve a similar continuous profiling effect. When running in continuous mode, we set the thread filter and everything is fine. The other mode is a one-time short-term opening without setting the thread filter, so it will enter the fallback mode. |
Contributor
|
@yanglong1010 Thank you so much for this patch. This is a public mirror of a repository we do new work in internally, so I can't merge your patch here, but I will apply your commit manually and sync it here later. Further patches are very welcome, and if you ever want to discuss features or give feedback, please email me at richard [dot] startin [at] datadoghq.com |
Contributor
|
@yanglong1010 your commit was merged here 6f95dc4 |
r1viollet
added a commit
that referenced
this pull request
Oct 23, 2024
Avoid shifting a negative value
This was raised by asan nightly runs
> Task :ddprof-test:testClasses
ddprof-lib/src/main/cpp/vmStructs.cpp:816:34: runtime error: left shift of negative value -1
#0 0x7f6f6bd20631 in ScopeDesc::readInt()
#1 0x7f6f6bce54a6 in ScopeDesc::decode(int)
#2 0x7f6f6bce7fb0 in StackWalker::walkVM(void*, ASGCT_CallFrame*, int, void const*, void const*)
Merged
2 tasks
2 tasks
rkennke
added a commit
that referenced
this pull request
Jul 8, 2026
- #1/#2 (ContextValueCache invalidation): replace the Java command-string parsing (isStartAction) — which didn't match arguments.cpp's tokenization — with a native, authoritative signal. Profiler::start sets a _context_value_dict_reset flag in the reset block where _context_value_map.clearAll() runs (reached only for ACTION_START), and JavaProfiler.execute consumes it via consumeContextDictionaryReset0() to drop the cache. No command re-parsing in Java. This does not fully close the concurrent race window (resolve() during the reset), which is no worse than the deprecated DirectByteBuffer path; fully closing it is tracked as a follow-up (see PR discussion / Jira). - #3 (clearContextValue0 validity): capture and restore the prior valid flag instead of unconditionally re-attaching with valid=1, so clearing one attribute cannot resurrect a record that clearTraceContext0 intentionally left deactivated. Added AllNativeContextTest.clearContextValuePreservesInvalidState. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
rkennke
added a commit
that referenced
this pull request
Jul 10, 2026
…1) (#631) * docs(context): design note for all-native OTEL context storage Captures the UAF problem, options A-E, the JMH benchmark campaign and its governing principle, the combined-API design, the consumer audit, and the expand -> migrate -> contract migration plan. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(context): all-native OTEL context write JNI primitives (PROF-15271) Add native setTraceContext0/clearTraceContext0 (combined per-scope activate/deactivate) and setContextValue0/clearContextValue0 (single attribute), plus attrs_data compaction and LRS helpers, in javaApi.cpp, with matching private native declarations on JavaProfiler. Each resolves the current carrier's OtelThreadContextRecord via ProfiledThread::current(). A JNI native frame pins a mounted virtual thread to its carrier for the duration of the call, so the resolved record is guaranteed live and cannot migrate mid-write -- there is no cached per-thread DirectByteBuffer to dangle. Writes follow the same detach -> mutate -> attach valid-flag protocol as ThreadContext, keeping the async-signal-handler sampler (ContextApi::get) coherent. Native layer only; the public Java API and value cache built on top follow in a later commit. The DirectByteBuffer path is unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(context): all-native public write API + global value cache (PROF-15271) Add the public all-native context API on JavaProfiler (provisional names, pending dd-trace-java coordination): setTraceContext/clearTraceContext (combined per-scope activate/deactivate) and setContextValue/clearContextValue (single attribute), each backed by the native primitives. Relocate the value->(encoding,utf8) memoization out of the per-thread ThreadContext into a new process-wide ContextValueCache. Because the encoding is a process-global immutable ID (registerConstant0), a shared lock-free cache is correct: immutable entries published via a single AtomicReferenceArray slot store (no torn reads), a miss re-registers idempotently, and collisions self-heal. This avoids duplicating the table across every carrier/virtual thread and needs no CarrierThreadLocal. registerConstant0 is made package-private for reuse (relocates in phase 3). Coexists with the DirectByteBuffer path (both write the same native record); the DBB path is unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(context): deprecate the DirectByteBuffer context API (PROF-15271) Mark the DBB write/read surface `@Deprecated` pointing at the all-native API: JavaProfiler.setContext/clearContext/setContextAttribute/clearContextAttribute/ setContextAttributesByIdAndBytes/copyTags/getThreadContext, and the DBB-path helper classes ThreadContext, ScopeStack, and ContextSetter. Non-breaking (annotations only) -- signals the migration for dd-trace-java (phase 2) ahead of removal (phase 3). Java has no -Werror, so the deprecation notes are informational; the DBB path still functions unchanged during coexistence. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(context): native path self-initializes the LRS attrs_data entry (PROF-15271) setTraceContext0/clearTraceContext0 now write the fixed LRS entry header (key_index=0, length=16) in addition to its hex value, instead of relying on the ThreadContext constructor to have initialized it. This makes the all-native path correct on a record that only saw the ProfiledThread zero-init -- i.e. when no DirectByteBuffer/ThreadContext was ever created (the phase-2 pure-native case), where attrs_data[1] would otherwise be 0 and mis-frame the LRS entry. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(context): all-native context write API tests (PROF-15271) AllNativeContextTest exercises setTraceContext/clearTraceContext/setContextValue/ clearContextValue: round-trip readback, equivalence with the DirectByteBuffer path, clear semantics, span-transition attribute reset, single-attribute set/clear, attrs_data overflow, rejected (null/oversized) values, DBB<->native coexistence on one thread, and coherence of native writes from many mounted virtual threads (the migration-safe path). Read-back uses the ThreadContext DBB read methods as an oracle -- a view over the same native record the all-native path writes. All 9 pass on JDK 21 (debug); the existing context suite (Otel/Carrier/Tag/ProcessContext) is unregressed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(context): reference the preserved benchmark-experiment branch (PROF-15271) Point the design note appendix at branch context-storage-benchmark-experiment (pushed to origin), which preserves the full JMH campaign + native prototypes for reproducibility, rather than describing scaffolding that lives only on that branch. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(context): production perf-guard benchmark for the all-native cycle (PROF-15271) ContextCombinedBenchmark compares the production per-scope activate+deactivate cycle: the combined native calls (setTraceContext + clearTraceContext) vs the deprecated DirectByteBuffer sequence as dd-trace-java actually drives it (profiler.setContext + 2x setContextAttribute + clear...), on platform + mounted virtual threads, both storage modes. Measured (JDK 21, debug, carrier): native ~136 ns vs DBB ~189 ns -- native ~28% faster, mode-independent, and a win on vthreads too (~147 vs ~193 ns). The DBB baseline here is the faithful production cost: each of its 6 calls pays a currentContext()/ CarrierThreadLocal lookup, which the 2-call combined native cycle avoids. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(context): add missing javadoc tags on new all-native context API Fixes check-javadoc "no comment" warnings for setTraceContext, setContextValue, and clearContextValue introduced in this PR. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * review: address pre-review findings on the all-native context API - Convert `// @deprecated` line comments into proper `@deprecated` javadoc tags on the deprecated JavaProfiler methods and the ThreadContext / ScopeStack / ContextSetter classes, so the rationale renders in javadoc and `-Xlint` stops warning about the missing tag. - Reject out-of-range slots (`slot >= MAX_CONTEXT_SLOTS`) in setContextValue and resolveContextValue before resolving, so an invalid slot no longer registers an orphan entry in the permanent native Dictionary (matches the DBB ThreadContext.setContextAttribute guard and the existing javadoc). - Reconcile the perf figures in the design note: explain that ~7-10% is the prototype microbench baseline and ~28% is the production ContextCombinedBenchmark baseline (six real currentContext()-resolving calls), so they no longer read as contradictory. - Clarify the otelWriteLrsEntry comment: only setTraceContext0/clearTraceContext0 establish the LRS entry; setContextValue0 does not, which is harmless. - Add an assert(len <= 255) in otelReadUtf8 documenting the caller contract, keeping the clamp as release-build defense. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * review: address PR #631 review findings on the all-native context API - P1 (Codex/jbachorik): the all-native write entry points never initialized OTEL TLS, so a thread that used only setTraceContext/setContextValue wrote a record the sampler (ContextApi::get / wallClock) ignored and eBPF could not discover. setTraceContext0 / setContextValue0 now call ContextApi::initializeContextTLS(thrd) on first use, matching the DBB path. Added AllNativeContextSamplingTest: a wall-clock JFR test on a thread that never calls getThreadContext(), asserting a sample carries the natively written context (fails if the record is not published to the sampler). - jbachorik: ContextValueCache assumed process-lifetime-immutable encodings, but a fresh start resets the native Dictionary (clearAll), reassigning IDs. Added ContextValueCache.clear(), invoked from execute() on a 'start' action, and corrected the class javadoc invariant. - jbachorik: MAX_CONTEXT_SLOTS duplicated native DD_TAGS_CAPACITY with only a comment binding them. Added native accessor maxContextSlots0() and MaxContextSlotsTest to catch drift from DD_TAGS_CAPACITY / ThreadContext.MAX_CUSTOM_SLOTS at test time. - jbachorik nits: added tests for the native null byte[] guard in otelReadUtf8, the otelCompactAttr 2-byte (zero-length value) boundary, and the setContextValue slot == MAX_CONTEXT_SLOTS boundary. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * review: address kaahos findings on the all-native context API - #1/#2 (ContextValueCache invalidation): replace the Java command-string parsing (isStartAction) — which didn't match arguments.cpp's tokenization — with a native, authoritative signal. Profiler::start sets a _context_value_dict_reset flag in the reset block where _context_value_map.clearAll() runs (reached only for ACTION_START), and JavaProfiler.execute consumes it via consumeContextDictionaryReset0() to drop the cache. No command re-parsing in Java. This does not fully close the concurrent race window (resolve() during the reset), which is no worse than the deprecated DirectByteBuffer path; fully closing it is tracked as a follow-up (see PR discussion / Jira). - #3 (clearContextValue0 validity): capture and restore the prior valid flag instead of unconditionally re-attaching with valid=1, so clearing one attribute cannot resurrect a record that clearTraceContext0 intentionally left deactivated. Added AllNativeContextTest.clearContextValuePreservesInvalidState. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * review: setContextValue0 preserves prior valid state (kaahos) Apply the same guard as clearContextValue0: capture the record's valid flag up front and restore it instead of hardcoding valid=1. Writing a single attribute must not resurrect a record that clearTraceContext0 intentionally deactivated — republishing it would expose a span-less record (zero trace/span) as valid. Mirrors the DBB path's guard in ThreadContext.setContextAttributesByIdAndBytes ("never resurrect a cleared, span-less record"). Added AllNativeContextTest.setContextValuePreservesInvalidState. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(context): fail fast on illegal arguments in the all-native write API Distinguish caller programming errors (fail loud) from runtime/data conditions (soft signal), instead of silently swallowing the former: - setTraceContext rejects spanId==0 with IllegalArgumentException — it is the activation path and requires a real span; clearing is clearTraceContext. This drops the DBB path's conflation of setContext(0,0,0,0) with a clear and never publishes a span-less record. The native setTraceContext0 primitive asserts spanId!=0 as debug-only defense (the wrapper validates first). - Out-of-range custom-attribute slots now throw IllegalArgumentException in setTraceContext (non-negative slot >= MAX_CONTEXT_SLOTS; negative stays the documented skip sentinel), setContextValue, and clearContextValue — instead of being silently skipped / returning false / no-op'ing. Data conditions stay soft (not exceptions), matching normal control flow: a null or oversized value, or a full Dictionary, still returns false (setContextValue) or skips (setTraceContext). Native keeps its hard slot range guards (load-bearing for memory safety and to defend direct-JNI calls). Tests: added setTraceContextRejectsZeroSpanId; expanded slotBoundaryIsRejected to assert IllegalArgumentException across setContextValue/clearContextValue/ setTraceContext and that a negative activation slot still skips without throwing. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * context: setContextValue0 publishes app context without a span Reverse the earlier "preserve prior valid" change on setContextValue0. Setting a value means "make it visible": an attribute must be observable to the sampler even when no span is active (zero trace/span). This is dd-trace-java's app-context-independent-of-span model (PR #11646) — app-owned attributes such as http.route, and the reapply-after-deactivation path, must stay visible between spans. It mirrors the DBB single-attribute setter (ThreadContext.setContextAttributeDirect), which always re-attaches; the earlier change had wrongly modeled it on the *bulk* setter's valid==0 guard. clearContextValue0 keeps preserving the prior valid flag: removing a value must not resurrect a deactivated record. So the asymmetry is intentional and matches the DBB single-vs-bulk behavior the tracer's reapply logic was built around: set publishes, clear does not. Test renamed/rewritten: setContextValuePublishesAppContextWithoutSpan asserts a value set after clearTraceContext republishes (valid=1, span=0, value observable). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * context: native copyContextTags read + phase 2 migration plan Add JavaProfiler.copyContextTags(int[]) + native copyContextTags0: reads the current thread's custom-attribute sidecar tag encodings directly from the record (ProfiledThread::current()), with no ThreadContext / DirectByteBuffer. Unlike the deprecated DBB copyTags — which goes through ThreadContext, whose ctor resets the record — this observes encodings written via the all-native setContextValue path and does not clobber them. This is the read the dd-trace-java Phase 2 migration needs for its (test-only) snapshot() oracle once writes go all-native. Also adds the Phase 2 plan doc (dd-trace-java migration onto the all-native API). Test: AllNativeContextTest.copyContextTagsReadsNativeEncodings (never calls getThreadContext, proving the native read observes native writes). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * context: add ContextSetter.size() (pure-Java slot count) The dd-trace-java Phase 2 bridge sizes its app-offset arrays from the context attribute count. It previously read that as snapshotTags().length — a DBB read that creates a ThreadContext. Expose size() (attributes.size(), pure Java) so the bridge can size without touching the DBB path; offsetOf + size are the only ContextSetter methods it still uses. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
2 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.
hi,
Our project uses the java-profiler code, and the Java process has a Too many open files error. After using strace -k -e trace=openat -f java-pid, I feel that I may have found the reason. After I modified it slightly, I have verified that the problem can be solved. I don't know if I can contribute a little code to java-profiler.
[WARN] perf_event_open for TID 30704 failed: Too many open files
ulimit -n
256
lsof -p 30499|grep '/proc/30499/task'|wc -l
202
lsof -p 30499|grep '/proc/30499/task'|head
java 30499 root 23r DIR 0,3 0 61771716 /proc/30499/task
java 30499 root 24r DIR 0,3 0 61771716 /proc/30499/task
java 30499 root 51r DIR 0,3 0 61771716 /proc/30499/task
java 30499 root 52r DIR 0,3 0 61771716 /proc/30499/task
java 30499 root 53r DIR 0,3 0 61771716 /proc/30499/task
java 30499 root 54r DIR 0,3 0 61771716 /proc/30499/task
java 30499 root 55r DIR 0,3 0 61771716 /proc/30499/task
java 30499 root 56r DIR 0,3 0 61771716 /proc/30499/task
java 30499 root 57r DIR 0,3 0 61771716 /proc/30499/task
java 30499 root 58r DIR 0,3 0 61771716 /proc/30499/task