nuttxpr commented on PR #15636: URL: https://github.com/apache/nuttx/pull/15636#issuecomment-2606232851
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) Yes, this PR appears to meet the NuttX requirements, although some clarifications and additions would strengthen it. **Strengths:** * **Clear Summary:** The summary explains the problem (non-inline function leading to overhead), the solution (making it inline), and provides supporting evidence (cross-reference table demonstrating frequent calls). * **Impact:** Identifies the affected area (SMP scenario). * **Testing:** Mentions the tested targets. **Areas for Improvement:** * **Quantify the Performance Improvement:** The summary mentions improved performance, but lacks concrete data. Include benchmarks or profiling data showing the performance difference before and after the change. Even a rough estimate is better than nothing. * **Testing Logs:** The "Testing logs before change" and "Testing logs after change" sections are empty. Provide actual logs or output demonstrating the change's effect. This is crucial for verifying the claimed performance improvement and ensuring no regressions. If the improvement is only visible through profiling, provide those results. * **Impact - User:** While the user impact might be implicit (improved performance), explicitly stating "NO" would be clearer. * **Impact - Build:** State "NO" (unless the change affects the build process in some way). * **Impact - Hardware:** Be more specific. Does this change affect *all* Xtensa architectures or just ESP32S3 and ESP32? Clarify the scope of the hardware impact. * **Impact - Documentation:** State "NO" (unless documentation needs updating). * **Impact - Security:** State "NO" (unless there are security implications). * **Impact - Compatibility:** State "NO" (unless there are compatibility concerns). * **Build Host:** Specify your build host details as requested (OS, CPU, Compiler). **Example of improved Testing section:** ``` Testing I confirm that changes are verified on local setup and works as intended: * Build Host(s): Linux, x86_64, GCC 9.x * Target(s): esp32s3-devkit:smp, esp32-devkitc:smp Testing logs before change (execution time of test application): ``` time ./test_app real 0m1.500s user 0m1.000s sys 0m0.500s ``` Testing logs after change: ``` time ./test_app real 0m1.200s user 0m0.800s sys 0m0.400s ``` This demonstrates a ~20% reduction in execution time for the test application after making `up_cpu_index` inline. (Replace with your actual test application and results) ``` By addressing these points, you'll create a more robust and convincing PR that is easier for reviewers to assess and approve. -- 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
