[Lldb-commits] [PATCH] D11066: Introduce a MainLoop class and switch llgs to use it

[Pavel Labath]

labath marked 3 inline comments as done.
labath added a comment.

Thank you for the comments. My (rather lengthy) replies are below.

In http://reviews.llvm.org/D11066#201959, @clayborg wrote:

> Since there are going to require multiple patches to make this work it might be best to do this in a branch. I worry about being able to handle everything in a single threaded way and have it be faster. I worry about deadlocks and other things that might not be anticipated. Did you profile this to make sure the threading is a huge overhead? What exact parts are holding up things?


It is going to be a series of patches, but it's not going to be that big, since I have already done a big thread cleanup inside NativeProcessLinux. llgs now consists of three threads
a) network communication thread
b) inferior monitoring thread
c) stdio forwarding thread
This commit makes (a) use the mainloop abstraction. I expect I will need two more to port over (b) and (c), and then maybe some small cleanup commits. I think it will be easier to review this if I keep the work on the main branch, and I believe it will work without introducing regressions.
( Just to make things clear, I do not intend to do anything with the client threads here. The IOHandler thing i suggest below is merely an abstraction switch, but all the thread interactions remain the same. I do feel that the client has too many threads, but that is a completely different topic. :) )

I already have a butchered version of llgs, which runs on a single thread. I have noticed no deadlocks and my benchmarks indicate that the round-trip time for the qThreadStopInfo packet has gone down from 7.8 to 5.4 ms (these numbers include network latency). For comparison switching to jThreadsInfo did not speed things up at all, it merely added up the individual round-trip (but I have some ideas on how to improve that as well). The threading is an overhead for us since we have to perform all debug operations on a designated thread. This involves a lot of back-and-forth when we are responding to packets since our debug operations are quite small-grained (e.g. "read this register"). Now we could alleviate this by making the operations more coarse-grained ("read all GPRs"), but I think we would hit this wall sooner or later. As I understand it, debugserver is also single-threaded for performance reasons, right?

This is why I think it would be better to get rid of the thread ping-pong first, and then profiling the remaining bottle-necks (of which there are many, I am sure) will become easier.

> What are the signals supposed to be for? Just handling "SIGHUP" and "SIGINT"? If so, you need to make sure no one does any real work inside any of these functions because it isn't safe to do 95% of any function calls in a signal handler function.


Currently, the signal handler just sets a flag. This flag will be checked in the Run() function and the appropriate callback invoked if it is set. NativeProcessLinux will install a callback for SIGCHLD and will retrieve all the info it needs when it gets called.

> If the MainLoop is handling a packet that is deadlocked somehow, you will never be able to consume the event you just created for SIGHUP and you won't be able to cancel/kill/disconnect...


SIGHUP is not my main motivation for this, but I have ported it while I was at it. SIGHUP is used as a request for graceful termination, so checking for the flag after packet handling completes is a good choice I think (and this is what the current implementation is doing). If the process is locked up, then we are unlikely to be able to terminate gracefully, so sending a signal which will kill us outright (SIGTERM or any other unhandled signal) is as good solution as any.

In http://reviews.llvm.org/D11066#202473, @amccarth wrote:

> But that doesn't rule out this MainLoop abstraction.  A simple debug loop in a separate thread could translate raw Windows debugging events into events that could be detected by MainLoop.  So if MainLoop helps on other platforms, the Windows implementation could be made to work with this model, probably without a significant difference in performance or code complexity.


I don't think the current implementation of ProcessWindows needs to worry about the MainLoop (see the comment about IOHandler on why i am involving you). However, if windows ever moves to the remote debugging model then we may need to do something like you suggest.


================
Comment at: include/lldb/Host/posix/MainLoopPosix.h:81
@@ +80,3 @@
+    // signal handler. However, since the callback is not invoked synchronously, you cannot use
+    // this mechanism to handle SIGSEGV and the like.
+    SignalHandleUP
----------------
amccarth wrote:
> What sorts of signals might this mechanism be used for?
> 
> Be aware that Windows defines fewer signals than POSIX.
The main use for me now is the receipt of SIGCHLD (which signals that there is something interesting happening in the inferior). On linux we can use this to multiplex inferior events and network communication on a single thread.

I know that on windows, signals aren't really a thing and if we don't need them there, MainLoopWindows does not even have to declare this method. On the other hand, I can imagine implementing a MLW::RegisterWindowsSpecificHandle, which would then multiplex socket I/O with some other events using whatever method is right for that on windows. Or maybe it does not need additional methods and everything that we need to wait for can be represented with IOObjects.

The main value of this abstraction I see for windows would be for things like IOHandlerProcessSTDIO, which is disabled for windows, because there you cannot select() over non-socket file descriptors. I imagine the code there can be replaced by something like
```
void IOHandlerProcessSTDIO::Run() {
  MainLoop::ReadHandleUP = m_mainloop.RegisterReadObject(m_read_file,
    [] { do_whatever_you_need_to_do_when_data_is_available(); } );
  m_mainloop.Run();
}
void IOHandlerProcessSTDIO::Cancel() {
  m_mainloop.RequestTermination();
}
```
(for this we would need to make RequestTermination be safe to execute from another thread, which I haven't done yet, since I don't need it, but it can be easily added)

================
Comment at: include/lldb/Host/windows/MainLoopWindows.h:21
@@ +20,3 @@
+
+class MainLoopWindows
+{
----------------
amccarth wrote:
> Is there a particular reason you didn't defined a common interface in a base class that both MainLoopWindows and MainLoopPosix would derive from?  Is it concern over the performance of virtual function calls?  Or are you expecting different platforms to provide significantly different public interfaces?
I saw the typedef pattern in LockFile.h, but I did not notice that there actually is a LockFileBase that they inherit from. I will add a Base class for this.

================
Comment at: tools/lldb-server/lldb-gdbserver.cpp:667
@@ -690,3 +666,3 @@
 
-    ConnectToRemote(gdb_server, reverse_connect,
+    ConnectToRemote(mainloop, gdb_server, reverse_connect,
                     host_and_port, progname, subcommand,
----------------
clayborg wrote:
> Why do you need to pass "mainloop" if it is already in gdb_server?
I have refactored this to avoid passing the parameter. Main loop is now run directly from the main function. (mainloop is in gdb_server, because the NativeProcessLinux will need to register additional events when it get's spawned).


http://reviews.llvm.org/D11066