[iOS] Add tracker support into ios-rpc application #7876

apeskov · 2021-04-18T22:50:59Z

Some set of improvements for iOS RPC application. The main point of changes is in build procedure and introducing support of direct RPC connection and connection via Tracker.

Build process improvements

Direct linking with tvm_runtime.dylib. Previous approach using direct include of source files. Some users and developer face issues with with missed functionality because and have to manually add several more include lines. This patch make iOS PRC server linked with tvm_runtime.dylib directly to eliminate this disadvantage.
Enabled ARC by default
Disabled bitcode. The latest changes in tvm build flags introduces an -undefined,error linker flag which is not compatible with bitcode.
Custom DSO loader is enabled by default. A lot of person are forgetting to enable it during manual build.
Move TVM_LOG_CUSTOMIZE=1 define moved from sources to project level. Previous it was leading to ODR violation is some cases.
Custom DSO sub project pointer is up. To fix internal memory leak.

UI changes

Hide keyboard when editing is finished.
Store text fields values in app cache. No need to manually fill it each time.
Introduce mode selector "Tracker/Proxy/PureRPC"
Single button to connect/disconnect with enhanced size

RPC functional changes

Introduced command line flags to run server form host side. Required to implement automatic RPC server runner.

--host_url The tracker/proxy address, Default=0.0.0.0"
--host_port The tracker/proxy port, Default=9190"
--key The key used to identify the device type in tracker. Default=""
--custom_addr Custom IP Address to Report to RPC Tracker. Default=""
--immediate_connect No UI interconnection, connect to tracker immediately. Default=False
--verbose Allow to print status info to std out. Default=False
--server_mode Server mode. Can be "pure_server", "proxy" or "tracker".

The printing of server status into std out is added (also print information about real IP in wifi network and RPC server port). Required to check server state from host runner.
Split ViewController and RPC Server login into different classes
Avoid using internal interface of RPCEndpoint::Create. Switched to publicly available Registry::Get("rpc.CreateEventDrivenServer")
Make RPC server api asynchronous. RPC server is executed in separate thread. No more blocking of UI.

Note:
Arguments support and status printing are a prerequisites for implementation python based RPC Server launcher (on top of iOS-deploy and simctl tools). Will be introduced a little bit later.

apps/ios_rpc/tvmrpc/TVMRuntime.mm

apps/ios_rpc/tvmrpc/rpc_server.h

areusch · 2021-05-24T23:38:10Z

apps/ios_rpc/tvmrpc/rpc_server.h

