Skip to content

Conversation

IvanKobzarev
Copy link
Contributor

@IvanKobzarev IvanKobzarev commented Oct 25, 2019

ghstack-source-id: 5edaf471557c25098ca0547229f2763760866887
Pull Request resolved: #28708

Some cpp formatting changes as I run clang-format -i

Testing on devserver:
make assets (models):

pushd android/test_app/; python make_assets.py; popd

Build test_app apk:

TRACE_ENABLED=1 sh android/build_test_app.sh

find . -type f -name *apk 
./android/test_app/app/build/outputs/apk/mobNet2Quant/debug/test_app-mobNet2Quant-debug.apk
./android/test_app/app/build/outputs/apk/resnet18/debug/test_app-resnet18-debug.apk

Install apk:
adb install -r test_app-mobNet2Quant-debug.apk
Run app on the device.
Systrace:

$ANDROID_HOME/platform-tools/systrace/systrace.py -t 10 -a org.pytorch.testapp.mobNet2Quant sched freq idle am wm gfx view binder_driver hal dalvik camera input res -o trace.html

trace.html contains sections like jni::Module::forward

Screenshot 2019-11-12 18 36 30

@IvanKobzarev IvanKobzarev force-pushed the ik_jni_tracing branch 2 times, most recently from b6ea5d5 to d9363de Compare October 25, 2019 22:17
@IvanKobzarev IvanKobzarev requested a review from dreiss October 25, 2019 22:17

namespace pytorch_jni {

#ifdef TRACE_ENABLED
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would push this down to everywhere that you're currently doing #ifdef __ANDROID__. That way, you don't have to put guards around all the Trace calls. Optimized builds with tracing not enabled will just inline all the calls and you won't take any speed or size hit.

@IvanKobzarev IvanKobzarev force-pushed the ik_jni_tracing branch 4 times, most recently from daec0dd to 5d64730 Compare November 13, 2019 22:06
typedef void* (*fp_ATrace_beginSection)(const char* sectionName);
typedef void* (*fp_ATrace_endSection)(void);

static void* (*ATrace_beginSection)(const char* sectionName);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe use the typedefs here?

bool Trace::is_initialized_ = false;

#if defined(TRACE_ENABLED) && defined(__ANDROID__)
Trace::fp_ATrace_beginSection Trace::ATrace_beginSection = {};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would drop the = {} here. Static scalars are always initialized to null/0, and using {} with a scalar feels weird to me. Just preference, though.

Trace::fp_ATrace_endSection Trace::ATrace_endSection = {};
#endif

void Trace::ensureInit() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider moving all of these function definitions (except for init) into the class definition in the header so they can be inlined.

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@IvanKobzarev has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@IvanKobzarev has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@IvanKobzarev merged this pull request in aa6e992.

csarofeen pushed a commit to mruberry/pytorch that referenced this pull request Nov 18, 2019
Summary:
ghstack-source-id: 5edaf47
Pull Request resolved: pytorch#28708

Some cpp formatting changes as I run `clang-format -i`

Testing on devserver:
make assets (models):
```
pushd android/test_app/; python make_assets.py; popd
```
Build test_app apk:
```
TRACE_ENABLED=1 sh android/build_test_app.sh

find . -type f -name *apk
./android/test_app/app/build/outputs/apk/mobNet2Quant/debug/test_app-mobNet2Quant-debug.apk
./android/test_app/app/build/outputs/apk/resnet18/debug/test_app-resnet18-debug.apk
```

Install apk:
`adb install -r test_app-mobNet2Quant-debug.apk`
Run app on the device.
Systrace:
```
$ANDROID_HOME/platform-tools/systrace/systrace.py -t 10 -a org.pytorch.testapp.mobNet2Quant sched freq idle am wm gfx view binder_driver hal dalvik camera input res -o trace.html
```
trace.html contains sections like `jni::Module::forward`

![Screenshot 2019-11-12 18 36 30](https://user-images.githubusercontent.com/6638825/68728156-5d245580-057b-11ea-9e71-e47681894fe4.png)
Pull Request resolved: pytorch#28712

Differential Revision: D18495898

Pulled By: IvanKobzarev

fbshipit-source-id: 0bced4a442f9dd90525520972a2c1f5d51f57df3
@facebook-github-bot facebook-github-bot deleted the ik_jni_tracing branch July 13, 2020 17:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants