On 1/16/23 18:37, Andres Freund wrote:
Hi,
On 2023-01-02 14:28:20 +0100, David Geier wrote:
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.
I don't feel strong about it and you have a point that we most likely
only convert ones we've accumulated a fair amount of cycles.
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.
Fully agreed. Why not replacing pg_time_usec_t with instr_time in a
separate patch? I haven't looked carefully enough if all occurrences
could sanely replaced but at least the ones that accumulate time seem
good starting points.
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.
I don't feel strong about it, but like Tom tend towards keeping the
initialization macro.
Thanks that you have improved on the first patch and fixed these issues
in a better way.
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.
OK. I'll propose follow-on patches ones we're done with the ones at hand.
I'll then rebase the RDTSC patches on your patch set.
--
David Geier
(ServiceNow)
