nuttxpr commented on PR #15381: URL: https://github.com/apache/nuttx/pull/15381#issuecomment-2565001366
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) No, this PR description does not adequately meet the NuttX requirements. While concise, it lacks crucial details. Here's a breakdown: **Missing Information in Summary:** * **Why is inlining necessary?** Quantify the performance improvement expected. "Better performance" is vague. Provide benchmarks or profiling data if possible. * **What function is being inlined?** The PR needs to specify the exact function name. * **How does the inlining work?** This seems trivial for inlining, but a brief mention of the mechanism (e.g., compiler optimization, manual inlining) would be helpful. * **Issue references:** If this addresses a reported performance issue, link the relevant NuttX issue. **Missing Information in Impact:** * **Is new feature added? Is existing feature changed?** This is a performance optimization, so it's likely changing an existing feature. State that explicitly. * **Impact on user:** Even if the answer is NO, explicitly state it. * **Impact on build:** Likely NO, but state it explicitly. * **Impact on hardware:** While it says "RISCV Only," it needs to specify *which* RISC-V platforms and why only RISC-V. Is this a change to RISC-V-specific code? Does the inlining optimization only apply to RISC-V compilers? Be specific. * **Impact on documentation:** Likely NO, but state it explicitly. * **Impact on security, compatibility:** Explicitly state NO for each. **Missing Information in Testing:** * **Build Host(s):** Be specific about the OS, CPU, and compiler version used for local testing. * **Target(s):** Be very specific about the RISC-V architecture, board, and configuration tested. "sim" is not enough detail if a simulator was used. * **Testing logs:** Providing "before" and "after" logs is essential. These logs should demonstrate the performance improvement. Simply saying "Local machine & CI" is insufficient. Include relevant performance metrics (e.g., execution time, CPU cycles). CI results can be linked later when available, but initial local testing results are crucial for review. **Example of an Improved PR Description:** ``` ## Summary This PR inlines the `function_name()` function to improve performance on RISC-V platforms. Profiling revealed that `function_name()` is a frequent call site and inlining it is expected to reduce function call overhead by approximately X%. This addresses the performance issue reported in [NuttX Issue #XYZ]. ## Impact * **Is new feature added? Is existing feature changed?** YES, existing feature (performance of `function_name()`) is changed. * **Impact on user:** NO * **Impact on build:** NO * **Impact on hardware:** YES, this change specifically targets RISC-V platforms (e.g., `board1:config1`, `board2:config2`). The optimization is implemented using a compiler-specific attribute that is only available for the RISC-V compiler used. * **Impact on documentation:** NO * **Impact on security:** NO * **Impact on compatibility:** NO * **Anything else to consider:** N/A ## Testing I confirm that changes are verified on local setup and works as intended: * **Build Host(s):** Linux (Ubuntu 20.04), x86_64, GCC 10.2.1 * **Target(s):** RISC-V (QEMU virt, config: `xyz_config`), SiFive HiFive Unleashed (config: `abc_config`) Testing logs before change: ``` # Output of `time command` or other relevant performance measurement tool ... Execution time: 1.234 seconds ... ``` Testing logs after change: ``` # Output of `time command` or other relevant performance measurement tool ... Execution time: 0.987 seconds ... ``` ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
