3628: docs: expand profiling guide with JVM and async-profiler coverage#43
3628: docs: expand profiling guide with JVM and async-profiler coverage#43martin-augment wants to merge 2 commits intomainfrom
Conversation
Rename profiling_native_code.md to profiling.md and add sections for async-profiler (unified JVM + native flame graphs), Java Flight Recorder, a tool comparison table, and practical tips for profiling Comet's mixed JVM/Rust execution.
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the documentation for profiling Comet applications by consolidating and expanding existing guides into a single, comprehensive resource. It aims to provide developers with clear instructions and recommendations for using various profiling tools across JVM and native Rust components, improving the ability to diagnose performance issues and optimize code effectively. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
PR Review: docs: expand profiling guide with JVM and async-profiler coverageThis is a well-written, substantial improvement to the contributor documentation. Replacing the narrow I verified that the A few issues to address: Bug: Broken relative links (appears twice)The link The correct relative path is -See [benchmarks/tpc/README.md](../../benchmarks/tpc/README.md) for details.
+See [benchmarks/tpc/README.md](../../../benchmarks/tpc/README.md) for details.Bug: Missing
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request significantly improves the profiling documentation by consolidating information about native code profiling and expanding it to cover JVM profiling with JFR and unified profiling with async-profiler. The new profiling.md guide is comprehensive and provides clear instructions and examples for different tools and scenarios. I've made a couple of minor suggestions to improve clarity and correct a grammatical error in the new documentation.
| For continuous recording without a fixed duration: | ||
|
|
||
| ```shell | ||
| --conf "spark.driver.extraJavaOptions=-XX:StartFlightRecording=disk=true,maxsize=500m,filename=driver.jfr" |
There was a problem hiding this comment.
This example for continuous JFR recording is presented as a single --conf flag, which is an incomplete command. For clarity and consistency with the preceding example, it would be better to show the full spark-submit command structure. This also provides an opportunity to include the corresponding spark.executor.extraJavaOptions which is important for profiling Spark applications.
| --conf "spark.driver.extraJavaOptions=-XX:StartFlightRecording=disk=true,maxsize=500m,filename=driver.jfr" | |
| spark-submit \ | |
| --conf "spark.driver.extraJavaOptions=-XX:StartFlightRecording=disk=true,maxsize=500m,filename=driver.jfr" \ | |
| --conf "spark.executor.extraJavaOptions=-XX:StartFlightRecording=disk=true,maxsize=500m,filename=executor.jfr" \ | |
| ... |
| harness = false | ||
| ``` | ||
|
|
||
| These benchmarks are useful when for comparing performance between releases or between feature branches and the |
There was a problem hiding this comment.
There's a minor grammatical error here. The phrase 'when for' should likely be just 'for'.
| These benchmarks are useful when for comparing performance between releases or between feature branches and the | |
| These benchmarks are useful for comparing performance between releases or between feature branches and the |
There was a problem hiding this comment.
value:good-to-have; category:documentation; feedback: The Gemini AI reviewer is correct! There is a minor typo in the sentence that should be corrected to prevent an incorrect grammar.
🤖 Augment PR SummarySummary: This PR consolidates and expands the contributor profiling documentation into a single cross-stack guide for Comet. Changes:
Why: Provide a clearer “choose the right profiler” reference for Comet development, covering JVM + native hot paths as well as native-only micro-benchmark work. 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| Adding a New Expression <adding_a_new_expression> | ||
| Tracing <tracing> | ||
| Profiling Native Code <profiling_native_code> | ||
| Profiling <profiling> |
There was a problem hiding this comment.
Since this renames/removes the old profiling_native_code page, it’s worth double-checking there aren’t any remaining references (internal links, bookmarks) to profiling_native_code that would now become broken in the built docs.
Severity: low
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
There was a problem hiding this comment.
value:good-to-have; category:documentation; feedback: The Augment AI reviewer is correct! Since a documentation page is removed it is good to check whether that page is referred by another page! I just double checked and there are no other references.
| ```shell | ||
| # Linux x64 | ||
| wget https://github.com/async-profiler/async-profiler/releases/download/v3.0/async-profiler-3.0-linux-x64.tar.gz | ||
| tar xzf async-profiler-3.0-linux-x64.tar.gz -C /opt/async-profiler --strip-components=1 |
There was a problem hiding this comment.
tar ... -C /opt/async-profiler will fail if /opt/async-profiler doesn’t already exist (and writing to /opt often requires root), so readers following this verbatim may hit an error.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
There was a problem hiding this comment.
value:good-to=have; category:documentation; feedback: The Augment AI reviewer is correct! The /opt folder may not exist or may exist but usually requires root permissions. To prevent any confusion in the readers it would be better to use some folder in the user's $HOME
| ```shell | ||
| spark-submit \ | ||
| --conf "spark.driver.extraJavaOptions=-agentpath:/opt/async-profiler/lib/libasyncProfiler.so=start,event=cpu,file=driver.html" \ | ||
| --conf "spark.executor.extraJavaOptions=-agentpath:/opt/async-profiler/lib/libasyncProfiler.so=start,event=cpu,file=executor.html" \ |
There was a problem hiding this comment.
Using fixed output filenames like executor.html/executor.jfr can lead to profiles being overwritten if multiple executors/JVMs write into the same working directory on a host.
Severity: low
Other Locations
docs/source/contributor-guide/profiling.md:138
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
There was a problem hiding this comment.
value:good-but-wont-fix; category:documentation; feedback: The Augment AI reviewer is correct! The profiling output files would be overwritten but usually this is the needed behavior. The developer makes some improvement and reloads the page in the browser to see whether it has a good effect. If the developer wants to compare results then (s)he need to generate unique file names, e.g. with a timestamp.
| ### Integrated benchmark profiling | ||
|
|
||
| The TPC benchmark scripts in `benchmarks/tpc/` have built-in async-profiler support via | ||
| the `--async-profiler` flag. See [benchmarks/tpc/README.md](../../benchmarks/tpc/README.md) |
There was a problem hiding this comment.
The relative link to ../../benchmarks/tpc/README.md points outside the docs source tree, which may render as a broken link in the published documentation build (even though it works in the repo).
Severity: medium
Other Locations
docs/source/contributor-guide/profiling.md:177
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
There was a problem hiding this comment.
value:useful; category: documentation; feedback: The Augment AI reviewer is correct! The link is broken. It should use three "go up"s to point to the wanted document. Prevents releasing documentation with broken links.
value:useful; category: documentation; feedback: The Claude AI reviewer is correct! The link is broken. It should use three "go up"s to point to the wanted document. Prevents releasing documentation with broken links. |
value:good-to=have; category:documentation; feedback: The Claude AI reviewer is correct! The /opt folder may not exist or may exist but usually requires root permissions. To prevent any confusion in the readers it would be better to use some folder in the user's $HOME |
2 participants
3628: To review by AI