Hello Alvaro,
> I'm not really sure about the fact that we operate on those Stats
> structs without locking. I see upthread you convinced Michael that it
> was okay, but is it really? How severe is the damage if two threads
> happen to collide?
For stats shared among threads, when it occurs one data about one
transaction is not counted.
On the risk side: the collision probability is pretty low because the time
to update a value is a "few" cycles, and the time to execute a transaction
is typically in ms: I think under 1/10,000,000 data could be lost.
On the advantageous side: locking costs significant time thus would impact
performance, I think that the measured performance loss because the
occasional transaction data is not counted is lower that the performance
loss due to systematically locking.
So for me this is really a low risk trade-off.
> [...]
> It seems a bit funny to have the start_time not be reset when 0.0 is
> passed, which is almost all the callers. Using a float as a boolean
> looks pretty odd; is that kosher? Maybe it'd be a good idea to have a
> separate boolean flag instead? Something like this
>
> /*
> * Initialize a StatsData struct to all zeroes. Use the given
> * start_time only if reset_start_time, otherwise keep the original
> * value.
> */
> static void
> initStats(StatsData *sd, double start_time, bool reset_start_time)
> {
> sd->cnt = 0;
> sd->skipped = 0;
> initSimpleStats(&sd->latency);
> initSimpleStats(&sd->lag);
>
> /* not necessarily overriden? */
> if (reset_start_time)
> sd->start_time = start_time;
> }
Obviously this would work. I did not think the special case was worth the
extra argument. This one has some oddity too, because the second argument
is ignored depending on the third. Do as you feel.
> I renamed a couple of your functionettes, for instance doSimpleStats to
> addToSimpleStats and appendSimpleStats to mergeSimpleStats.
Fine with me.
--
Fabien.
