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

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

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

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

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

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

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