Re: CPS/throttle patches

[Richard Fuchs <dfx@gmx.at>]

On Tue, Jun 11, 2002 at 03:08:53PM +1000, Peter Zelezny wrote:
[...]
> Just a few notes:
> 
> 1) Could you use xchat coding style? Like
>   if () 
>   {
>   }
>   i.e. line up the "{" and  "}" :)

Sure, no problem.

> 2) gettimeofday() isn't available on win32. We could say, to hell with win32!
>    But there's an easy fix:
> 	GTimeVal timev;
> 	g_get_current_time (&timev);
>    GTimeVal has the same fields as struct timeval;

No problem either. I would have used that anyway if i knew about it.

> 3) fe_timeout_add (250, dcc_calc_cps_timer, 0);
>    Isn't it a bit too intensive for an irc client to be going through
>    a list of calculations every 250ms? The DCC GUI is only updated every
>    2 seconds anyway (1 second in 1.9.x).

Yes, I'm not quite satisfied with that myself. The thing is, the calculation
of the CPS sum is necessary to turn on or off the global CPS throttle, and
doing this only every 1 or 2 seconds just isn't enough (especially if you're
sitting on a fat pipe which could send a lot of data in this time). I'll try
to think of a better solution for this problem.

> 4) Did you try using time(0) instead of gettimeofday(), or was that just
>    too coarse?

I did that before, it was too coarse. See below why.

> 5) What happens if we receive an Exception (the other end disconnects) while
>    the iotag is removed?
> 
> +		if (dcc->throttled) {
> +			fe_input_remove(dcc->iotag);
> +			dcc->iotag = 0;
> +			return FALSE;
> +		}

Ah, very good point. I didn't think of that. But if the socket is readable,
and we don't read() the data, wouldn't that flood the select() loop?

> It seems to me that the cps calculations could be done alot simpler, without
> needing an array of struct timevals. For example, every second, you record the
> file position:
> 
> time	pos	gain
> 0	0	0
> 1	4096	4096
> 2	8192	4096
> 3	10000	1808
> 4	12000	2000
> 5	17000	5000
> 6	20000	3000
> 
> cps = (4096 + 4096 + 1808 + 2000 + 5000 + 3000) / 6
> cps = 3333
> sounds about right I think?
> Of course, you only keep a record of the last X (10 or 20?) "gains".
> Then, if the xfer stalls, you should notice:
> 
> time	pos	gain
> 7	21000	1000
> 8	21000	0
> 9	21000	0
> 10	21000	0
> 
> cps = (1000 + 0 + 0 + 0) / 4
> cps = 250 (and eventually zero, once the "sliding window" moves beyond
>            the gain of 1000).
> 
> This way, you only need to keep an array of CPS_NUM_SAMPLES ints:
> 
> 	int gains[CPS_NUM_SAMPLES];/* bytes received in the last few seconds */
> 	int index_into_gains;
> 
> Ok, now pick holes in my theory :)

I'll try. ;)  Actually, I did try exactly what you describe before I
rewrote the whole thing to the way it is now. As it turned out, the problem
is that the file position doesn't increase gradually, but in steps of the
sender's (or our) block size. So, in fact, the table of time/pos/gain could
look like this:

time pos   gain
0    0     0
1    512   512
2    1024  512
3    2048  1024
4    2560  512
5    3584  1024

And so on. The end result is that the reported CPS is always a multiple of
the block size (or a fraction of it, depending on the number of samples you
have), and this just isn't right. It looks especially disturbing with low
CPS rates. So, the only solution I could come up with is to record the
precise time when the file position changed, and advance the position in the
array by one when a second since the last position passed. And this is what
happens now.

By the way, it doesn't make much difference whether you record the file
positions, or the gains in the positions for each sample. Actually I find it
easier to record the positions, so you can just do ((lastpos - firstpos) /
timepassed). Otherwise you'd have to go through the whole array and
calculate the sum of the gains, and that's just more CPU wasted. ;)

As I said, this patch is work in progress, it's far from finished yet. But
I'm glad about some feedback. :)

Richard
--
XChat-discuss: mailing list for XChat users
Archive:       http://mail.nl.linux.org/xchat-discuss/