Horton

Thread

Commits

  1. Add unlikely() to CHECK_FOR_INTERRUPTS()

  2. Add CHECK_FOR_INTERRUPTS() to the repeat() function

  1. repeat() function, CHECK_FOR_INTERRUPTS(), and unlikely()

    Joe Conway <mail@joeconway.com> — 2020-05-12T12:06:58Z

    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

  2. Re: repeat() function, CHECK_FOR_INTERRUPTS(), and unlikely()

    Joe Conway <mail@joeconway.com> — 2020-05-25T13:02:04Z

    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

  3. Re: repeat() function, CHECK_FOR_INTERRUPTS(), and unlikely()

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-05-25T13:52:13Z

    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

  4. Re: repeat() function, CHECK_FOR_INTERRUPTS(), and unlikely()

    Joe Conway <mail@joeconway.com> — 2020-05-25T14:10:43Z

    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

  5. Re: repeat() function, CHECK_FOR_INTERRUPTS(), and unlikely()

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-05-25T15:14:39Z

    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

  6. Re: repeat() function, CHECK_FOR_INTERRUPTS(), and unlikely()

    Michael Paquier <michael@paquier.xyz> — 2020-05-27T07:29:27Z

    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

  7. Re: repeat() function, CHECK_FOR_INTERRUPTS(), and unlikely()

    Joe Conway <mail@joeconway.com> — 2020-05-28T17:23:46Z

    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

  8. Re: repeat() function, CHECK_FOR_INTERRUPTS(), and unlikely()

    Joe Conway <mail@joeconway.com> — 2020-06-04T20:48:19Z

    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

  9. Re: repeat() function, CHECK_FOR_INTERRUPTS(), and unlikely()

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2020-06-04T21:20:57Z

    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

  10. Re: repeat() function, CHECK_FOR_INTERRUPTS(), and unlikely()

    Joe Conway <mail@joeconway.com> — 2020-06-05T20:53:01Z

    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