Repository navigation
Upgrade async-profiler to 4.5 - #3267
hengyunabc wants to merge 2 commits into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: 260d990e6d
ℹ️ 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".
| /** | ||
| * Tracks async-profiler's recording state and provides timestamps aligned with the profiler clock. | ||
| */ | ||
| public class Recording { |
There was a problem hiding this comment.
The commit introduces the JNI-backed Recording state machine and several distinct Span filtering paths, but a repo-wide search of core/src/test finds no tests referencing either type. These branches include normal-thread, virtual-thread, timestamp-boundary, unavailable-library, and recording-state behavior, so relying only on manual smoke tests leaves regressions in newly public APIs undetected; add unit, smoke, and functional coverage as required by the repository.
AGENTS.md reference: AGENTS.md:L258-L258
Useful? React with 👍 / 👎.
| /** | ||
| * Tracks async-profiler's recording state and provides timestamps aligned with the profiler clock. | ||
| */ |
There was a problem hiding this comment.
Both new profiler classes introduce English Javadocs and inline implementation comments, while the repository explicitly requires all code comments to be written in Chinese; translate the explanatory comments while retaining required license identifiers unchanged.
AGENTS.md reference: AGENTS.md:L243-L243
Useful? React with 👍 / 👎.
| if (isVirtualThread() || hasProfileSamples(startTime)) { | ||
| Recording.emitSpan(startTime, Recording.timestamp(), tag); |
There was a problem hiding this comment.
On JDK 21+ virtual threads, isVirtualThread() makes this condition succeed without consulting hasProfileSamples, so endIfProfiled records every span even when no profiling sample occurred; the identical shortcut in emitIfProfiled has the same behavior. High-frequency virtual-thread users choosing the conditional APIs to avoid native calls and JFR growth therefore receive unconditional spans instead, potentially causing substantial recording overhead and misleading results.
Useful? React with 👍 / 👎.
| if (isVirtualThread() || hasProfileSamples(startTime)) { | ||
| Recording.emitSpan(startTime, endTime, tag); |
There was a problem hiding this comment.
When emitIfProfiled is invoked after its supplied endTime, a newer profiling sample can make hasProfileSamples(startTime) true even though that sample lies outside the documented [startTime, endTime] interval. This produces a false-positive profiler.Span event for delayed or historical intervals; compare the last-sample timestamp against both bounds before emitting.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🟡 Changes recommended
The dumpOtlp() API removal introduces avoidable binary incompatibility for existing consumers (especially via JMX), and should retain a delegating deprecated overload to prevent runtime NoSuchMethodErrors.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR upgrades the bundled async-profiler integration from v4.4 to v4.5 to pick up upstream fixes (notably the essential-library parsing change to avoid long pauses during profiler loading) and syncs Arthas’ embedded Java API and documentation references with the upstream v4.5 state.
Changes:
- Updated profiler documentation (EN/ZH) and in-code references to async-profiler v4.5.
- Synced embedded async-profiler Java API changes, including
dumpOtlp(Counter)and addingRecording/Spanhelpers. - Updated source attribution to the upstream v4.5 tag/commit.
File summaries
| File | Description |
|---|---|
| site/docs/en/doc/profiler.md | Updates upstream reference links and displayed profiler version to v4.5. |
| site/docs/doc/profiler.md | Updates upstream reference links/version and minor wording/formatting adjustments. |
| core/src/main/java/one/profiler/Span.java | Adds span-recording helper API for async-profiler JFR recordings. |
| core/src/main/java/one/profiler/Recording.java | Adds recording state/timestamp utilities aligned with profiler clock. |
| core/src/main/java/one/profiler/package-info.java | Updates upstream attribution to v4.5 tag/commit. |
| core/src/main/java/one/profiler/AsyncProfilerMXBean.java | Updates MXBean API signature for OTLP dumping. |
| core/src/main/java/one/profiler/AsyncProfiler.java | Updates collapsed/OTLP command generation for new counter handling. |
| core/src/main/java/com/taobao/arthas/core/command/monitor200/ProfilerCommand.java | Updates upstream reference link to v4.5. |
Review details
Suppressed comments (1)
core/src/main/java/one/profiler/AsyncProfiler.java:252
AsyncProfiler.dumpOtlp()was removed, which is a binary-incompatible change for any code compiled against the previous API. Consider keeping a deprecated no-arg overload that delegates todumpOtlp(Counter.SAMPLES), and also fail fast if the newcounterparameter is null to avoid silently defaulting to "total".
@Override
public byte[] dumpOtlp(Counter counter) {
try {
return execute1("otlp," + (counter == Counter.SAMPLES ? "samples" : "total"));
} catch (IOException e) {
- Files reviewed: 8/11 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| String dumpCollapsed(Counter counter); | ||
| String dumpTraces(int maxTraces); | ||
| String dumpFlat(int maxMethods); | ||
| byte[] dumpOtlp(); | ||
| byte[] dumpOtlp(Counter counter); |
| @Override | ||
| public String dumpCollapsed(Counter counter) { | ||
| try { | ||
| return execute0("collapsed," + counter.name().toLowerCase()); | ||
| return execute0("collapsed," + (counter == Counter.SAMPLES ? "samples" : "total")); | ||
| } catch (IOException e) { |
1da7d82 to
260d990
Compare
Upgrade the bundled async-profiler from 4.4 to 4.5, which includes the upstream essential-library parsing fix for profiler loading.
11aaea310cb75843f0e7532365d8cedc8d0e14c8, includingdumpOtlp(Counter), while intentionally omitting the newRecordingandSpanclasses.core/src/main/java/one/profiler/package-info.javaand update the Chinese/English profiler version references.In 4.5, loading the native library can bind
Recordingand retain its defining ClassLoader through a JNI global reference, even if the Span API is never called. Stopping profiling does not release that reference; see async-profiler #1811. Arthas does not use the Java Span API, and the native library skips this optional binding whenRecordingis absent. Ordinary sampling, HTML flame graphs, and JFR output remain available.Spanis omitted together with itsRecordingdependency. Reconsider these classes when upstream addresses the reference lifecycle and Arthas needs the Span API.Validation:
./mvnw -B -ntp -pl core -am test -DargLine=-XX:+StartAttachListener— 331 tests passed, with no failures, errors, or skips. In the fresh worktree, first generated the vmtool dependency JAR with./mvnw -B -ntp -pl arthas-vmtool -am package -DskipTests../mvnw -B -ntp -pl core jar:jar— passed; verified that the core JAR contains the remaining Java 8 profiler API classes and noRecordingorSpanclasses.ArthasClassloader; version/status, CPU/HTML, CPU JFR, and wall/allocation/monitor-lock JFR checks passed. JDK 21jfrsyncalso passed. Parsed all generated JFR files and checked the expected event counts.git diff --checkpassed.Linux native runtime and cold-cache/many-library scenarios were not tested locally. Full distribution packaging was not rerun for the class-omission follow-up.
Closes #3266.