Re: Shave a few instructions from child-process startup sequence

Just a small patch; hopefully useful.

Best regards,
--
Gurjeet Singh

http://gurjeet.singh.im/

EnterpriseDB Inc.

On Thu, Oct 31, 2013 at 2:41 AM, Gurjeet Singh <gurjeet(at)singh(dot)im> wrote:
> Just a small patch; hopefully useful.

This is valid saving as we are filling array ListenSocket[] in
StreamServerPort() serially, so during ClosePostmasterPorts() once if
it encountered PGINVALID_SOCKET, it is valid to break the loop.
Although savings are small considering this doesn't occur in any
performance path, still I think this is right thing to do in code.

It is better to register this patch in CF app list, unless someone
feels this is not right.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> writes:
> On Thu, Oct 31, 2013 at 2:41 AM, Gurjeet Singh <gurjeet(at)singh(dot)im> wrote:
>> Just a small patch; hopefully useful.

> This is valid saving as we are filling array ListenSocket[] in
> StreamServerPort() serially, so during ClosePostmasterPorts() once if
> it encountered PGINVALID_SOCKET, it is valid to break the loop.
> Although savings are small considering this doesn't occur in any
> performance path, still I think this is right thing to do in code.

> It is better to register this patch in CF app list, unless someone
> feels this is not right.

I think this is adding fragility for absolutely no meaningful savings.
The existing code does not depend on the assumption that the array
is filled consecutively and no entries are closed early. Why should
we add such an assumption here?

regards, tom lane

On Fri, Nov 1, 2013 at 12:20 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> writes:
>> On Thu, Oct 31, 2013 at 2:41 AM, Gurjeet Singh <gurjeet(at)singh(dot)im> wrote:
>>> Just a small patch; hopefully useful.
>
>> This is valid saving as we are filling array ListenSocket[] in
>> StreamServerPort() serially, so during ClosePostmasterPorts() once if
>> it encountered PGINVALID_SOCKET, it is valid to break the loop.
>> Although savings are small considering this doesn't occur in any
>> performance path, still I think this is right thing to do in code.
>
>> It is better to register this patch in CF app list, unless someone
>> feels this is not right.
>
> I think this is adding fragility for absolutely no meaningful savings.
> The existing code does not depend on the assumption that the array
> is filled consecutively and no entries are closed early. Why should
> we add such an assumption here?

+1.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Thu, Oct 31, 2013 at 11:20 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> writes:
> > On Thu, Oct 31, 2013 at 2:41 AM, Gurjeet Singh <gurjeet(at)singh(dot)im> wrote:
> >> Just a small patch; hopefully useful.
>
> > This is valid saving as we are filling array ListenSocket[] in
> > StreamServerPort() serially, so during ClosePostmasterPorts() once if
> > it encountered PGINVALID_SOCKET, it is valid to break the loop.
> > Although savings are small considering this doesn't occur in any
> > performance path, still I think this is right thing to do in code.
>
> > It is better to register this patch in CF app list, unless someone
> > feels this is not right.
>
> I think this is adding fragility for absolutely no meaningful savings.
> The existing code does not depend on the assumption that the array
> is filled consecutively and no entries are closed early. Why should
> we add such an assumption here?
>

IMHO, typical configurations have one, or maybe two of these array elements
populated; one for TCP 5432 (for localhost or *), and the other for Unix
Domain Sockets. Saving 62 iterations and comparisons in startup sequence
may not be much, but IMHO it does match logic elsewhere.

In fact, this was inspired by the following code in ServerLoop():

ServerLoop()
{
...
/*
* New connection pending on any of our sockets? If so, fork a child
* process to deal with it.
*/
if (selres > 0)
{
int i;

for (i = 0; i < MAXLISTEN; i++)
{
if (ListenSocket[i] == PGINVALID_SOCKET)
break;
if (FD_ISSET(ListenSocket[i], &rmask))
{

And looking for other precedences, I found that initMasks() function also
relies on the array being filled consecutively and the first
PGINVALID_SOCKET marks end of valid array members.

Best regards,
--
Gurjeet Singh

http://gurjeet.singh.im/

EnterpriseDB Inc.

On Fri, Nov 1, 2013 at 9:50 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> writes:
>> On Thu, Oct 31, 2013 at 2:41 AM, Gurjeet Singh <gurjeet(at)singh(dot)im> wrote:
>>> Just a small patch; hopefully useful.
>
>> This is valid saving as we are filling array ListenSocket[] in
>> StreamServerPort() serially, so during ClosePostmasterPorts() once if
>> it encountered PGINVALID_SOCKET, it is valid to break the loop.
>> Although savings are small considering this doesn't occur in any
>> performance path, still I think this is right thing to do in code.
>
>> It is better to register this patch in CF app list, unless someone
>> feels this is not right.
>
> I think this is adding fragility for absolutely no meaningful savings.
> The existing code does not depend on the assumption that the array
> is filled consecutively and no entries are closed early.

As I could see, it appears to me that code in ServerLoop and
initMasks is already dependent on it, if any socket is closed out of
order, it can break the logic in these API's. Do me and Gurjeet are
missing some point here?

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> writes:
> On Fri, Nov 1, 2013 at 9:50 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> I think this is adding fragility for absolutely no meaningful savings.
>> The existing code does not depend on the assumption that the array
>> is filled consecutively and no entries are closed early.

> As I could see, it appears to me that code in ServerLoop and
> initMasks is already dependent on it, if any socket is closed out of
> order, it can break the logic in these API's. Do me and Gurjeet are
> missing some point here?

It's not hard to foresee that we might have to fix those assumptions
someday. If we were buying a lot by adding a similar assumption here,
it might be worth doing even in the face of having to revert it later.
But we're not buying much. A few instructions during postmaster shutdown
is entirely negligible.

regards, tom lane

On Mon, Nov 4, 2013 at 12:20 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> But we're not buying much. A few instructions during postmaster shutdown
> is entirely negligible.
>

The patch is for ClosePostmasterPorts(), which is called from every child
process startup sequence (as $subject also implies), not in postmaster
shutdown. I hope that adds some weight to the argument.

Best regards,
--
Gurjeet Singh gurjeet.singh.im
EnterpriseDB Inc. www.enterprisedb.com

On 11/5/13, 2:47 AM, Gurjeet Singh wrote:
> On Mon, Nov 4, 2013 at 12:20 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us
> <mailto:tgl(at)sss(dot)pgh(dot)pa(dot)us>> wrote:
>
> But we're not buying much. A few instructions during postmaster
> shutdown
> is entirely negligible.
>
>
> The patch is for ClosePostmasterPorts(), which is called from every
> child process startup sequence (as $subject also implies), not in
> postmaster shutdown. I hope that adds some weight to the argument.

If there is a concern about future maintenance, you could add assertions
(in appropriate compile mode) that the rest of the array is indeed
PGINVALID_SOCKET. I think that could be a win for both performance and
maintainability.

On Wed, Nov 20, 2013 at 10:21 AM, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:

> On 11/5/13, 2:47 AM, Gurjeet Singh wrote:
> > On Mon, Nov 4, 2013 at 12:20 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us
> > <mailto:tgl(at)sss(dot)pgh(dot)pa(dot)us>> wrote:
> >
> > But we're not buying much. A few instructions during postmaster
> > shutdown
> > is entirely negligible.
> >
> >
> > The patch is for ClosePostmasterPorts(), which is called from every
> > child process startup sequence (as $subject also implies), not in
> > postmaster shutdown. I hope that adds some weight to the argument.
>
> If there is a concern about future maintenance, you could add assertions
> (in appropriate compile mode) that the rest of the array is indeed
> PGINVALID_SOCKET. I think that could be a win for both performance and
> maintainability.
>

Makes sense! Does the attached patch look like what you expected? I also
added a comment to explain the expectation.

Thanks and best regards,
--
Gurjeet Singh http://gurjeet.singh.im/

EDB Inc. www.EnterpriseDB.com <http://www.enterprisedb.com>

src/backend/postmaster/postmaster.c:2255: indent with spaces.
+ else
src/backend/postmaster/postmaster.c:2267: indent with spaces.
+ break;

On Tue, Nov 26, 2013 at 9:42 PM, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:

> src/backend/postmaster/postmaster.c:2255: indent with spaces.
> + else
> src/backend/postmaster/postmaster.c:2267: indent with spaces.
> + break

Not sure how that happened! Attached is the updated patch.

Best regards,
--
Gurjeet Singh http://gurjeet.singh.im/

EnterprsieDB Inc. www.enterprisedb.com

On Tue, Nov 26, 2013 at 10:12 PM, Gurjeet Singh <gurjeet(at)singh(dot)im> wrote:
> On Tue, Nov 26, 2013 at 9:42 PM, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
>>
>> src/backend/postmaster/postmaster.c:2255: indent with spaces.
>> + else
>> src/backend/postmaster/postmaster.c:2267: indent with spaces.
>> + break
>
>
> Not sure how that happened! Attached is the updated patch.

This is a performance patch, so it should come with benchmark results
demonstrating that it accomplishes its intended purpose. I don't see
any.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Wed, Nov 27, 2013 at 9:12 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

>
> This is a performance patch, so it should come with benchmark results
> demonstrating that it accomplishes its intended purpose. I don't see
> any.
>

Yes, this is a performance patch, but as the subject says, it saves a few
instructions. I don't know how to write a test case that can measure
savings of skipping a few instructions in a startup sequence that
potentially takes thousands, or even millions, of instructions.

Best regards,
--
Gurjeet Singh http://gurjeet.singh.im/

EDB Corp. www.EnterpriseDB.com <http://www.enterprisedb.com>

Peter Eisentraut escribió:
> src/backend/postmaster/postmaster.c:2255: indent with spaces.
> + else
> src/backend/postmaster/postmaster.c:2267: indent with spaces.
> + break;

I just checked the Jenkins page for this patch:
http://pgci.eisentraut.org/jenkins/job/postgresql_commitfest_world/243/
just to make sure I can figure out what it means. You reported it as
"build unstable" in the commitfest entry:
https://commitfest.postgresql.org/action/patch_view?id=1277
However, looking at Jenkins, I couldn't figure out what the problem is.
I can go to the "GNU make + GCC warnings" page, which lists one
warning -- but we already know it:
unused variable ‘yyg’ [-Wunused-variable]

I can go to the "console output" page, which has this:

01:24:53 + tar cJf postgresql-9.4.bin.tar.xz postgresql-9.4.bin/
01:24:53 [WARNINGS] Parsing warnings in console log with parser GNU Make + GNU Compiler (gcc)
01:24:53 [WARNINGS] Computing warning deltas based on reference build #242
01:24:53 [WARNINGS] Ignore new warnings since this is the first valid build
01:24:53 Archiving artifacts
01:24:53 WARN: No artifacts found that match the file pattern "**/regression.diffs,**/regression.out,cpluspluscheck.out". Configuration error?
01:24:53 WARN: ‘**/regression.diffs’ doesn’t match anything: ‘**’ exists but not ‘**/regression.diffs’
01:24:53 Checking console output
01:24:53 /var/lib/jenkins/jobs/postgresql_commitfest_world/builds/2013-11-25_01-14-27/log:
01:24:53 5644dbce38ce0f5f16155eba9988fee1 -
01:24:53 Build step 'Jenkins Text Finder' changed build result to UNSTABLE

But I can hardly blame the patch for the above, can I?

I'm not saying I like the patch; I just wonder how to make Jenkins work
for me effectively. Is this a configuration error? Should I be looking
at some other page?

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 1/20/14, 8:08 PM, Alvaro Herrera wrote:
> Peter Eisentraut escribió:
>> src/backend/postmaster/postmaster.c:2255: indent with spaces.
>> + else
>> src/backend/postmaster/postmaster.c:2267: indent with spaces.
>> + break;
>
> I just checked the Jenkins page for this patch:
> http://pgci.eisentraut.org/jenkins/job/postgresql_commitfest_world/243/
> just to make sure I can figure out what it means. You reported it as
> "build unstable" in the commitfest entry:
> https://commitfest.postgresql.org/action/patch_view?id=1277
> However, looking at Jenkins, I couldn't figure out what the problem is.

In this case, it was the whitespace violation. (Yeah, I'm constantly
debating with myself whether it's worth reporting that, but at the
moment I'm still on the side of the fence that wants to make people
submit clean patches.)

In general, it's sometimes a bit hard to find out what caused the build
to fail. Jenkins can detect and report that for standard tools (e.g.,
compiler warnings, JUnit test results), but not for our custom test
tools. Another issue is that the build is running with make -k, so the
issue could be somewhere in the middle of the build log. I'm exploring
new plugins to improve that, as it's a significant problem.

On Sun, Dec 1, 2013 at 12:07:21PM -0500, Gurjeet Singh wrote:
> On Wed, Nov 27, 2013 at 9:12 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
>
> This is a performance patch, so it should come with benchmark results
> demonstrating that it accomplishes its intended purpose. I don't see
> any.
>
>
> Yes, this is a performance patch, but as the subject says, it saves a few
> instructions. I don't know how to write a test case that can measure savings of
> skipping a few instructions in a startup sequence that potentially takes
> thousands, or even millions, of instructions.

Are we saying we don't want this patch?

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

Bruce Momjian <bruce(at)momjian(dot)us> writes:
> On Sun, Dec 1, 2013 at 12:07:21PM -0500, Gurjeet Singh wrote:
>> Yes, this is a performance patch, but as the subject says, it saves a few
>> instructions. I don't know how to write a test case that can measure savings of
>> skipping a few instructions in a startup sequence that potentially takes
>> thousands, or even millions, of instructions.

> Are we saying we don't want this patch?

I don't --- I think it makes the code less robust for no gain worthy
of the name.

regards, tom lane