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

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

> > 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'm attaching a patch that removes hardcoded field sizes from ALL %d
> > and %u fields. Some extra care had to be taken in places where a
> > numerical field is followed by a string (%s), since the '4' was
> > hardcoded in those places too. The bug you described is now gone, and
> > the code is more robust.
> > 
> > I noticed another, very related issue. x11.trm has
> > 
> >     /* badly outrange labels can overflow into text field */
> >     if (x < 10000 && y < 10000) {
> > 	PRINT3("T%d %d %s\n", x, y, str);
> >     }
> > 
> > Here we're hardcoding the 10000 again. This has the effect of not
> > printing the legend label when the window is skinny (as you were making
> > it). This test needs to be adjusted or removed entirely; not sure
> > which. CVS says this test was added by lhecking in 1998. This predates
> > all of sourceforge. Newsgroup logs go back that far, but I didn't see
> > any mention of this issue. Thoughts?
> 
> That does look like an early instance of the same problem.
> But I wonder how that code ever triggered?

Is lhecking still around? Think he still remembers?

> 
> > Finally, in writing the patch, I stumbled on what looks like a bug in
> > what looks like dead code. In gplt_x11.c, look at the
> > 
> >     else if (*buffer == X11_GR_FILLED_POLYGON) {	/* filled
> > [snip]
> > I suspect the whole block is dead, and this bug is thus
> > never hit. If this whole block is truly dead, it should be removed.
> > dima
> 
> I think it must be left over from when the choice between binary/ascii
> polygon encoding was a configuration option.  Since 2009 the ascii 
> option no longer exists.  So yeah, that section is dead code.

Great. Can you remove it then?

Dima