Shave a few instructions from child-process startup sequence
Just a small patch; hopefully useful.
Best regards,
--
Gurjeet Singh
EnterpriseDB Inc.
Attachments:
shave_a_few_instructions_in_child_startup.patchtext/x-diff; charset=US-ASCII; name=shave_a_few_instructions_in_child_startup.patchDownload
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index ccb8b86..48dc7af 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -2236,6 +2236,8 @@ ClosePostmasterPorts(bool am_syslogger)
StreamClose(ListenSocket[i]);
ListenSocket[i] = PGINVALID_SOCKET;
}
+ else
+ break;
}
/* If using syslogger, close the read side of the pipe */
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Oct 31, 2013 at 11:20 PM, 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?
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
EnterpriseDB Inc.
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Mon, Nov 4, 2013 at 12:20 AM, Tom Lane <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.
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@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.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Nov 20, 2013 at 10:21 AM, Peter Eisentraut <peter_e@gmx.net> wrote:
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.
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>
Attachments:
shave_a_few_instructions_in_child_startup.v2.patchtext/x-diff; charset=US-ASCII; name=shave_a_few_instructions_in_child_startup.v2.patchDownload
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index ccb8b86..9efc9fa 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -2236,6 +2236,20 @@ ClosePostmasterPorts(bool am_syslogger)
StreamClose(ListenSocket[i]);
ListenSocket[i] = PGINVALID_SOCKET;
}
+ else
+ {
+ /*
+ * Do not process the rest of the array elements since we expect
+ * the presence of an invalid socket id to mark the end of valid
+ * elements.
+ */
+#ifdef USE_ASSERT_CHECKING
+ int j;
+ for(j = i; j < MAXLISTEN; j++)
+ Assert(ListenSocket[i] == PGINVALID_SOCKET);
+#endif
+ break;
+ }
}
/* If using syslogger, close the read side of the pipe */
src/backend/postmaster/postmaster.c:2255: indent with spaces.
+ else
src/backend/postmaster/postmaster.c:2267: indent with spaces.
+ break;
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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.
Best regards,
--
Gurjeet Singh http://gurjeet.singh.im/
EnterprsieDB Inc. www.enterprisedb.com
Attachments:
shave_a_few_instructions_in_child_startup.v3.patchtext/x-diff; charset=US-ASCII; name=shave_a_few_instructions_in_child_startup.v3.patchDownload
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index ccb8b86..cdae6e5 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -2236,6 +2236,20 @@ ClosePostmasterPorts(bool am_syslogger)
StreamClose(ListenSocket[i]);
ListenSocket[i] = PGINVALID_SOCKET;
}
+ else
+ {
+ /*
+ * Do not process the rest of the array elements since we expect
+ * the presence of an invalid socket id to mark the end of valid
+ * elements.
+ */
+#ifdef USE_ASSERT_CHECKING
+ int j;
+ for(j = i; j < MAXLISTEN; j++)
+ Assert(ListenSocket[i] == PGINVALID_SOCKET);
+#endif
+ break;
+ }
}
/* If using syslogger, close the read side of the pipe */
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.
+ breakNot 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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.
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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. +
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers