Re: Reworked qt terminal is much faster – and fails to work on Mac

[Daniel J S. <dan...@ie...>]

 From thread:

Re: enhanced_recursion()  and do_event(), can they be redesigned?

On 02/25/2014 11:39 PM, Daniel J Sebald wrote:
> On 02/24/2014 09:59 AM, Jérôme Lodewyck wrote:
[snip]
>> Concerning the code you propose: apart from the synchronization problems
>> between the Qt event loop and the main thread that you mention, I am
>> worried about the performances. As far as I can see, the communication
>> between the main thread and the thread running the Qt event loop relies
>> on the signal/slot mechanism, which I think can be significantly slower
>> that direct function calls. From rough testing, plotting a graph with a
>> million points takes about 3x more time with the code you posted.
[snip]
> So, how about as a first step this weekend I take just the QProcess
> startup code and make a nice diff/patch for you to try out and tweak how
> you see fit. We'll see if that much gets us to a functioning OSX Qt
> terminal. Simplifying that should clear things up so that if OSX Qt term
> still doesn't function we might find the source of the problem.

I wrote a clean patch which I placed here

https://sourceforge.net/p/gnuplot/patches/653/

for cleaning up the linking and waiting for gnuplot_qt socket.  This has 
no thread-related code.  It is simply what I think is improved use of 
existing Qt class member functions for waiting.  The basic summary is 
that the while loops which "poll" for input or response are removed.  To 
me, polling means whenever a loop just circulates as fast as possible 
when waiting on something.  So, while there are some while loops still 
left in the code after this patch, they are of the variety that has a 
proper Qt wait mechanism which handles all that detail for us.

As far as Mac OSX, I think there is nothing within qt_term.cpp anymore 
that is specific to OSX.

A larger summary of changes is below, but please give the patch a try 
and let me know how things work.

After this post I will write a follow-up post about threading and the 
possibility of speeding up qt_term transfer.

Dan


* I changed the QProcess to be a conventional one rather than a 
disconnected one.  That way there is still some access to the process so 
that the constructed server name can be retrieved from the process 
itself rather than having to duplicate the construction of the name in 
multiple places.  We can then use the Qt waiting mechanisms and get rid 
of the custom while loop that waits for an external server to appear.

* Note:  In qt_flushOutBuffer() the flush() is necessary because there 
is no event loop.  If there were an event loop, just socket.write() 
would be enough.

* I commented out

	// Avoid dead-locking when no more data is available
	if (qt->socket.bytesToWrite() > 0)
		qt->socket.waitForBytesWritten(-1);

because I don't think it is necessary.  I left it there just in case it 
is needed on some systems.  I doubt it, though.

* I added

     qt->socket.disconnectFromServer();

to the end of qt_atexit().  That routine waits for all pending data to 
be sent before returning and complains if one attempts to wait on 
disconnect.  So, an additional wait-on-disconnect doesn't seem necessary.

* I altered the way that qt_term waits on gnuplot_qt when it wants font 
property info.  (We are going to need to add a similar routine in order 
to get the true terminal size back from gnuplot_qt term in to fix the 
issue Ethan noted.)  I make use of the proper timeout mechanism for Qt 
rather than introducing "micro sleep" and timeouts of 0 or -1.  If one 
reviews that hunk of code they'll realize it is pretty conventional, 
i.e., keep reading bytes until enough are read to fill 
sizeof(gp_event_t).  It turns out that one read typically get all of the 
bytes necessary.

* Even after this change, I don't like the fact that qt_sendFont() 
discards events.  It is usually carriage returns from holding the Enter 
key down and an event slips in before gnuplot_qt receives 
GEFontMetricRequest.  However, I'm not sure what to do without some kind 
of queueing mechanism.

* Similar to the waiting on font info, I changed this while loop:

   while (qt->socket.bytesAvailable() >= (int)sizeof(gp_event_t)) {
   }

because, as with Yuriy Kaminskiy, that construct doesn't sit well with 
me.  Beyond seeming like it could get stuck in certain situations (maybe 
not), I don't like that it is whirling away waiting for the number of 
bytes to be sufficient.  We should be using the Qt mechanism to do any 
type of waiting rather than doing polling.

* I changed that construct for Windows as well.  It looks like almost an 
exact copy so I suspect it shouldn't break Windows Qt term.  But if it 
breaks something, we'll have to do another rev.

* The only thing I worry about (and I've left a note in the code) is 
that when waiting for responses from gnuplot_qt (such as font props) if 
there was a preceeding plot that has a huge amount of data.  If the plot 
doesn't complete in the timeout, then there could be an error.  The 
proper thing would be to dynamically adjust the wait time.  But let's 
hold off on that scheme.  I defined DEFAULT_TIMEOUT at the top of 
qt_term.cpp and we can lengthen that if need be.

* There may be a chance that Windows persist works correctly now.  I 
changed the qt_atexit() function slightly to use Qt routines

	// This Qt function waits until pending data to write is complete.
	qt->socket.disconnectFromServer();

Before that change it seemed like qt_atexit() might be wrapping things 
up too briskly, not giving enough time for proper interchange and 
disconnect.  Perhaps Windows automatically deletes the gnuplot_qt 
process without proper disconnect, or more likely the persist command 
wasn't making its way to gnuplot_qt.