Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Logging][Hexagon] Improve logging on Hexagon #13072

Merged
merged 12 commits into from
Oct 28, 2022

Conversation

supersat
Copy link
Contributor

Currently Hexagon logging is done with a custom LogMessageImpl in hexagon_common.cc. This ends up calling HexagonLog and HEXAGON_PRINT which uses the HAP FARF API. Unfortunately, the TVM log level is lost along the way, with logs being produced at FARF’s ALWAYS level. This becomes especially noisy with RPC debug logging, which generates enough noise to cause some log data to be dropped. It also introduces a lot of useless noise, as the FARF API produces its own line number information, which only points to where hexagon_common.cc calls HEXAGON_PRINT. Using the HAP_debug_runtime API lets us pass the log level and file line information directly, and enables runtime selection of logging levels.

This commit explicitly passes the log level to LogMessage/LogMessageImpl and updates Hexagon's custom LogMessageImpl to use the HAP_debug_runtime API.

@tvm-bot
Copy link
Collaborator

tvm-bot commented Oct 13, 2022

Thanks for contributing to TVM! Please refer to the contributing guidelines https://tvm.apache.org/docs/contribute/ for useful information and tips. Please request code reviews from Reviewers by @-ing them in a comment.

Generated by tvm-bot

@supersat supersat force-pushed the improve-hexagon-logging branch from 2be47be to 046af15 Compare October 16, 2022 05:21
@supersat
Copy link
Contributor Author

@areusch areusch added needs-triage PRs or issues that need to be investigated by maintainers to find the right assignees to address it and removed needs-triage PRs or issues that need to be investigated by maintainers to find the right assignees to address it labels Oct 19, 2022
@supersat supersat marked this pull request as ready for review October 20, 2022 01:05
@supersat
Copy link
Contributor Author

cc: @areusch @kparzysz-quic

src/runtime/logging.cc Outdated Show resolved Hide resolved
@nverke
Copy link
Contributor

nverke commented Oct 20, 2022

LGTM! Love to see those clean log lines in the discussion post! I want to see if this improves perf at all.

Copy link
Collaborator

@janetsc janetsc left a comment

Choose a reason for hiding this comment

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

Thanks for doing this! I have just a few clarifying questions (which likely could be addressed with comments), but otherwise looks good.

include/tvm/runtime/logging.h Outdated Show resolved Hide resolved
src/runtime/hexagon/hexagon_common.cc Show resolved Hide resolved
src/runtime/hexagon/rpc/android_bash.sh.template Outdated Show resolved Hide resolved
Copy link
Member

@mehrdadh mehrdadh left a comment

Choose a reason for hiding this comment

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

overall looks good, added few comments

apps/ios_rpc/tvmrpc/TVMRuntime.mm Outdated Show resolved Hide resolved
src/runtime/hexagon/rpc/android_bash.sh.template Outdated Show resolved Hide resolved
Currently Hexagon logging is done with a custom LogMessageImpl in
hexagon_common.cc. This ends up calling HexagonLog and HEXAGON_PRINT
which uses the HAP FARF API. Unfortunately, the TVM log level is lost
along the way, with logs being produced at FARF’s ALWAYS level. This
becomes especially noisy with RPC debug logging, which generates enough
noise to cause some log data to be dropped. It also introduces a lot of
useless noise, as the FARF API produces its own line number information,
which only points to where hexagon_common.cc calls HEXAGON_PRINT. Using
the HAP_debug_v2 API lets us pass the log level and file line
information directly, and enables runtime selection of logging levels.

This commit explicity passes the log level to LogMessage/LogMessageImpl
and updates Hexagon's custom LogMessageImpl to use the HAP_debug_v2 API.
@supersat supersat force-pushed the improve-hexagon-logging branch from 41b1717 to fe1c5ba Compare October 28, 2022 01:15
@mehrdadh mehrdadh requested a review from nverke October 28, 2022 16:24
@mehrdadh
Copy link
Member

@supersat LGTM! I started a job on hardware. Will merge after that finishes

Copy link
Contributor

@kparzysz-quic kparzysz-quic left a comment

Choose a reason for hiding this comment

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

LGTM

@mehrdadh mehrdadh merged commit c0f33df into apache:main Oct 28, 2022
@supersat supersat deleted the improve-hexagon-logging branch October 28, 2022 19:59
xinetzone pushed a commit to daobook/tvm that referenced this pull request Nov 10, 2022
* [Logging][Hexagon] Improve logging on Hexagon

Currently Hexagon logging is done with a custom LogMessageImpl in
hexagon_common.cc. This ends up calling HexagonLog and HEXAGON_PRINT
which uses the HAP FARF API. Unfortunately, the TVM log level is lost
along the way, with logs being produced at FARF’s ALWAYS level. This
becomes especially noisy with RPC debug logging, which generates enough
noise to cause some log data to be dropped. It also introduces a lot of
useless noise, as the FARF API produces its own line number information,
which only points to where hexagon_common.cc calls HEXAGON_PRINT. Using
the HAP_debug_v2 API lets us pass the log level and file line
information directly, and enables runtime selection of logging levels.

This commit explicity passes the log level to LogMessage/LogMessageImpl
and updates Hexagon's custom LogMessageImpl to use the HAP_debug_v2 API.

* Adjust Hexagon rpc_server logging to use the DEBUG level

* Update hexagon_api launcher script to omit DEBUG-level logs

* Update WASM LogMessageImpl to accept explicit level

* Update Android LogMessageImpl to accept and forward explicit log level

* Move LogMessage::level_strings_ out of some ifdefs

* Update iOS LogMessageImpl to accept explicit log level

* Attempt to fix Windows build

* Add comments about runtime hexagon log level encodings

* Remove unneeded string processing in LogMessage

* Remove TODO

* Update HexagonLauncherAndroid to accept runtime log filtering configuration
xinetzone pushed a commit to daobook/tvm that referenced this pull request Nov 25, 2022
* [Logging][Hexagon] Improve logging on Hexagon

Currently Hexagon logging is done with a custom LogMessageImpl in
hexagon_common.cc. This ends up calling HexagonLog and HEXAGON_PRINT
which uses the HAP FARF API. Unfortunately, the TVM log level is lost
along the way, with logs being produced at FARF’s ALWAYS level. This
becomes especially noisy with RPC debug logging, which generates enough
noise to cause some log data to be dropped. It also introduces a lot of
useless noise, as the FARF API produces its own line number information,
which only points to where hexagon_common.cc calls HEXAGON_PRINT. Using
the HAP_debug_v2 API lets us pass the log level and file line
information directly, and enables runtime selection of logging levels.

This commit explicity passes the log level to LogMessage/LogMessageImpl
and updates Hexagon's custom LogMessageImpl to use the HAP_debug_v2 API.

* Adjust Hexagon rpc_server logging to use the DEBUG level

* Update hexagon_api launcher script to omit DEBUG-level logs

* Update WASM LogMessageImpl to accept explicit level

* Update Android LogMessageImpl to accept and forward explicit log level

* Move LogMessage::level_strings_ out of some ifdefs

* Update iOS LogMessageImpl to accept explicit log level

* Attempt to fix Windows build

* Add comments about runtime hexagon log level encodings

* Remove unneeded string processing in LogMessage

* Remove TODO

* Update HexagonLauncherAndroid to accept runtime log filtering configuration
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants