[RPC] Android RPC Performance Regression Fix, Update Android RPC to use Tracker #1457
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tqchen
requested changes
Jul 19, 2018
|
|
||
|
|
||
| /** | ||
| * Server processor with tracker connection (based on standalone). |
| private final SocketFileDescriptorGetter socketFileDescriptorGetter; | ||
| private final String trackerHost; | ||
| private final int trackerPort; | ||
| private final String key; |
| return trackerSocket; | ||
| } | ||
|
|
||
| private void register(Socket trackerSocket) throws IOException { |
| } | ||
| } | ||
|
|
||
| private Socket connectToTracker() throws IOException{ |
| continue; | ||
| } | ||
| int keyLen = Utils.wrapBytes(Utils.recvAll(in, 4)).getInt(); | ||
| recvKey = Utils.decodeToStr(Utils.recvAll(in, keyLen)); |
There was a problem hiding this comment.
Consider adds Utils.recvStr and Utils.sendStr since it is commonly used
| Socket trackerSocket = null; | ||
| String recvKey = null; | ||
| try { | ||
| trackerSocket = connectToTracker(); |
There was a problem hiding this comment.
Consider wrap a standalone class TrackerClient and move all related logics to there, to make the logic cleaner
| Socket trackerSocket = null; | ||
| String recvKey = null; | ||
| try { | ||
| // open a socket and handshake with tracker |
There was a problem hiding this comment.
Put more comment in here, document the general steps in the protocol
tqchen
requested changes
Jul 19, 2018
| } | ||
|
|
||
| // if we find the matchKey, we do not need to refresh | ||
| private boolean checkMatchKey(Socket trackerSocket) throws IOException { |
There was a problem hiding this comment.
This function name is a bit confusing, maybe we can do needRefreshMatchKey
|
Please update the android_rpc/README.md to include instructions to use rpc_tracker |
tqchen
approved these changes
Jul 20, 2018
|
|
@eqy can you followup on @merrymercy 's comment? |
tqchen
pushed a commit
to tqchen/tvm
that referenced
this pull request
Aug 4, 2018
sergei-mironov
pushed a commit
to sergei-mironov/tvm
that referenced
this pull request
Aug 8, 2018
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
#1433 and #1426 raise the issue of a performance regression introduced in #1387, due to a change in the Activity/Service Layout of the app.
Originally, the RPC App used a single process, and a single Activity for the entire app, comprising both the RPC Server and UI.
#1387 moved the RPC Server to an Android Service in an isolated process, and added a watchdog to the main Activity. Unfortunately, processes running Services in Android are treated as lower priority than processes running Activities, so this caused a CPU performance regression in Android RPC.
We investigated reversing the layout to run the watchdog in a ForegroundService and instead run the RPC server in an Activity, but keeping the (watchdog) Service alive when the RPC server's Activity and process crash proved to be difficult and unreliable. We now choose to run the RPC server in a separate Activity isolated from the main Activity's process. This way, if the RPC server crashes, it takes its Activity down by itself and resumes the main Activity of the app, which can trigger a restart of the RPC server. Using a separate Activity also addresses the CPU performance regression issue. (ForegroundServices also complicated handling API levels >= 26 vs. < 26, which we are now happy to avoid).
The watchdog implementation has also been tweaked to use
notifyandwaitinstead of a polling mechanism, which should minimize any additional CPU overhead (though we believe the regression was due to using a Service).Additionally, the introduction of the RPC Tracker in #1080 duplicates much of the functionality of the RPC Proxy that the Android RPC app was originally designed to use. We now switch the Android RPC app to use the RPC Tracker protocol.
This leaves us with a few UI changes to the Android app:
ON, the RPC server will restart itself)CC @alex-weaver @merrymercy @yzhliu