pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedFrom: David Geier <geidav.pg@gmail.com>
To: Andres Freund <andres@anarazel.de>
Cc: Robert Haas <robertmhaas@gmail.com>
Cc: vignesh C <vignesh21@gmail.com>
Cc: Lukas Fittl <lukas@fittl.com>
Cc: Michael Paquier <michael@paquier.xyz>
Cc: Ibrar Ahmed <ibrar.ahmad@gmail.com>
Cc: Maciek Sakrejda <m.sakrejda@gmail.com>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: Reduce timing overhead of EXPLAIN ANALYZE using rdtsc?
Date: Mon, 23 Jan 2023 18:49:37 +0100
Message-ID: <5a09bfe1-8a6c-de2e-d2b0-62abc5a8fe7a@gmail.com> (raw)
In-Reply-To: <20230121041200.225hljezqtpijvq4@awork3.anarazel.de>
References: <d6e84dc8-75d1-4c3b-4b32-c4fcb7852275@gmail.com>
<3eaeaa3a-b78e-ef0d-7319-5d713bbc09a0@gmail.com>
<CAP53PkwtWtY-hkSwV7E6g_n657RnFcK0asSp0foSk5Qz_CCJXQ@mail.gmail.com>
<cc72a411-aa07-834c-85c0-489a5924f8bc@gmail.com>
<CALDaNm080KHmRHo8OPcAEj+vNzXejwHgmddji5hkAdCwNsuqKA@mail.gmail.com>
<b201ac3c-1bff-2414-be8e-fc287f78be1a@gmail.com>
<20230113195547.k4nlrmawpijqwlsa@awork3.anarazel.de>
<CA+TgmoYOvh=k-H9m21Lh-SWbn7TNurm3JoOVxW+kOO=Gn1_8Xw@mail.gmail.com>
<20230117164758.gx4uuzhk5grw7zea@awork3.anarazel.de>
<50c9f291-fc60-1d2e-e286-0fb888586e7e@gmail.com>
<20230121041200.225hljezqtpijvq4@awork3.anarazel.de>
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)
view thread (172+ messages) latest in thread
Message-ID: <5a09bfe1-8a6c-de2e-d2b0-62abc5a8fe7a@gmail.com>
Permalink: ../5a09bfe1-8a6c-de2e-d2b0-62abc5a8fe7a@gmail.com/
Also on: postgresql.org/message-id/5a09bfe1-8a6c-de2e-d2b0-62abc5a8fe7a@gmail.com
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: geidav.pg@gmail.com, andres@anarazel.de, robertmhaas@gmail.com, vignesh21@gmail.com, lukas@fittl.com, michael@paquier.xyz, ibrar.ahmad@gmail.com, m.sakrejda@gmail.com
Subject: Re: Reduce timing overhead of EXPLAIN ANALYZE using rdtsc?
In-Reply-To: <5a09bfe1-8a6c-de2e-d2b0-62abc5a8fe7a@gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox