Re: Reduce timing overhead of EXPLAIN ANALYZE using rdtsc?

Hi,

On 1/21/23 05:12, Andres Freund wrote:
> We do currently do the conversion quite frequently.  Admittedly I was
> partially motivated by trying to get the per-loop overhead in pg_test_timing
> down ;)
>
> But I think it's a real issue. Places where we do, but shouldn't, convert:
>
> - ExecReScan() - quite painful, we can end up with a lot of those
> - InstrStopNode() - adds a good bit of overhead to simple
InstrStopNode() doesn't convert in the general case but only for the 
first tuple or when async. So it goes somewhat hand in hand with 
ExecReScan().
> - PendingWalStats.wal_write_time - this is particularly bad because it happens
>    within very contended code
> - calls to pgstat_count_buffer_read_time(), pgstat_count_buffer_write_time() -
>    they can be very frequent
> - pgbench.c, as we already discussed
> - pg_stat_statements.c
> - ...
>
> These all will get a bit slower when moving to a "variable" frequency.
I wonder if we will be able to measure any of them easily. But given 
that it's many more places than I had realized and given that the 
optimized code is not too involved, let's give it a try.
> What was your approach for avoiding the costly operation?  I ended up with a
> integer multiplication + shift approximation for the floating point
> multiplication (which in turn uses the inverse of the division by the
> frequency). To allow for sufficient precision while also avoiding overflows, I
> had to make that branch conditional, with a slow path for large numbers of
> nanoseconds.

It seems like we ended up with the same. I do:

sec = ticks / frequency_hz
ns  = ticks / frequency_hz * 1,000,000,000
ns  = ticks * (1,000,000,000 / frequency_hz)
ns  = ticks * (1,000,000 / frequency_khz) <-- now in kilohertz

Now, the constant scaling factor in parentheses is typically a floating 
point number. For example for a frequency of 2.5 GHz it would be 2.5. To 
work around that we can do something like:

ns  = ticks * (1,000,000 * scaler / frequency_khz) / scaler

Where scaler is a power-of-2, big enough to maintain enough precision 
while allowing for a shift to implement the division.

The additional multiplication with scaler makes that the maximum range 
go down, because we must ensure we never overflow. I'm wondering if we 
cannot pick scaler in such a way that remaining range of cycles is large 
enough for our use case and we can therefore live without bothering for 
the overflow case. What would be "enough"? 1 year? 10 years? ...

Otherwise, we indeed need code that cares for the potential overflow. My 
hunch is that it can be done branchless, but it for sure adds dependent 
instructions. Maybe in that case a branch is better that almost 
certainly will never be taken?

I'll include the code in the new patch set which I'll latest submit 
tomorrow.

> I think it'd be great - but I'm not sure we're there yet, reliability and
> code-complexity wise.
Thanks to your commits, the diff of the new patch set will be already 
much smaller and easier to review. What's your biggest concern in terms 
of reliability?
> I think it might be worth makign the rdts aspect somewhat
> measurable. E.g. allowing pg_test_timing to use both at the same time, and
> have it compare elapsed time with both sources of counters.
I haven't yet looked into pg_test_timing. I'll do that while including 
your patches into the new patch set.

-- 
David Geier
(ServiceNow)