Re: [PATCHES] Win32: Minimising desktop heap usage
Dave Page [EMAIL PROTECTED] writes: [ get rid of wsprintf in favor of snprintf ] +1 for not depending on nonstandard subroutines without need. But please note the standard locution is snprintf(buf, sizeof(buf), ... Not sizeof() - 1. ! char*tmppath=0; Please use NULL not 0 ... actually, since the variable is unconditionally assigned to just below, I'd say this should just be char *tmppath;. I don't like useless initializations: if the compiler is not smart enough to discard them then they waste cycles, and they also increase the risk of bugs-of-omission in future changes. If someone were to change things around so that tmppath didn't get assigned to in one code path, then the compiler would complain about use of an uninitialized variable --- *unless* you've written a useless initialization such as the above, in which case the mistake might pass unnoticed for awhile. So don't initialize a local variable unless you're giving it an actually meaningful value. ! /* ! * Note: We use getenv here because the more modern SHGetSpecialFolderPath() ! * will force us to link with shell32.lib which eats valuable desktop heap. ! */ ! tmppath = getenv(APPDATA); Hmm, is there any functional downside to this? I suppose SHGetSpecialFolderPath does some things that getenv does not... Otherwise it looks good... regards, tom lane ---(end of broadcast)--- TIP 1: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
Re: [PATCHES] Win32: Minimising desktop heap usage
Dave Page [EMAIL PROTECTED] writes: Tom Lane wrote: So don't initialize a local variable unless you're giving it an actually meaningful value. The downside is that we see a useless warning that, if sufficiently multiplied, might make it hard to see something we are interested in. [ looks again... ] Actually, I think you just proved my point for me. The ZeroMemory call should go away, no? (Does this mean that the Windows builds fail to detect dereferencing NULL? Bad if so.) Hmm, is there any functional downside to this? I suppose SHGetSpecialFolderPath does some things that getenv does not... A good percentage of the special folder paths you might query with SHGetSpecialFolderPath() are not actually in the environment. APPDATA certainly is on XP, 2K3 and Vista though, and I've found MS KB articles referring to it being on 2K as well. OK, in that case seems no problem. regards, tom lane ---(end of broadcast)--- TIP 5: don't forget to increase your free space map settings
Re: [PATCHES] Win32: Minimising desktop heap usage
Tom Lane wrote:
Dave Page [EMAIL PROTECTED] writes:
[ get rid of wsprintf in favor of snprintf ]
+1 for not depending on nonstandard subroutines without need.
But please note the standard locution is snprintf(buf, sizeof(buf), ...
Not sizeof() - 1.
Noted.
!char*tmppath=0;
Please use NULL not 0 ... actually, since the variable is
unconditionally assigned to just below, I'd say this should just be
char *tmppath;. I don't like useless initializations: if the compiler
is not smart enough to discard them then they waste cycles, and they
also increase the risk of bugs-of-omission in future changes. If
someone were to change things around so that tmppath didn't get assigned
to in one code path, then the compiler would complain about use of an
uninitialized variable --- *unless* you've written a useless
initialization such as the above, in which case the mistake might pass
unnoticed for awhile. So don't initialize a local variable unless
you're giving it an actually meaningful value.
The downside is that we see a useless warning that, if sufficiently
multiplied, might make it hard to see something we are interested in.
In this case, not initialising it will also create the first 'accepted'
warning in VC++ in our code :-(. All the others come from Kerberos.
Modified in the attached patch however.
! /*
! * Note: We use getenv here because the more modern
SHGetSpecialFolderPath()
! * will force us to link with shell32.lib which eats valuable desktop
heap.
! */
! tmppath = getenv(APPDATA);
Hmm, is there any functional downside to this? I suppose
SHGetSpecialFolderPath does some things that getenv does not...
A good percentage of the special folder paths you might query with
SHGetSpecialFolderPath() are not actually in the environment. APPDATA
certainly is on XP, 2K3 and Vista though, and I've found MS KB articles
referring to it being on 2K as well.
Regards, Dave.
Index: src/backend/port/win32/signal.c
===
RCS file: /projects/cvsroot/pgsql/src/backend/port/win32/signal.c,v
retrieving revision 1.18
diff -c -r1.18 signal.c
*** src/backend/port/win32/signal.c 5 Jan 2007 22:19:35 - 1.18
--- src/backend/port/win32/signal.c 23 Oct 2007 14:44:35 -
***
*** 178,184
char pipename[128];
HANDLE pipe;
! wsprintf(pipename, .\\pipe\\pgsignal_%d, (int) pid);
pipe = CreateNamedPipe(pipename, PIPE_ACCESS_DUPLEX,
PIPE_TYPE_MESSAGE | PIPE_READMODE_MESSAGE | PIPE_WAIT,
--- 178,184
char pipename[128];
HANDLE pipe;
! snprintf(pipename, sizeof(pipename), .\\pipe\\pgsignal_%u, (int) pid);
pipe = CreateNamedPipe(pipename, PIPE_ACCESS_DUPLEX,
PIPE_TYPE_MESSAGE | PIPE_READMODE_MESSAGE | PIPE_WAIT,
***
*** 251,257
char pipename[128];
HANDLE pipe = pgwin32_initial_signal_pipe;
! wsprintf(pipename, .\\pipe\\pgsignal_%d, GetCurrentProcessId());
for (;;)
{
--- 251,257
char pipename[128];
HANDLE pipe = pgwin32_initial_signal_pipe;
! snprintf(pipename, sizeof(pipename), .\\pipe\\pgsignal_%u, GetCurrentProcessId());
for (;;)
{
Index: src/port/kill.c
===
RCS file: /projects/cvsroot/pgsql/src/port/kill.c,v
retrieving revision 1.8
diff -c -r1.8 kill.c
*** src/port/kill.c 5 Jan 2007 22:20:02 - 1.8
--- src/port/kill.c 23 Oct 2007 14:44:35 -
***
*** 38,44
errno = EINVAL;
return -1;
}
! wsprintf(pipename, .\\pipe\\pgsignal_%i, pid);
if (!CallNamedPipe(pipename, sigData, 1, sigRet, 1, bytes, 1000))
{
if (GetLastError() == ERROR_FILE_NOT_FOUND)
--- 38,44
errno = EINVAL;
return -1;
}
! snprintf(pipename, sizeof(pipename), .\\pipe\\pgsignal_%u, pid);
if (!CallNamedPipe(pipename, sigData, 1, sigRet, 1, bytes, 1000))
{
if (GetLastError() == ERROR_FILE_NOT_FOUND)
Index: src/port/path.c
===
RCS file: /projects/cvsroot/pgsql/src/port/path.c,v
retrieving revision 1.71
diff -c -r1.71 path.c
*** src/port/path.c 5 Jan 2007 22:20:02 - 1.71
--- src/port/path.c 23 Oct 2007 14:47:29 -
***
*** 628,637
strlcpy(ret_path, pwd-pw_dir, MAXPGPATH);
return true;
#else
! char tmppath[MAX_PATH];
ZeroMemory(tmppath, sizeof(tmppath));
! if (SHGetFolderPath(NULL, CSIDL_APPDATA, NULL, 0, tmppath) != S_OK)
return false;
snprintf(ret_path, MAXPGPATH, %s/postgresql, tmppath);
return true;
--- 628,643
strlcpy(ret_path, pwd-pw_dir, MAXPGPATH);
return true;
#else
! char *tmppath;
ZeroMemory(tmppath, sizeof(tmppath));
!
! /*
! * Note: We use getenv here because the more modern SHGetSpecialFolderPath()
! * will force us to link with shell32.lib which eats valuable desktop heap.
! */
!
Re: [PATCHES] Win32: Minimising desktop heap usage
Tom Lane [EMAIL PROTECTED] writes: ! /* ! * Note: We use getenv here because the more modern SHGetSpecialFolderPath() ! * will force us to link with shell32.lib which eats valuable desktop heap. ! */ ! tmppath = getenv(APPDATA); Hmm, is there any functional downside to this? I suppose SHGetSpecialFolderPath does some things that getenv does not... The functional difference appears to be that the environment variable is set on startup (or login?) and doesn't necessarily have the most up to date value if it's been changed. But it's not something you're likely to change and you can always adjust the environment variable manually to fix the problem. -- Gregory Stark EnterpriseDB http://www.enterprisedb.com ---(end of broadcast)--- TIP 4: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] Win32: Minimising desktop heap usage
Tom Lane wrote:
[ looks again... ] Actually, I think you just proved my point for me.
The ZeroMemory call should go away, no?
Yup, quite correct. v3 attached.
/D
Index: src/backend/port/win32/signal.c
===
RCS file: /projects/cvsroot/pgsql/src/backend/port/win32/signal.c,v
retrieving revision 1.18
diff -c -r1.18 signal.c
*** src/backend/port/win32/signal.c 5 Jan 2007 22:19:35 - 1.18
--- src/backend/port/win32/signal.c 23 Oct 2007 16:02:41 -
***
*** 178,184
char pipename[128];
HANDLE pipe;
! wsprintf(pipename, .\\pipe\\pgsignal_%d, (int) pid);
pipe = CreateNamedPipe(pipename, PIPE_ACCESS_DUPLEX,
PIPE_TYPE_MESSAGE | PIPE_READMODE_MESSAGE | PIPE_WAIT,
--- 178,184
char pipename[128];
HANDLE pipe;
! snprintf(pipename, sizeof(pipename), .\\pipe\\pgsignal_%u, (int) pid);
pipe = CreateNamedPipe(pipename, PIPE_ACCESS_DUPLEX,
PIPE_TYPE_MESSAGE | PIPE_READMODE_MESSAGE | PIPE_WAIT,
***
*** 251,257
char pipename[128];
HANDLE pipe = pgwin32_initial_signal_pipe;
! wsprintf(pipename, .\\pipe\\pgsignal_%d, GetCurrentProcessId());
for (;;)
{
--- 251,257
char pipename[128];
HANDLE pipe = pgwin32_initial_signal_pipe;
! snprintf(pipename, sizeof(pipename), .\\pipe\\pgsignal_%u, GetCurrentProcessId());
for (;;)
{
Index: src/port/kill.c
===
RCS file: /projects/cvsroot/pgsql/src/port/kill.c,v
retrieving revision 1.8
diff -c -r1.8 kill.c
*** src/port/kill.c 5 Jan 2007 22:20:02 - 1.8
--- src/port/kill.c 23 Oct 2007 16:02:41 -
***
*** 38,44
errno = EINVAL;
return -1;
}
! wsprintf(pipename, .\\pipe\\pgsignal_%i, pid);
if (!CallNamedPipe(pipename, sigData, 1, sigRet, 1, bytes, 1000))
{
if (GetLastError() == ERROR_FILE_NOT_FOUND)
--- 38,44
errno = EINVAL;
return -1;
}
! snprintf(pipename, sizeof(pipename), .\\pipe\\pgsignal_%u, pid);
if (!CallNamedPipe(pipename, sigData, 1, sigRet, 1, bytes, 1000))
{
if (GetLastError() == ERROR_FILE_NOT_FOUND)
Index: src/port/path.c
===
RCS file: /projects/cvsroot/pgsql/src/port/path.c,v
retrieving revision 1.71
diff -c -r1.71 path.c
*** src/port/path.c 5 Jan 2007 22:20:02 - 1.71
--- src/port/path.c 23 Oct 2007 16:04:42 -
***
*** 628,637
strlcpy(ret_path, pwd-pw_dir, MAXPGPATH);
return true;
#else
! char tmppath[MAX_PATH];
! ZeroMemory(tmppath, sizeof(tmppath));
! if (SHGetFolderPath(NULL, CSIDL_APPDATA, NULL, 0, tmppath) != S_OK)
return false;
snprintf(ret_path, MAXPGPATH, %s/postgresql, tmppath);
return true;
--- 628,641
strlcpy(ret_path, pwd-pw_dir, MAXPGPATH);
return true;
#else
! char *tmppath;
! /*
! * Note: We use getenv here because the more modern SHGetSpecialFolderPath()
! * will force us to link with shell32.lib which eats valuable desktop heap.
! */
! tmppath = getenv(APPDATA);
! if (!tmppath)
return false;
snprintf(ret_path, MAXPGPATH, %s/postgresql, tmppath);
return true;
---(end of broadcast)---
TIP 1: if posting/reading through Usenet, please send an appropriate
subscribe-nomail command to [EMAIL PROTECTED] so that your
message can get through to the mailing list cleanly
Re: [PATCHES] Win32: Minimising desktop heap usage
Gregory Stark wrote: Tom Lane [EMAIL PROTECTED] writes: ! /* ! * Note: We use getenv here because the more modern SHGetSpecialFolderPath() ! * will force us to link with shell32.lib which eats valuable desktop heap. ! */ ! tmppath = getenv(APPDATA); Hmm, is there any functional downside to this? I suppose SHGetSpecialFolderPath does some things that getenv does not... The functional difference appears to be that the environment variable is set on startup (or login?) and doesn't necessarily have the most up to date value if it's been changed. But it's not something you're likely to change and you can always adjust the environment variable manually to fix the problem. If you're hacking the registry to change it then you've only got yourself to blame if you don't update the environment as well imho. I also wouldn't be at all surprised if many apps build paths containing the value returned by SHGetSpecialFolderPath() (or %APPDATA%) at startup and then use that value from then on rather than repeatedly calling the API function. It's not like you'd expect the value to change at all often, if ever. /D ---(end of broadcast)--- TIP 7: You can help support the PostgreSQL project by donating at http://www.postgresql.org/about/donate
Re: [PATCHES] Win32: Minimising desktop heap usage
Tom Lane wrote: Dave Page [EMAIL PROTECTED] writes: Tom Lane wrote: So don't initialize a local variable unless you're giving it an actually meaningful value. The downside is that we see a useless warning that, if sufficiently multiplied, might make it hard to see something we are interested in. [ looks again... ] Actually, I think you just proved my point for me. The ZeroMemory call should go away, no? (Does this mean that the Windows builds fail to detect dereferencing NULL? Bad if so.) Windows builds don't fail to detect that in genereal. ZeroMemory, however, has a protection specifically against being passed NULL input, IIRC. //Magnus ---(end of broadcast)--- TIP 1: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
Re: [PATCHES] Win32: Minimising desktop heap usage
Dave Page wrote: Tom Lane wrote: [ looks again... ] Actually, I think you just proved my point for me. The ZeroMemory call should go away, no? Yup, quite correct. v3 attached. Great job tracking this down! Patch looks good from here (after the fixes per Tom). Applied, many thanks! //Magnus ---(end of broadcast)--- TIP 6: explain analyze is your friend
