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

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

> On Fri, 28 Sep 2012 10:37:06 -0700
> Dima Kogan <gn...@di...> wrote:
>
> > > Hi Ethan.
> > > 
> > > Good you found the issue. Do you know why field widths are
> > > specified at all? Why don't we PRINT2("V%d %d\n") and then
> > > sscanf("%d %d", ...) on the other side? No efficiency is gained
> > > by specifying the widths, it only builds in limitations and makes
> > > the code brittle, as we have just observed.
> > 
> > The basic X11 framework was in place long before my involvement, so
> > I do not know.  My best guess is that the fixed field width was
> > chosen so as to reduce the amount of data sent through the
> > gnuplot->gnuplot_x11 channel.
> > Adding a space in front of every coordinate pair would increase the
> > traffic by as much as 25%.
> > 
> > As I recall, when the polygon encoding was switched from formatted
> > to binary (2004), the advocates of binary encoding/decoding had
> > benchmarks showing data transfer really was a performance
> > bottleneck, and reducing the number of bytes sent over the channel
> > resulted in faster plotting. I do not know if this would still be
> > true on modern machines.
> > 
> > We should benchmark before and after your latest patch.
> 
> The extra space can also save bytes when the values being sent across
> have fewer than 4 digits in them. As I see it, one should use ASCII
> data links if they want robustness and readability, and binary ones
> if they want speed. Here we're sending data in ASCII, while worrying
> about a few extra cycles. If you make up a test that benchmarks
> before and after this patch, I'll write another version of the data
> passing, that uses binary data; if there're any performance gains
> here, that's where they are.


I just ran some benchmarks myself. The results are quite interesting.

First off, the test machine description:

gcc (Debian 4.7.0-12) 4.7.0

CPU:
vendor_id	: GenuineIntel
cpu family	: 6
model		: 15
model name	: Intel(R) Core(TM)2 CPU         T7400  @ 2.16GHz
stepping	: 6

This is a 2-core machine, but everything in gnuplot is single-threaded
so this doesn't matter.

I generated 2 large-ish data files. Both contain an identical sinusoid:
one stores it in ascii, another in binary with packed single-precision
floats. This isn't what we're testing, but I wanted to get this
conversion step into the data as a reference. Commands to generate data:

$ perl -e 'for(0..2000000) { print pack "f*", $_, sin($_/100000);        }' > dat.bin
$ perl -e 'for(0..2000000) { print "$_ " .        sin($_/100000) . "\n"; }' > dat.ascii


For each gnuplot build I tested, I timed 3 things:

1. inboard x11 from binary (with 'terminal xlib')
2. inboard x11 from ascii  (with 'terminal xlib')
3. outboard x11            (just gnuplot_x11 executable)

I tested 4 different gnuplot builds:

1. before the split-printf patch (uses %04d format)
2. after the split-printf patch (uses space-separated %d format)
3. after the split-printf-patch but ALSO sending the V command in
binary. V commands were the bulk of the xlib-generated data stream, so
this is our hotspot. 16-bit integers for each argument of V
4. Same as previous, but using 32-bit integers

ASCII input tests were run by making a 'tst.gp' file with

================
set term xlib
set output "out.xlib"
plot "dat.ascii" with lines
================

Then generating timings by running multiple times
$ time ./gnuplot tst.gp
$ time ./gnuplot_x11 < out.xlib > /dev/null


Binary input tests were run similarly, but with a 'tst.gp' such as

================
set term xlib
set output "out.xlib"
plot "dat.bin" binary format="%float32%float32" with lines
================


Results. Each record is usertime,systemtime in seconds.

|                     | %04d      | %d        | %d, 16-bit binary V | %d, 32-bit binary V |
|---------------------+-----------+-----------+---------------------+---------------------|
| inboard from ASCII  | 1.91,0.21 | 1.86,0.21 | 1.53,0.19           | 1.53,0.21           |
| inboard from binary | 0.95,0.20 | 0.89,0.20 | 0.56,0.16           | 0.56,0.20           |
| outboard            | 0.82,0.11 | 0.85,0.11 | 0.19,0.10           | 0.21,0.11           |

First off, we see that splitting the fields with whitespace speeds up
the inboard driver a little bit, while slowing down the outboard one a
little bit. Not completely sure why, but the difference is negligible,
so I didn't go digging.

However, sending the data over in binary produces HUGE performance
gains. The inboard driver is 37% faster (when the original input is
binary too), while the outboard one is a whopping 76% faster. Not quite
sure why this is so uneven; maybe the outboard driver parses the data
more times than it needs to?

None of this is really surprising, but it really reinforces the earlier
point that seeking performance gains in our ASCII representation is
foolish, leading to minimal speedups while making the code less
manageable. When I started this, I wasn't going to advocate that we
move to a binary data stream, but the speedup is so significant that we
really should, I think.

I'm attaching a patch that changes the V command to work in binary.
Note that this patch is not at all good-enough to merge yet; I'm
attaching it so that others could run these tests as well. So, should
we move the intensive commands to binary? What commands are these,
other than 'V'?

dima