Thread: repeat() function, CHECK_FOR_INTERRUPTS(), and unlikely()
I was doing some memory testing under fractional CPU allocations and it became
painfully obvious that the repeat() function needs CHECK_FOR_INTERRUPTS().
I exchanged a few emails offlist with Tom about it, and (at the risk of putting
words in his mouth) he agreed and felt it was a candidate for backpatching.
Very small patch attached. Quick and dirty performance test:
explain analyze SELECT repeat('A', 300000000);
explain analyze SELECT repeat('A', 300000000);
explain analyze SELECT repeat('A', 300000000);
With an -O2 optimized build:
Without CHECK_FOR_INTERRUPTS
Planning Time: 1077.238 ms
Execution Time: 0.016 ms
Planning Time: 1080.381 ms
Execution Time: 0.013 ms
Planning Time: 1072.049 ms
Execution Time: 0.013 ms
With CHECK_FOR_INTERRUPTS
Planning Time: 1078.703 ms
Execution Time: 0.013 ms
Planning Time: 1077.495 ms
Execution Time: 0.013 ms
Planning Time: 1076.793 ms
Execution Time: 0.013 ms
While discussing the above, Tom also wondered whether we should add unlikely()
to the CHECK_FOR_INTERRUPTS() macro.
Small patch for that also attached. I was not sure about the WIN32 stanza on
that (to do it or not; if so, what about the UNBLOCKED_SIGNAL_QUEUE() test).
I tested as above with unlikely() and did not see any discernible difference,
but the added check might improve other code paths.
Comments or objections?
Thanks,
Joe
--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development
Attachment
On 5/12/20 8:06 AM, Joe Conway wrote: > I was doing some memory testing under fractional CPU allocations and it became > painfully obvious that the repeat() function needs CHECK_FOR_INTERRUPTS(). > > I exchanged a few emails offlist with Tom about it, and (at the risk of putting > words in his mouth) he agreed and felt it was a candidate for backpatching. > > Very small patch attached. Quick and dirty performance test: <snip> > While discussing the above, Tom also wondered whether we should add unlikely() > to the CHECK_FOR_INTERRUPTS() macro. > > Small patch for that also attached. I was not sure about the WIN32 stanza on > that (to do it or not; if so, what about the UNBLOCKED_SIGNAL_QUEUE() test). > > I tested as above with unlikely() and did not see any discernible difference, > but the added check might improve other code paths. > > Comments or objections? Seeing none ... I intend to backpatch and push these two patches in the next day or so. Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development
Attachment
Joe Conway <mail@joeconway.com> writes:
>> Comments or objections?
> Seeing none ... I intend to backpatch and push these two patches in the next day
> or so.
There was some question as to what (if anything) to do with the Windows
version of CHECK_FOR_INTERRUPTS. Have you resolved that?
regards, tom lane
On 5/25/20 9:52 AM, Tom Lane wrote:
> Joe Conway <mail@joeconway.com> writes:
>>> Comments or objections?
>
>> Seeing none ... I intend to backpatch and push these two patches in the next day
>> or so.
>
> There was some question as to what (if anything) to do with the Windows
> version of CHECK_FOR_INTERRUPTS. Have you resolved that?
Relevant hunk:
*************** do { \
*** 107,113 ****
do { \
if (UNBLOCKED_SIGNAL_QUEUE()) \
pgwin32_dispatch_queued_signals(); \
! if (InterruptPending) \
ProcessInterrupts(); \
} while(0)
#endif /* WIN32 */
--- 107,113 ----
do { \
if (UNBLOCKED_SIGNAL_QUEUE()) \
pgwin32_dispatch_queued_signals(); \
! if (unlikely(InterruptPending)) \
ProcessInterrupts(); \
} while(0)
#endif /* WIN32 */
Two questions.
First, as I understand it, unlikely() is a gcc thing, so it does nothing at all
for MSVC builds of Windows, which presumably are the predominate ones. The
question here is whether it is worth doing at all for Windows builds. On the
other hand it seems unlikely to harm anything, so I think it is reasonable to
leave the patch as is in that respect.
The second question is whether UNBLOCKED_SIGNAL_QUEUE() warrants its own
likely() or unlikely() wrapper. I have no idea, but we could always add that
later if someone deems it worthwhile.
Joe
--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development
Attachment
Joe Conway <mail@joeconway.com> writes:
> On 5/25/20 9:52 AM, Tom Lane wrote:
>> There was some question as to what (if anything) to do with the Windows
>> version of CHECK_FOR_INTERRUPTS. Have you resolved that?
> Two questions.
> First, as I understand it, unlikely() is a gcc thing, so it does nothing at all
> for MSVC builds of Windows, which presumably are the predominate ones. The
> question here is whether it is worth doing at all for Windows builds. On the
> other hand it seems unlikely to harm anything, so I think it is reasonable to
> leave the patch as is in that respect.
Perhaps I'm an optimist, but I think that eventually we will figure out
how to make unlikely() work for MSVC. In the meantime we might as well
let it work for gcc-on-Windows builds.
> The second question is whether UNBLOCKED_SIGNAL_QUEUE() warrants its own
> likely() or unlikely() wrapper. I have no idea, but we could always add that
> later if someone deems it worthwhile.
I think that each of those tests should have a separate unlikely() marker,
since the whole point here is that we don't expect either of those tests
to yield true in the huge majority of CHECK_FOR_INTERRUPTS executions.
regards, tom lane
On Mon, May 25, 2020 at 11:14:39AM -0400, Tom Lane wrote: > Perhaps I'm an optimist, but I think that eventually we will figure out > how to make unlikely() work for MSVC. In the meantime we might as well > let it work for gcc-on-Windows builds. I am less optimistic than that, but there is hope. This was mentioned as something considered for implementation in April 2019: https://developercommunity.visualstudio.com/idea/488669/please-add-likelyunlikely-builtins.html > I think that each of those tests should have a separate unlikely() marker, > since the whole point here is that we don't expect either of those tests > to yield true in the huge majority of CHECK_FOR_INTERRUPTS executions. +1. I am not sure that the addition of unlikely() should be backpatched though, that's not something usually done. -- Michael
Attachment
On 5/27/20 3:29 AM, Michael Paquier wrote: >> I think that each of those tests should have a separate unlikely() marker, >> since the whole point here is that we don't expect either of those tests >> to yield true in the huge majority of CHECK_FOR_INTERRUPTS executions. > > +1. I am not sure that the addition of unlikely() should be > backpatched though, that's not something usually done. I backpatched and pushed the changes to the repeat() function. Any other opinions regarding backpatch of the unlikely() addition to CHECK_FOR_INTERRUPTS()? Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development
Attachment
On 5/28/20 1:23 PM, Joe Conway wrote: > On 5/27/20 3:29 AM, Michael Paquier wrote: >>> I think that each of those tests should have a separate unlikely() marker, >>> since the whole point here is that we don't expect either of those tests >>> to yield true in the huge majority of CHECK_FOR_INTERRUPTS executions. >> >> +1. I am not sure that the addition of unlikely() should be >> backpatched though, that's not something usually done. > > I backpatched and pushed the changes to the repeat() function. Any other > opinions regarding backpatch of the unlikely() addition to CHECK_FOR_INTERRUPTS()? So far I have Tom +1 Michael -1 me +0 on backpatching the addition of unlikely() to CHECK_FOR_INTERRUPTS(). Assuming no one else chimes in I will push the attached to all supported branches sometime before Tom creates the REL_13_STABLE branch on Sunday. Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development
Attachment
On 2020-May-28, Joe Conway wrote: > I backpatched and pushed the changes to the repeat() function. Any other > opinions regarding backpatch of the unlikely() addition to CHECK_FOR_INTERRUPTS()? We don't use unlikely() in 9.6 at all, so I would stop that backpatching at 10 anyhow. (We did backpatch unlikely()'s definition afterwards.) -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 6/4/20 5:20 PM, Alvaro Herrera wrote: > On 2020-May-28, Joe Conway wrote: > >> I backpatched and pushed the changes to the repeat() function. Any other >> opinions regarding backpatch of the unlikely() addition to CHECK_FOR_INTERRUPTS()? > > We don't use unlikely() in 9.6 at all, so I would stop that backpatching > at 10 anyhow. (We did backpatch unlikely()'s definition afterwards.) Correct you are -- thanks for the heads up! Pushed to REL_10_STABLE and later. Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development