Re: [PATCH] x11 terminal respects the requested aspect ratio

[Dima K. <gn...@di...>]

Hi!

> On Sat, 29 Sep 2012 11:44:09 -0700
> "sfeam (Ethan Merritt)" <eam...@gm...> wrote:
>
> On Saturday, 29 September 2012, Dima Kogan wrote:
> > > On Fri, 28 Sep 2012 10:37:06 -0700
> 
> Hi Dima,
> 
> Just some preliminary thoughts...
> I quickly replicated your benchmarks and see roughly the same results
> (faster CPU but limited memory; 32bit environment).
> 
> However, I have a few general concerns.
> 
> 1) The output of the ascii version is hugely redundant. Most of the
> successive V commands are identical because the vectors are shorter
> than the resolution of the plot coordinates.  If this were a common
> thing we would do well to add a filtering step in x11.trm so that
> a new "V" command is only sent if it differs from the previous command.
> For example running the ascii output through "uniq" reduces the file
> size by a factor of 7, with a speed increase of 50x(!!) running 
> through gnuplot_x11.

Sounds great. Should I do this, or do you want to?


> 2) The binary output is not terminating the "V" records with a '\n'.
> This is fine on linux, but there is a long history of problem reports
> on Windows arising from very long buffers sent through a pipe.  I don't
> know the details well enough to say whether this would trigger similar
> problems, but I do worry.  I think it's worth also trying a variant
> that writes a trailing '\n'. If writing a newline for every V command 
> causes a significant slowdown then perhaps we could generalize the
> existing code in X11_filled_polygon() that breaks long binary buffers
> into smaller chunks.

The binary-V implementation I had wouldn't touch the trailing '\n', since a
particular binary field size is assumed. Furthermore, the binary numbers could
have '\n' bytes in them, making a trailing '\n' even less meaningful. Out of
curiosity, I changed to code to output a '\n' after each binary V record. There
was a slight, but noticeable speed penalty. If we go down the binary-data path,
I'd prefer to not to add the '\n' simply because if anything ever looks at that
byte, then something had already gone wrong.


> 3) I worry that the binary code might not work on all supported
> platforms.  Since it's just a few lines of code, I suggest that rather
> than replacing the existing 'V' command we add a parallel command
> 'B' for the bainry version.  In x11.trm the choice between using 'V'
> or 'B' could be a compile-time option.   We can default to the binary
> version, but anyone having problems with it could revert to the old
> ascii version with a configuration flag.

My preference would be to switch entirely so that the amount of code being
maintained doesn't grow, but you're the boss. :)



> Oh, and I noticed something else about the new aspect-ratio code while
> running the benchmarks.  I use
>    ./gnuplot_x11 -noevents < foo.x11
> to benchmark the outboard driver.  Since there is no feedback in place,
> the inboard driver doesn't know about the aspect ratio and the plot
> is not scaled properly to the plot window.  Now this is not the usual
> path for display x11 output, but I wonder if we can fix that easily.
> Maybe disabling the rescaling code if the feedback pipe is not present?
> Or maybe sending an initial set of scaling commands that are always
> correct, which are later over-ridden as needed in the interactive case
> but remain in effect for the case of -noevents or no feedback pipe?

I'll take a look into this.

Keeping track of all the patches, proposals, the following are currently being
considered:

1. Patch for variable-width, space-separated fields to allow 5-digit values to
   'just work'; sent out to this list a few days ago. There was a concern that
   the extra spaces carry with them a performance cost. The benchmarks I sent
   out indicate that the performance cost is negligible at worst. I think this
   patch should be merged

2. Missing legend label with tall windows. Caused by the 'if (x < 10000 && y <
   10000)' test in x11.trm. Removing this test entirely seems to work for the
   most part. The only issue I've seen is that if the window is too tall, the
   label can overflow past the plot edge. However, the existing test is a very
   poor way to check for that condition anyway; and if an overflow is detected
   then both the legend label AND the legend symbol should be removed. So I
   argue the current test is not useful, and should be removed entirely.

3. Dead code such as the 'else if (*buffer == X11_GR_FILLED_POLYGON)' block. I'd
   like to push for these to be removed. These make it more difficult for new
   people to look at the codebase, and make maintenance more difficult than it
   should be.

4. strtolstrtol() instead of scanf() sounds like an easy win. Can you think of
   any reason to NOT make that change?

5. Adding default window size to the inboard x11 driver so that the 'terminal
   xlib' produces plots that have decent defaults when sent to the outboard
   driver manually. I'll do this.

6. Removing duplicate messages (such as duplicate consecutive V commands) sounds
   great. We should do it

7. General thoughts about speeding up the inboard -> outboard link by making
   some things binary. I like binary. Making this switch will probably make some
   things break at first, but it'll likely be worth it. If we're touching that
   at all, more radical methods may be better. For instance, instead of a V
   command for each point, we could have a V command that predeclares a long
   stream of points. So for instance you'd have "V100\n" to indicate that 100
   binary tuples follow. This would reduce the overhead even more, but would
   require read_input() to be changed to work with long streams of binary data.
   I prefer a method like this much more than what was in the last patch. If
   you'd consider merging something like this, tell me and I'll write it.

dima