Re: [HACKERS] Shave a few instructions from child-process startup sequence
Bruce Momjian br...@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
Re: [HACKERS] Shave a few instructions from child-process startup sequence
On Sun, Dec 1, 2013 at 12:07:21PM -0500, Gurjeet Singh wrote: On Wed, Nov 27, 2013 at 9:12 AM, Robert Haas robertmh...@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 br...@momjian.ushttp://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
Re: [HACKERS] Shave a few instructions from child-process startup sequence
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
Re: [HACKERS] Shave a few instructions from child-process startup sequence
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 Herrerahttp://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
Re: [HACKERS] Shave a few instructions from child-process startup sequence
On Wed, Nov 27, 2013 at 9:12 AM, Robert Haas robertmh...@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
Re: [HACKERS] Shave a few instructions from child-process startup sequence
On Tue, Nov 26, 2013 at 10:12 PM, Gurjeet Singh gurj...@singh.im wrote: On Tue, Nov 26, 2013 at 9:42 PM, Peter Eisentraut pete...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Shave a few instructions from child-process startup sequence
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
Re: [HACKERS] Shave a few instructions from child-process startup sequence
On Tue, Nov 26, 2013 at 9:42 PM, Peter Eisentraut pete...@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
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 */
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Shave a few instructions from child-process startup sequence
On Wed, Nov 20, 2013 at 10:21 AM, Peter Eisentraut pete...@gmx.net wrote:
On 11/5/13, 2:47 AM, Gurjeet Singh wrote:
On Mon, Nov 4, 2013 at 12:20 AM, Tom Lane t...@sss.pgh.pa.us
mailto:t...@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
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 */
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Shave a few instructions from child-process startup sequence
On 11/5/13, 2:47 AM, Gurjeet Singh wrote: On Mon, Nov 4, 2013 at 12:20 AM, Tom Lane t...@sss.pgh.pa.us mailto:t...@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
Re: [HACKERS] Shave a few instructions from child-process startup sequence
On Mon, Nov 4, 2013 at 12:20 AM, Tom Lane t...@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
Re: [HACKERS] Shave a few instructions from child-process startup sequence
On Fri, Nov 1, 2013 at 9:50 AM, Tom Lane t...@sss.pgh.pa.us wrote: Amit Kapila amit.kapil...@gmail.com writes: On Thu, Oct 31, 2013 at 2:41 AM, Gurjeet Singh gurj...@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
Re: [HACKERS] Shave a few instructions from child-process startup sequence
Amit Kapila amit.kapil...@gmail.com writes: On Fri, Nov 1, 2013 at 9:50 AM, Tom Lane t...@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
Re: [HACKERS] Shave a few instructions from child-process startup sequence
On Thu, Oct 31, 2013 at 11:20 PM, Tom Lane t...@sss.pgh.pa.us wrote:
Amit Kapila amit.kapil...@gmail.com writes:
On Thu, Oct 31, 2013 at 2:41 AM, Gurjeet Singh gurj...@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
http://gurjeet.singh.im/
EnterpriseDB Inc.
Re: [HACKERS] Shave a few instructions from child-process startup sequence
On Thu, Oct 31, 2013 at 2:41 AM, Gurjeet Singh gurj...@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
Re: [HACKERS] Shave a few instructions from child-process startup sequence
Amit Kapila amit.kapil...@gmail.com writes: On Thu, Oct 31, 2013 at 2:41 AM, Gurjeet Singh gurj...@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
Re: [HACKERS] Shave a few instructions from child-process startup sequence
On Fri, Nov 1, 2013 at 12:20 AM, Tom Lane t...@sss.pgh.pa.us wrote: Amit Kapila amit.kapil...@gmail.com writes: On Thu, Oct 31, 2013 at 2:41 AM, Gurjeet Singh gurj...@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
