Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2705063002:
Adding support for native profiling of AppRTCMobile (Closed)
|
Created:
3 years, 10 months ago by henrika_webrtc Modified:
3 years, 10 months ago Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdding support for native profiling of AppRTCMobile.
See perf_setup.sh for instructions.
NOTRY=TRUE
BUG=NONE
Review-Url: https://codereview.webrtc.org/2705063002
Cr-Commit-Position: refs/heads/master@{#16826}
Committed: https://chromium.googlesource.com/external/webrtc/+/7a1798d637ee34f84c3dc1b0a70ad5a62ab7ae9b
Patch Set 1 #Patch Set 2 : nit #
Total comments: 15
Patch Set 3 : Changes based on feedback from kjellander@ #Patch Set 4 : cleanup #Patch Set 5 : Added plot_flame_graph #
Total comments: 20
Patch Set 6 : Final changes #
Total comments: 1
Patch Set 7 : Fixed .gitignore #
Total comments: 1
Patch Set 8 : nit #
Messages
Total messages: 25 (10 generated)
Description was changed from ========== Adding support for native profiling of AppRTCMobile Adding support for native profiling of AppRTCMobile cleanup *** EXPERIMENTAL *** patch from issue 2701663002 at patchset 100001 (http://crrev.com/2701663002#ps100001) BUG= ========== to ========== Adding support for native profiling of AppRTCMobile BUG=NONE ==========
henrika@webrtc.org changed reviewers: + kjellander@webrtc.org
Description was changed from ========== Adding support for native profiling of AppRTCMobile BUG=NONE ========== to ========== Adding support for native profiling of AppRTCMobile. See perf_setup.sh for instructions. TODO: Upload existing tools to tools-webrtc/android/profiling. Will add simpleperf and Flame Graph. BUG=NONE ==========
Henrik, could you please check these scripts. It is a tool that will enable all users to profile native WebRTC on Android. My plan was to upload the external tools as well after having an OK on the basic script.
PTAL
Mainly minor things. Does this script pollute your environment a lot after sourced or is it just the perf_* functions that gets added? https://codereview.webrtc.org/2705063002/diff/20001/tools-webrtc/android/prof... File tools-webrtc/android/profiling/perf_setup.sh (right): https://codereview.webrtc.org/2705063002/diff/20001/tools-webrtc/android/prof... tools-webrtc/android/profiling/perf_setup.sh:61: unset BUILD_DIR Why unset it? https://codereview.webrtc.org/2705063002/diff/20001/tools-webrtc/android/prof... tools-webrtc/android/profiling/perf_setup.sh:135: # Allows usgage of running report commands on the device. usgage -> usage? https://codereview.webrtc.org/2705063002/diff/20001/tools-webrtc/android/prof... tools-webrtc/android/profiling/perf_setup.sh:237: # TODO(henrika): add comments... Which ones are you referring to? I think it's pretty well documented already below. https://codereview.webrtc.org/2705063002/diff/20001/tools-webrtc/android/prof... tools-webrtc/android/profiling/perf_setup.sh:254: if [[ -z "$ENVSETUP_GYP_CHROME_SRC" ]]; then This sounds like something that might go away. Maybe check some other variable? https://codereview.webrtc.org/2705063002/diff/20001/tools-webrtc/android/prof... File tools-webrtc/android/profiling/utilities.sh (right): https://codereview.webrtc.org/2705063002/diff/20001/tools-webrtc/android/prof... tools-webrtc/android/profiling/utilities.sh:63: function find_app_pid() { Maybe document what argument is expected to be passed (a string?) and what is returned (a PID integer?). https://codereview.webrtc.org/2705063002/diff/20001/tools-webrtc/android/prof... tools-webrtc/android/profiling/utilities.sh:131: adb shell setprop security.perf_harden 0 If the below checks for root, maybe this one also needs that? (or just skip both and assume rooted device). https://codereview.webrtc.org/2705063002/diff/20001/tools-webrtc/android/prof... tools-webrtc/android/profiling/utilities.sh:139: image_is_root \ If the device is not rooted this will just silently return. Isn't that potentially confusing? Do you even need to check, since you already have a check in the main script?
Thanks Henrik, done changes based on your feedback and also added support for downloading the tools we need. The ideas is that we now automatically clone what we need and then have the option to remove all parts when done using new methods. https://codereview.webrtc.org/2705063002/diff/20001/tools-webrtc/android/prof... File tools-webrtc/android/profiling/perf_setup.sh (right): https://codereview.webrtc.org/2705063002/diff/20001/tools-webrtc/android/prof... tools-webrtc/android/profiling/perf_setup.sh:61: unset BUILD_DIR Long story. But essentially. If I don't do it, we can reuse an old BUILD_DIR if we call the script without parameter after a round where one test failed. So, I need to unset to ensure that we can detect the error (no build dir) each time. https://codereview.webrtc.org/2705063002/diff/20001/tools-webrtc/android/prof... tools-webrtc/android/profiling/perf_setup.sh:135: # Allows usgage of running report commands on the device. On 2017/02/22 14:46:57, kjellander_webrtc wrote: > usgage -> usage? Done. https://codereview.webrtc.org/2705063002/diff/20001/tools-webrtc/android/prof... tools-webrtc/android/profiling/perf_setup.sh:237: # TODO(henrika): add comments... Removed. https://codereview.webrtc.org/2705063002/diff/20001/tools-webrtc/android/prof... tools-webrtc/android/profiling/perf_setup.sh:254: if [[ -z "$ENVSETUP_GYP_CHROME_SRC" ]]; then I have not found any other way actually. It works as is... https://codereview.webrtc.org/2705063002/diff/20001/tools-webrtc/android/prof... File tools-webrtc/android/profiling/utilities.sh (right): https://codereview.webrtc.org/2705063002/diff/20001/tools-webrtc/android/prof... tools-webrtc/android/profiling/utilities.sh:63: function find_app_pid() { On 2017/02/22 14:46:57, kjellander_webrtc wrote: > Maybe document what argument is expected to be passed (a string?) and what is > returned (a PID integer?). Done. https://codereview.webrtc.org/2705063002/diff/20001/tools-webrtc/android/prof... tools-webrtc/android/profiling/utilities.sh:131: adb shell setprop security.perf_harden 0 Good point. Will fix and align with function below. Not needed actually. https://codereview.webrtc.org/2705063002/diff/20001/tools-webrtc/android/prof... tools-webrtc/android/profiling/utilities.sh:139: image_is_root \ As you say. Not really needed since we check that in main.
Adding Fredrik as well since we are getting closer to something that works. I have a few more details to fix and will ping when I ask for a new review round. So, don't review just yet ;-)
Description was changed from ========== Adding support for native profiling of AppRTCMobile. See perf_setup.sh for instructions. TODO: Upload existing tools to tools-webrtc/android/profiling. Will add simpleperf and Flame Graph. BUG=NONE ========== to ========== Adding support for native profiling of AppRTCMobile. See perf_setup.sh for instructions. BUG=NONE ==========
henrika@webrtc.org changed reviewers: + solenberg@webrtc.org
Done more improvements. More or less plugg-and-play now ;-) Fredrik, please take a look as well. Thanks ;-)
rs lgtm - I'm not that well versed in shell scripting https://codereview.webrtc.org/2705063002/diff/80001/tools-webrtc/android/prof... File tools-webrtc/android/profiling/perf_setup.sh (right): https://codereview.webrtc.org/2705063002/diff/80001/tools-webrtc/android/prof... tools-webrtc/android/profiling/perf_setup.sh:2: I'm no bash expert, but I've learned that using set -e is a generally recommended. https://codereview.webrtc.org/2705063002/diff/80001/tools-webrtc/android/prof... tools-webrtc/android/profiling/perf_setup.sh:16: # Source this script once from the WebRTC source directory and resolve any Replace "WebRTC source directory" with webrtc/src/ ? https://codereview.webrtc.org/2705063002/diff/80001/tools-webrtc/android/prof... tools-webrtc/android/profiling/perf_setup.sh:27: # > perf_record perf_record 120 ? https://codereview.webrtc.org/2705063002/diff/80001/tools-webrtc/android/prof... tools-webrtc/android/profiling/perf_setup.sh:30: # > perf_cleanup Add a note about running perf_update after uploading new APK https://codereview.webrtc.org/2705063002/diff/80001/tools-webrtc/android/prof... tools-webrtc/android/profiling/perf_setup.sh:116: # Copy kernal symbols from device to symbol cache in tmp. kernal -> kernel https://codereview.webrtc.org/2705063002/diff/80001/tools-webrtc/android/prof... tools-webrtc/android/profiling/perf_setup.sh:271: } nit: blank line
Sorry but I have a few new comments I'd like to be fixed. https://codereview.webrtc.org/2705063002/diff/20001/tools-webrtc/android/prof... File tools-webrtc/android/profiling/perf_setup.sh (right): https://codereview.webrtc.org/2705063002/diff/20001/tools-webrtc/android/prof... tools-webrtc/android/profiling/perf_setup.sh:61: unset BUILD_DIR On 2017/02/22 15:47:11, henrika_webrtc wrote: > Long story. But essentially. If I don't do it, we can reuse an old BUILD_DIR if > we call the script without parameter after a round where one test failed. So, I > need to unset to ensure that we can detect the error (no build dir) each time. Acknowledged. https://codereview.webrtc.org/2705063002/diff/80001/tools-webrtc/android/prof... File tools-webrtc/android/profiling/perf_setup.sh (right): https://codereview.webrtc.org/2705063002/diff/80001/tools-webrtc/android/prof... tools-webrtc/android/profiling/perf_setup.sh:16: # Source this script once from the WebRTC source directory and resolve any On 2017/02/24 10:31:06, the sun wrote: > Replace "WebRTC source directory" with webrtc/src/ ? I'd suggest just WebRTC src/ directory (since what you name the one above is totally up to the user to decide). https://codereview.webrtc.org/2705063002/diff/80001/tools-webrtc/android/prof... tools-webrtc/android/profiling/perf_setup.sh:222: if app_is_running "${APP_NAME}"; then I suggest setting set -e in the functions that execute multiple commands, to prevent surprises if one command fails and the other ones just keep executing. Especially as there's several rm -rf invocations in the cleanup steps. https://codereview.webrtc.org/2705063002/diff/80001/tools-webrtc/android/prof... tools-webrtc/android/profiling/perf_setup.sh:411: # Download simpleperf to <src>/tools-webrtc/android/profiling/simpleperf/. Please add this to .gitignore so one doesn't have to stare at that simpleperf clone every time you do 'git status' https://codereview.webrtc.org/2705063002/diff/80001/tools-webrtc/android/prof... tools-webrtc/android/profiling/perf_setup.sh:447: # Download Flame Graph to <src>/tools-webrtc/android/profiling/flamegraph/. add another .gitignore entry for this
Great comments from both of you. Let me try to fix all parts in one last round.
Done. PTAL ;-) https://codereview.webrtc.org/2705063002/diff/80001/tools-webrtc/android/prof... File tools-webrtc/android/profiling/perf_setup.sh (right): https://codereview.webrtc.org/2705063002/diff/80001/tools-webrtc/android/prof... tools-webrtc/android/profiling/perf_setup.sh:2: Works in a clean script but this file must be sourced (to ensure that we load functions). If set -e is used, the complete terminal will crash (unless I add TRAP) and we don't want that. Hence, I will not use set -e ;-) PS I use return instead of exit for the same reason. https://codereview.webrtc.org/2705063002/diff/80001/tools-webrtc/android/prof... tools-webrtc/android/profiling/perf_setup.sh:16: # Source this script once from the WebRTC source directory and resolve any Using proposal from kjellander ;-) https://codereview.webrtc.org/2705063002/diff/80001/tools-webrtc/android/prof... tools-webrtc/android/profiling/perf_setup.sh:16: # Source this script once from the WebRTC source directory and resolve any On 2017/02/24 11:02:26, kjellander_webrtc wrote: > On 2017/02/24 10:31:06, the sun wrote: > > Replace "WebRTC source directory" with webrtc/src/ ? > > I'd suggest just WebRTC src/ directory (since what you name the one above is > totally up to the user to decide). Done. https://codereview.webrtc.org/2705063002/diff/80001/tools-webrtc/android/prof... tools-webrtc/android/profiling/perf_setup.sh:27: # > perf_record As is, 30 will be used as default. But I can add a parameter to show that it is possible to extend it. Also changing default from 30 to 60. https://codereview.webrtc.org/2705063002/diff/80001/tools-webrtc/android/prof... tools-webrtc/android/profiling/perf_setup.sh:30: # > perf_cleanup It is actually no longer needed. I added it to perf_record instead but you can still use it as a separate method. https://codereview.webrtc.org/2705063002/diff/80001/tools-webrtc/android/prof... tools-webrtc/android/profiling/perf_setup.sh:116: # Copy kernal symbols from device to symbol cache in tmp. On 2017/02/24 10:31:06, the sun wrote: > kernal -> kernel Done. https://codereview.webrtc.org/2705063002/diff/80001/tools-webrtc/android/prof... tools-webrtc/android/profiling/perf_setup.sh:222: if app_is_running "${APP_NAME}"; then Discussed off-line. Resolved. https://codereview.webrtc.org/2705063002/diff/80001/tools-webrtc/android/prof... tools-webrtc/android/profiling/perf_setup.sh:271: } On 2017/02/24 10:31:06, the sun wrote: > nit: blank line Done. https://codereview.webrtc.org/2705063002/diff/80001/tools-webrtc/android/prof... tools-webrtc/android/profiling/perf_setup.sh:411: # Download simpleperf to <src>/tools-webrtc/android/profiling/simpleperf/. Good idea. Will do. https://codereview.webrtc.org/2705063002/diff/80001/tools-webrtc/android/prof... tools-webrtc/android/profiling/perf_setup.sh:447: # Download Flame Graph to <src>/tools-webrtc/android/profiling/flamegraph/. On 2017/02/24 11:02:26, kjellander_webrtc wrote: > add another .gitignore entry for this Done.
https://codereview.webrtc.org/2705063002/diff/100001/webrtc/.gitignore File webrtc/.gitignore (right): https://codereview.webrtc.org/2705063002/diff/100001/webrtc/.gitignore#newcode30 webrtc/.gitignore:30: tools-webrtc/android/profiling/flamegraph/ Move to our top-level .gitignore instead.
Thanks! PTAL
lgtm but fix the nit before submit https://codereview.webrtc.org/2705063002/diff/120001/.gitignore File .gitignore (right): https://codereview.webrtc.org/2705063002/diff/120001/.gitignore#newcode55 .gitignore:55: /tools-webrtc/android/profiling/flamegraph sort alphabetically ;)
Description was changed from ========== Adding support for native profiling of AppRTCMobile. See perf_setup.sh for instructions. BUG=NONE ========== to ========== Adding support for native profiling of AppRTCMobile. See perf_setup.sh for instructions. NOTRY=TRUE BUG=NONE ==========
The CQ bit was checked by henrika@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from solenberg@webrtc.org, kjellander@webrtc.org Link to the patchset: https://codereview.webrtc.org/2705063002/#ps140001 (title: "nit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1487940666702630,
"parent_rev": "bb5136f9403833d764504df4b80a38167e49f0cb", "commit_rev":
"7a1798d637ee34f84c3dc1b0a70ad5a62ab7ae9b"}
Message was sent while issue was closed.
Description was changed from ========== Adding support for native profiling of AppRTCMobile. See perf_setup.sh for instructions. NOTRY=TRUE BUG=NONE ========== to ========== Adding support for native profiling of AppRTCMobile. See perf_setup.sh for instructions. NOTRY=TRUE BUG=NONE Review-Url: https://codereview.webrtc.org/2705063002 Cr-Commit-Position: refs/heads/master@{#16826} Committed: https://chromium.googlesource.com/external/webrtc/+/7a1798d637ee34f84c3dc1b0a... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/external/webrtc/+/7a1798d637ee34f84c3dc1b0a... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
