Attached is a changeset that replaces the callback functions with a background command queue. (Actually, this is very similar in behavior, using the do_process_events() poll loop as a starting point.) The feature it adds is passing command results back to the GUI in the background. It uses QMutex and QWaitCondition to make sure the Octave worker thread is in a wait state before handling data on the GUI side of the thread divide. octave_value_list does no copy/delete in the signal/slot queuing mechanism across threads.
For this version of the changeset I've left in a number of fprintf()s to give a person an idea of what is happening, and to coincide with the results to follow. I've also used USE_CALLBACKS to ifdef out code rather than removing everything. That will make the changeset diff file a little smaller and easier to follow. I will create a new changeset with callbacks removed in a hour or two.
Here are comments from working with this:
1) Consider renaming libinterp/interp-core/oct-obj.h to libinterp/interp-core/ov-list.h or something similar. It took a long time for me to find where octave_value_list is declared.
2) There is already something like the Octave background command. It is QTerminal::relay_command (command) which sends a command to the input stream of Octave core and is used in a couple places for changing a directory and whatnot. However, that does not allow returning data and always puts some text on the terminal output and command history list. The background command queue is kind of nice in the sense it is low priority GUI events that happen between Octave worker thread's bigger tasks when idle at the command line.
3) Right now, there is a bit of mixing and matching with the octave_link. In some cases there are calls to static functions. In some cases a pointer to an instance of the object is used. Perhaps just one method is preferred down the road.
4) I put some fprintf commands at key locations (not include in this patch...to verbose and slow):
- octave_value copy constructor
- wherever there is a octave_value refcount decrement
- right before emitting signal across threads
- right after emitting signal across threads
First, there are an absolute ton of copies and destructs of octave_values for any sort of activity, even trivial things like "return" at the command line. If atomic references means more sophisticated copy, slowness will be an issue.
Second, here is a sample in which a breakpoint is set so that Octave core does a "dbstop" via eval_string() (I typed AAAA so that I new where the response began for the command):
Note all the activity going on in the core. And note that there appears to be no octave_value copy happening between before and after emitting (i.e., queueing) the signal transferring data across the thread. (That's the 5th and 6th last line above.) I take it that means that using octave_value_list is better than using octave_value.
To verify this, I changed the emitted signal so that it has an unused octave_value as one of its arguments like this:
and for a similar "dbstop" command get the following output:
Notice that there is an octave-value-copy associated with the emitted (queued) signal and an extra destructor call associated with everything finished when including an octave_value in the list of arguments. The above is the dangerous scenario because rep->count can be modified in an intermixed way between threads. Hence, octave_value_list is the preferred data format.
Lastly, here's the situation when a routine in the GUI thread actually uses the data from the octave_value_list signaled from the Octave worker thread. (I had to add more specific fprintf count++ and --count to tally things.) This is the dbstatus command when attempting to restore breakpoints:
Notice all that new activity due to the manipulations of octave_value instances in the GUI thread with the command results. I count 28 --count but only 20 count++. Maybe I missed some, but the point is that the GUI thread can work on all that data because the Octave worker thread is asleep. If things are programmed properly, the rep-count changes should be a zero sum and things properly cleaned up by time the GUI is done with computations.
6) With this setup it doesn't seem like atomic reference counting is necessary. The
is getting a little cumbersome though. Perhaps there is a better way to configure reference counting than having to include <config.h> in derivative header files.
6) I'm not completely happy with the setup for file_editor and file_editor_tab pointers and container classes. The signal-to-signal connections lose the original sender of the signal. (Could easily add that to list, but prefer to hold off if it isn't necessary.) Things are OK now, maybe global _octave_link is better, but not sure. The critical issue is that right now a file_editor_tab is created, then a ->load_file() is attempted and if that passes then connections are made. However, it is the load process where an inquiry for dbstatus() should be done, but that requires a connection which isn't yet set. I just didn't want to mess with that right now and cause problems that are sort of tangent to the code here. The octave_qt_link is a proper and good mechanism, it's just that this needs cleanup for better sequence of construction--consequence of not being modular and object oriented enough at beginning...which is OK.
7) dbstatus() is the only routine right now using the data coming from the background queue. The ones that don't need data really don't need the sleep mechanism, but I think it is better to always use that, just for consistency.
8) I think with the setup of having two core slots on both sides of the thread for the outer level GUI objects to attached to is pretty safe. If folks aren't comfortable with the computations going on in the GUI thread, it wouldn't be too difficult to interpret the commands in the background command queue (e.g., "dbstatus") and then emit signals accordingly. However, I sort of like things the way they are. It requires some knowledge on part of the programmer, but it stops the growing list of callbacks that aren't as rigorously defined as the core data objects are. It also makes for adding new features and behavior rather easy by just compiling GUI code and not having to compile core code. That is, this is for that class of GUI-initiated, non-workspace modifying events. (Command line generated is a different issue for a different time.)
(file #27902)
|