Hello Andres,
>> I've changed the performance calculations depending on -C or not. Ramp-up
>> effects are smoothed.
>
>> I've merged all time-related stuff (time_t, instr_time, int64) to use a
>> unique type (pg_time_usec_t) and set of functions/macros, which simplifies
>> the code somehow.
>
> Hm. I'm not convinced it's a good idea for pgbench to do its own thing
> here.
Given the unjustifiable heterogeneousness it induces and the simpler code
after the move, I think it is much better. Pgbench cloc is smaller after
barrier are added (4655 to 4650) thanks to that and a few other code
simplifications. Removing all INSTR_TIME_* costly macros is a relief in
itself…
>> +static int pthread_barrier_init(pthread_barrier_t *barrier, void *unused, int nthreads);
>
> How about using 'struct unknown_type *unused' instead of "unused"?
Haven't done it because I found no other instances in pg, and anyway this
code is only used once locally and NULL is passed.
>> +static pthread_barrier_t
>> + start_barrier, /* all threads are started */
>> + bench_barrier; /* benchmarking ready to start */
>
> We don't really need two barriers here.
Indeed. Down to one.
>> /* print out results */
>
> Given that we're changing the output (for the better) of pgbench again,
> I wonder if we should add the pgbench version to the benchmark
> output.
Not sure about it, but done anyway.
>> + pg_time_usec_t total_delay, /* benchmarking time */
>> + pg_time_usec_t conn_total_delay, /* is_connect */
>> + pg_time_usec_t conn_elapsed_delay, /* !is_connect */
>> + int64 latency_late)
>
> I'm not a fan of naming these 'delay'. To me that doesn't sounds like
> it's about the time the total benchmark has taken.
I have used '_duration', and tried to clarify some field and variable
names depending on what data they actually hold.
>> + printf("tps = %f (including reconnection times)\n", tps);
>> + printf("tps = %f (without initial connection establishing)\n", tps);
>
> Keeping these separate makes sense to me, they're just so wildly
> different.
Yep. I've added a comment about that.
>> +static inline pg_time_usec_t
>> +pg_time_get_usec(void)
>
> For me the function name sounds like you're getting the usec out of a
> pg_time. Not that it's getting a new timestamp.
I've used "pg_time_now()".
>> +#define PG_TIME_SET_CURRENT_LAZY(t) \
>> + if ((t) == 0) \
>> + (t) = pg_time_get_usec()
>
> I'd make it an inline function instead of this.
Done "pg_time_now_lazy(&now)"
I have also simplified the code around thread creation & join because it
was a mess: thread 0 was run in the middle of the stat collection loop…
I have updated the doc with actual current output, but excluding the
version display which would have to be changed between releases.
--
Fabien.
