Hi, On 2023-01-02 14:28:20 +0100, David Geier wrote: > I also somewhat improved the accuracy of the cycles to milli- and > microseconds conversion functions by having two more multipliers with higher > precision. For microseconds we could also keep the computation integer-only. > I'm wondering what to best do for seconds and milliseconds. I'm currently > leaning towards just keeping it as is, because the durations measured and > converted are usually long enough that precision shouldn't be a problem.
I'm doubtful this is worth the complexity it incurs. By the time we convert out of the instr_time format, the times shouldn't be small enough that the accuracy is affected much. Looking around, most of the existing uses of INSTR_TIME_GET_MICROSEC() actually accumulate themselves, and should instead keep things in the instr_time format and convert later. We'd win more accuracy / speed that way. I don't think the introduction of pg_time_usec_t was a great idea, but oh well. > Additionally, I initialized a few variables of type instr_time which > otherwise resulted in warnings due to use of potentially uninitialized > variables. Unless we decide, as I suggested downthread, that we deprecate INSTR_TIME_SET_ZERO(), that's unfortunately not the right fix. I've a similar patch that adds all the necesarry INSTR_TIME_SET_ZERO() calls. > What about renaming INSTR_TIME_GET_DOUBLE() to INSTR_TIME_GET_SECS() so that > it's consistent with the _MILLISEC() and _MICROSEC() variants? > The INSTR_TIME_GET_MICROSEC() returns a uint64 while the other variants > return double. This seems error prone. What about renaming the function or > also have the function return a double and cast where necessary at the call > site? I think those should be a separate discussion / patch. Greetings, Andres Freund