+   * \param ping_period Timeout for select call waiting
+   */
+  void AcceptConnection(TrackerClient* tracker, support::TCPSocket* conn_sock,
+                        support::SockAddr* addr, std::string* opts, int ping_period = 2) {


specify units: ping_period_sec

The same as previous comment. Copy-paste form cpp_rpc. Already removed. You may move this comment into original cpp_rpc application

tvm/apps/cpp_rpc/rpc_server.cc

Line 238 in 1c251f5

support::SockAddr* addr, std::string* opts, int ping_period = 2) {

areusch · 2021-05-24T23:38:51Z

apps/ios_rpc/tvmrpc/rpc_server.h

+   * \param opts The option string
+   */
+  int GetTimeOutFromOpts(const std::string& opts) const {
+    const std::string option = "-timeout=";


not sure if this should also have units? -timeout-sec

Agree. And already removed.

areusch · 2021-05-24T23:39:43Z

apps/ios_rpc/tvmrpc/TVMRuntime.mm

@@ -71,88 +73,19 @@ void LogMessageImpl(const std::string& file, int lineno, const std::string& mess
 namespace tvm {
 namespace runtime {

-class NSStreamChannel final : public RPCChannel {


just curious why this is going away? not needed if we use standard sockets?

Moved into "RPCServer.mm". And now it's wrapped into PackedFunction to meet API requirements of rpc.CreateEventDrivenServer.

jwfromm · 2021-06-07T16:06:25Z

@apeskov Can you address @areusch's comments? It looks like you made a lot of his recommended changes but it'd be good to make sure you're both on the same page.

apeskov · 2021-06-14T22:43:43Z

@areusch I've tried to answer your comments. Could you please continue a review?

Some part of code you pointed previously to improve was redesigned with latest commits, specially the code which was borrowed from cpp_rpc. I hope the new one will not cause a lot of questions.

areusch

@apeskov thanks! a couple more questions--mainly it would be helpful for me to understand the use case around introducing the gRPC transport. it's not that i'm opposed to doing that, but I'd like to understand why we may need it since it does add some complexity.

areusch · 2021-06-15T20:52:32Z

apps/ios_rpc/tvmrpc/RPCArgs.mm

+
+  bool immediate_connect = false;
+  bool verbose = false;
+  char server_mode = 0;


should that be an enum?

Sure, it could be enum. I've changed it to use enum form RPCServer.h.

areusch · 2021-06-15T20:53:11Z

apps/ios_rpc/tvmrpc/RPCArgs.mm

+      args.server_mode = 0;
+    } else if (server_mode == "proxy") {
+      args.server_mode = 1;
+    } else if (server_mode == "pure_server") {


what's the use case for gRPC?

I've tried to answer in reply message.

apps/ios_rpc/tvmrpc/ViewController.mm

areusch · 2021-06-15T22:05:28Z

apps/ios_rpc/tvmrpc/RPCArgs.h

+
+  /// Immediate server launch. No UI interaction.
+  /// 0 - UI interaction, 1 - automatically connect on launch
+  char immediate_connect;


why do you represent bool quantities with char? (instead of bool or uint8_t)

You are right, bool should be used here. Will change.

Originally it was a simplest way to share API between code written on Obj-C, Obj-C++, and C++ ("BOOL" and "bool" are not so compatible). But now there is no such need.

areusch · 2021-06-15T22:05:50Z

apps/ios_rpc/tvmrpc/RPCArgs.mm

+  [defaults setObject:[NSString stringWithUTF8String:g_rpc_args.host_url.c_str()]
+               forKey:@"tmvrpc_url"];
+  [defaults setInteger:g_rpc_args.host_port forKey:@"tmvrpc_port"];
+  [defaults setObject:[NSString stringWithUTF8String:g_rpc_args.key.c_str()] forKey:@"tmvrpc_key"];


nit: tvm, here and above

areusch · 2021-06-15T22:08:07Z

apps/ios_rpc/tvmrpc/RPCServer.mm

+/*!
+ */
+- (NSData*)requestInputDataPacked {
+  int size;


prefer to use the inttypes when taking sizeof() to compute payload lengths for wire data

Agree. Moved to int32_t everywhere.

areusch · 2021-06-15T22:08:35Z

apps/ios_rpc/tvmrpc/RPCServer.mm

+  switch (state_) {
+    case RPCServerProxyState_HandshakeToSend: {
+      // Send together kRPCMagic and server descriptor because of Proxy
+      int code = tvm::runtime::kRPCMagic;


apps/ios_rpc/tvmrpc/RPCServer.mm

apeskov · 2021-06-18T13:54:56Z

@apeskov thanks! a couple more questions--mainly it would be helpful for me to understand the use case around introducing the gRPC transport. it's not that i'm opposed to doing that, but I'd like to understand why we may need it since it does add some complexity.

Will assume that gRPC == PureRPC in terminology of this patch.

At first, I would like to note that "PureRPC" is a part of "Tracker" mode. The implementation of "Tracker" server mode is using using "PureRPC" as internal server. I've just expose it outside for person who would like to use direct connection to iOS without tracker/proxy. Removing this piece of functionality will not simplify implementation.

The main goal was to allow user and developers to work with iOS without requirements to keep tracker/proxy server. As example I have one laptop and one iOS device connected to it via USB (there are no local WiFi network at all, or it is very slow). With using PureRPC mode plus port tunnelling (via usbmux tool) I can work with such setup.

In addition to this patch I have a plan to introduce iOSRPCRunner script which will allow me to automate iOS test with quite simple code like next:

    # Will use first connected iOS device with automatic TCP port tunnelling to host
    ios_rpc_serv = RPCServerIOS.create_ios_rpc_server(key=key, mode="pure_server")    
    remote = rpc.connect(ios_rpc_serv.host, ios_rpc_serv.port, key=key)

    do_some_test_with_ios(remote)

Port tunnelling may significantly speed up work with iOS device. My local benchmarking shows 3x-4x speed depends on kind of workload (yes, wifi router matter).

Definitely, the same speed up result can be also achieved with tracker. For this purpose I've introduced flag --custom_addr, which help to use USB port tunnelling (as result host address replacing) for tracker clients. But this solution still require to have iOS device and host machine connected to one local WiFi network.

Totally, from my point of view, PureRPC is not so hard to support feature, but it can simplify/speedup my scripts in some cases :-)

apeskov · 2021-06-18T21:11:52Z

@areusch could you please continue review. I resolved all your comments excluding two. I've tried to explain in corresponding replies why I don't want to change it. Hope it will be enough.

areusch

@apeskov sorry i think the thing i'm confused about is: what functionality does RPCServerPure add over the existing tracker (or: why was the existing tracker not able to be run over usbmux)? perhaps it would be helpful to see an e.g. README.md explaining how to use RPCServerPure. Does it e.g. allow you to start/stop the tracker or RPC server? sorry my obj-c/iOS is not super great so I keep trying to review this but going in circles.

areusch · 2021-07-02T19:27:24Z

apps/ios_rpc/tvmrpc/RPCServer.mm

+ */
+FEventHandler CreateServerEventHandler(NSOutputStream* outputStream, std::string name,
+                                       std::string remote_key) {
+  const PackedFunc* event_handler_factor = Registry::Get("rpc.CreateEventDrivenServer");


nit: event_handler_factory

areusch · 2021-09-07T21:14:44Z

@apeskov just reviving this--please address the comment

echuraev · 2021-09-08T05:26:42Z

@apeskov just reviving this--please address the comment

Hello @areusch! @apeskov is busy with other activities. I'm working to finish this PR. What about your comment. I already fixed the typo, and now I'm working on adding a detailed instruction which will help to understand the new workflow and how it works.
Also, I'm going to clean up the ios_rpc application and remove old code.

Signed-off-by: Alexander Peskov <peskovnn@gmail.com>

Also containes: * Links with tvm_runtime.dylib * Minor improvements from UX perspective * Add cli args support * Add caching for url/port/key attributes Signed-off-by: Alexander Peskov <peskovnn@gmail.com>

Signed-off-by: Alexander Peskov <peskovnn@gmail.com>

Also: - Disabled bit-code - Enabled ARC - Use custom DSO loader by default - Single button to connect/disconnect - Add verbose flag Signed-off-by: Alexander Peskov <peskovnn@gmail.com>

Signed-off-by: Alexander Peskov <peskovnn@gmail.com>

echuraev · 2021-09-22T08:26:51Z

thanks @echuraev could we rename Pure to Standalone? or happy to hear other options. I think standalone makes sense to me--that would be --server_mode standalone and that would remove the name conflict with PureRPC.

Renamed to standalone

areusch

LGTM thanks @apeskov !

areusch · 2021-09-22T19:32:20Z

and thanks @echuraev !

* [IOS-RPC] Missprint in flag value Signed-off-by: Alexander Peskov <peskovnn@gmail.com> * [IOS-RPC] custom_dyld up commit id. Fix mem leak Signed-off-by: Alexander Peskov <peskovnn@gmail.com> * [IOS-RPC] build without scheme Signed-off-by: Alexander Peskov <peskovnn@gmail.com> * [IOS-RPC] Add tracker support into ios-rpc app Also containes: * Links with tvm_runtime.dylib * Minor improvements from UX perspective * Add cli args support * Add caching for url/port/key attributes Signed-off-by: Alexander Peskov <peskovnn@gmail.com> * [IOS-RPC] lint fix Signed-off-by: Alexander Peskov <peskovnn@gmail.com> * [IOS-RPC] Uniform servers Also: - Disabled bit-code - Enabled ARC - Use custom DSO loader by default - Single button to connect/disconnect - Add verbose flag Signed-off-by: Alexander Peskov <peskovnn@gmail.com> * [IOS_RPC] Min changes. Fix warnings Signed-off-by: Alexander Peskov <peskovnn@gmail.com> * [IOS-RPC] Fix review comments Signed-off-by: Alexander Peskov <peskovnn@gmail.com> * Fix RPC connection to tracker * Fix typo * Fix build for local developer profile * Add tvmrpc xcode scheme * Revert unnecessary change * Remove old mechanism of reloading libs * Display ip and port for PureRPC mode * Update tests * Remove tvmLauncher from ios_rpc * Add updating tvm_build_dir in init_proj script * Update default bundle * Update README.md for ios_rpc * Fix lint * Apply comments * Rename PureRPC to Standalone Co-authored-by: Egor Churaev <egor.churaev@gmail.com>

jcf94 added the status: need review label Apr 19, 2021

jwfromm requested a review from areusch May 24, 2021 16:36

areusch requested changes May 24, 2021

View reviewed changes

apeskov force-pushed the ap/ios-rpc-with-tracker branch 3 times, most recently from fe5622e to c3765ea Compare May 31, 2021 15:35

apeskov force-pushed the ap/ios-rpc-with-tracker branch from 3970ee7 to e6f6f08 Compare June 11, 2021 15:25

areusch reviewed Jun 15, 2021

View reviewed changes

areusch self-assigned this Jul 2, 2021

areusch reviewed Jul 2, 2021

View reviewed changes

areusch added status: need update need update based on feedbacks and removed status: need review labels Jul 7, 2021

echuraev force-pushed the ap/ios-rpc-with-tracker branch from d7049cf to 84bd23c Compare September 7, 2021 09:47

apeskov requested review from comaniac, Hzfengsy, jcf94, jroesch, junrushao, merrymercy, tqchen, vinx13, yzhliu and zhiics as code owners September 7, 2021 09:47

echuraev force-pushed the ap/ios-rpc-with-tracker branch from 84bd23c to 2b363ac Compare September 15, 2021 04:19

apeskov and others added 23 commits September 22, 2021 09:30

[IOS-RPC] Missprint in flag value

1035926

Signed-off-by: Alexander Peskov <peskovnn@gmail.com>

[IOS-RPC] custom_dyld up commit id. Fix mem leak

a4794c5

Signed-off-by: Alexander Peskov <peskovnn@gmail.com>

[IOS-RPC] build without scheme

55be91e

Signed-off-by: Alexander Peskov <peskovnn@gmail.com>

[IOS-RPC] Add tracker support into ios-rpc app

bcdff79

Also containes: * Links with tvm_runtime.dylib * Minor improvements from UX perspective * Add cli args support * Add caching for url/port/key attributes Signed-off-by: Alexander Peskov <peskovnn@gmail.com>

[IOS-RPC] lint fix

72a364b

Signed-off-by: Alexander Peskov <peskovnn@gmail.com>

[IOS-RPC] Uniform servers

cb3c00c

Also: - Disabled bit-code - Enabled ARC - Use custom DSO loader by default - Single button to connect/disconnect - Add verbose flag Signed-off-by: Alexander Peskov <peskovnn@gmail.com>

[IOS_RPC] Min changes. Fix warnings

9e03259

Signed-off-by: Alexander Peskov <peskovnn@gmail.com>

[IOS-RPC] Fix review comments

41e2d12

Signed-off-by: Alexander Peskov <peskovnn@gmail.com>

Fix RPC connection to tracker

3216c46

Fix typo

7eb6c02

Fix build for local developer profile

dbb743c

Add tvmrpc xcode scheme

c76640e

Revert unnecessary change

81621be

Remove old mechanism of reloading libs

7cb2b84

Display ip and port for PureRPC mode

ff9355c

Update tests

7cb54c6

Remove tvmLauncher from ios_rpc

d47314c

Add updating tvm_build_dir in init_proj script

c7d54c9

Update default bundle

9447285

Update README.md for ios_rpc

8526a9a

Fix lint

35589e4

Apply comments

d43039c

Rename PureRPC to Standalone

f6a2f8f

echuraev force-pushed the ap/ios-rpc-with-tracker branch from ca092c9 to f6a2f8f Compare September 22, 2021 07:40

areusch approved these changes Sep 22, 2021

View reviewed changes

areusch merged commit 76d0534 into apache:main Sep 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[iOS] Add tracker support into ios-rpc application #7876

[iOS] Add tracker support into ios-rpc application #7876

apeskov commented Apr 18, 2021 •

edited

Loading

areusch May 24, 2021

apeskov Jun 14, 2021

areusch May 24, 2021

apeskov Jun 14, 2021

areusch May 24, 2021

apeskov Jun 14, 2021

jwfromm commented Jun 7, 2021

apeskov commented Jun 14, 2021

areusch left a comment

areusch Jun 15, 2021

apeskov Jun 18, 2021

areusch Jun 15, 2021

apeskov Jun 18, 2021

areusch Jun 15, 2021

apeskov Jun 17, 2021

areusch Jun 15, 2021

apeskov Jun 18, 2021

areusch Jun 15, 2021

apeskov Jun 18, 2021

areusch Jun 15, 2021

apeskov Jun 18, 2021

apeskov commented Jun 18, 2021

apeskov commented Jun 18, 2021

areusch left a comment •

edited

Loading

areusch Jul 2, 2021

areusch commented Sep 7, 2021

echuraev commented Sep 8, 2021 •

edited

Loading

echuraev commented Sep 22, 2021

areusch left a comment

areusch commented Sep 22, 2021

[iOS] Add tracker support into ios-rpc application #7876

[iOS] Add tracker support into ios-rpc application #7876

Conversation

apeskov commented Apr 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jwfromm commented Jun 7, 2021

apeskov commented Jun 14, 2021

areusch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apeskov commented Jun 18, 2021

apeskov commented Jun 18, 2021

areusch left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

areusch commented Sep 7, 2021

echuraev commented Sep 8, 2021 • edited Loading

echuraev commented Sep 22, 2021

areusch left a comment

Choose a reason for hiding this comment

areusch commented Sep 22, 2021

apeskov commented Apr 18, 2021 •

edited

Loading

areusch left a comment •

edited

Loading

echuraev commented Sep 8, 2021 •

edited

Loading