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

On Thu, Oct 31, 2013 at 2:41 AM, Gurjeet Singh <gurjeet@singh.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.kapila16@gmail.com> writes:
> On Thu, Oct 31, 2013 at 2:41 AM, Gurjeet Singh <gurjeet@singh.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@sss.pgh.pa.us> wrote:
> Amit Kapila <amit.kapila16@gmail.com> writes:
>> On Thu, Oct 31, 2013 at 2:41 AM, Gurjeet Singh <gurjeet@singh.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 Fri, Nov 1, 2013 at 9:50 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Amit Kapila <amit.kapila16@gmail.com> writes:
>> On Thu, Oct 31, 2013 at 2:41 AM, Gurjeet Singh <gurjeet@singh.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.kapila16@gmail.com> writes:
> On Fri, Nov 1, 2013 at 9:50 AM, Tom Lane <tgl@sss.pgh.pa.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 11/5/13, 2:47 AM, Gurjeet Singh wrote:
> On Mon, Nov 4, 2013 at 12:20 AM, Tom Lane <tgl@sss.pgh.pa.us
> <mailto:tgl@sss.pgh.pa.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.

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 10:12 PM, Gurjeet Singh <gurjeet@singh.im> wrote:
> On Tue, Nov 26, 2013 at 9:42 PM, Peter Eisentraut <peter_e@gmx.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

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@gmail.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@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + Everyone has their own god. +

Bruce Momjian <bruce@momjian.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