https://github.com/labath commented:

That's one way to fix the problem. Another (maybe even more obvious) would be 
to use a single SyncService object but synchronize all access to it. I think 
your approach makes sense -- it unlocks more parallelism, though that 
parallelism may be a mirage if the device connection is going to be the 
bottleneck. And all the extra connections may add some overhead. Have you 
looked into how the two approaches compare?



As for the patch itself, the code makes sense, but the result looks very 
path-dependent (meaning: you'd never write code like this -- creating a 
temporary AdbClient object -- if you were writing this from scratch). It's also 
not optimal, as you're creating a temporary AdbClient in 
PlatformAndroid::GetSyncService and then creating *another* temporary object 
inside AdbClient::GetSyncService.

I think this would look better if PlatformAndroid::GetSyncService constructed a 
(new) SyncService directly. And the SyncService could create a temporary 
AdbClient object as an implementation detail (or not -- maybe it could handle 
all of the connection setup internally)

https://github.com/llvm/llvm-project/pull/145382
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to