patchGNU Octave - Patches: patch #8016, Prototype octave_value_list...

 
 

patch #8016: Prototype octave_value_list transfer

Submitted by:  Dan Sebald <sebald>
Submitted on:  Sat 13 Apr 2013 09:57:00 AM UTC  
 
Category: NonePriority: 5 - Normal
Status: NonePrivacy: Public
Assigned to: NoneOpen/Closed: Open

Add a New Comment (Rich MarkupRich Markup):
   

You are not logged in

Please log in, so followups can be emailed to you.

 

(Jump to the original submission Jump to the original submission)

Fri 26 Apr 2013 06:02:49 PM UTC, comment #6:

Noting that I tested a similar trans-thread communication as part of the dialogs changeset here:

https://savannah.gnu.org/patch/index.php?7990

Fri 26 Apr 2013 05:57:45 PM GMT, comment #25:

I found that the communication was robust with or without checking on the GUI side that the mutex can be locked before awakening the worker thread.

Dan Sebald <sebald>
Thu 18 Apr 2013 08:23:29 AM UTC, comment #5:

Yes, discussion on maintainers list is fine.

I remember something I thought about earlier in the day so I'm going to write a note here to not forget. Once a file is opened there really isn't a need for the connection between the background command result signal and the add_all_breakpoints() slot. I just tested that

at the end of add_all_breakpoints will break the connection. I think it's better left as is though for the moment because it might help uncover some other things with the debugger when that comes up.

Dan Sebald <sebald>
Thu 18 Apr 2013 07:44:31 AM UTC, comment #4:

If you want to discuss this, how about using the maintainers list?

John W. Eaton <jwe>
Project Administrator
Thu 18 Apr 2013 07:39:53 AM UTC, comment #3:

Here's a changeset with a little more clean up and the callback code removed. Same code as previous changeset, but the fprintf statements have been removed.

(file #27903)

Dan Sebald <sebald>
Thu 18 Apr 2013 05:45:16 AM UTC, comment #2:

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)

Dan Sebald <sebald>
Mon 15 Apr 2013 03:13:02 AM UTC, comment #1:

Will try something like this at a later time:

octave_value_list result = eval_string(...,nout)
if (OK)
for i=0; i< nout; i++
{
copy_count(i) = result(i).rep.count;
result(i).rep.count = -1;
}
emit command_result (result)

put_thread_to_sleep()

//Restore the count
for i=0; i< nout; i++
{
result(i).rep.count = copy_count(i);
}

None of the copy/deletes going on in the signal/slot mechanism will have any effect on the rep-count. In the GUI thread would be:

octave-link::handle_command_result (QObject *ID, octave_value_list& result)
{
// Transmit result to other objects in GUI.
emit have_command_result (ID, result);

// No longer need this data
wake_up_the_worker_thread()
}

Dan Sebald <sebald>
Sat 13 Apr 2013 09:57:00 AM UTC, original submission:

This a prototype of prototype quality bits and pieces to transfer commands and octave_value_list data across threads. I used the qtdialogs code as a starting point, so it really isn't too much actual code. I put PROTOTYPE CONCEPT in spots to identify the needed parts.

The steps to follow are:

1) Enter some type of matrix value at the command line like x = [1:10]'*[1:10]

2) Click on the little red button near the directory file menu.

3) Enter the name of the variable created in the input dialog. (Pressing OK will queue up two commands, the size of the matrix and the matrix).

4) Enter "qterrordlg ('hello');" at the command line. (This is supposed to simulate Octave core eventually getting around to processing the commands that have been queued.)

Dan Sebald <sebald>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach File(s):
   
   
Comment:
   

Attached Files
file #27903:  octave-background_command-2013apr18_v2.patch added by sebald (36kB - application/octet-stream)
file #27902:  octave-background_command-2013apr18_v1.patch added by sebald (33kB - application/octet-stream)
file #27858:  octave-ovlist_prototype-2013apr13.patch added by sebald (29kB - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by jwe (Posted a comment)
  • -unavailable- added by sebald (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 3 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Thu 18 Apr 2013 07:39:53 AM UTCsebaldAttached File-=>Added octave-background_command-2013apr18_v2.patch, #27903
    Thu 18 Apr 2013 05:45:16 AM UTCsebaldAttached File-=>Added octave-background_command-2013apr18_v1.patch, #27902
    Sat 13 Apr 2013 09:57:00 AM UTCsebaldAttached File-=>Added octave-ovlist_prototype-2013apr13.patch, #27858

    Back to the top


    Powered by Savane 3.1-cleanup