You are viewing a plain text version of this content. The canonical link for it is here.
Posted to modperl@perl.apache.org by jo...@sunstarsys.com on 2000/08/31 19:29:24 UTC
Apache.xs patch for get_client_block
The mod_perl implementation of get_client_block has a memory leak.
The following patch should keep it from from pissing in r->pool.
diff -u /var/lib/cpan/build/mod_perl-1.24/src/modules/perl/Apache.xs /usr/src/mod_perl-1.24/src/modules/perl/Apache.xs
--- /var/lib/cpan/build/mod_perl-1.24/src/modules/perl/Apache.xs Fri Apr
21 02:13:42 2000
+++ /usr/src/mod_perl-1.24/src/modules/perl/Apache.xs Sat Jul 15 18:27:58 2000
@@ -59,7 +59,7 @@
#define CORE_PRIVATE
#include "mod_perl.h"
#include "mod_perl_xs.h"
-
+#define BUFSIZE 8192
#ifdef USE_SFIO
#undef send_fd_length
@@ -994,10 +994,11 @@
PREINIT:
long nrd = 0;
+ char buf[BUFSIZE];
PPCODE:
- buffer = (char*)palloc(r->pool, bufsiz);
- nrd = get_client_block(r, buffer, bufsiz);
+ buffer = buf;
+ nrd = get_client_block(r, buffer, (bufsiz < BUFSIZE) ? bufsiz : BUFSIZE);
if ( nrd > 0 ) {
XPUSHs(sv_2mortal(newSViv((long)nrd)));
sv_setpvn((SV*)ST(1), buffer, nrd);
Joe Schaefer
joe@sunstarsys.com
SunStar Systems, Inc.
Re: Apache.xs patch for get_client_block
Posted by jo...@sunstarsys.com.
Doug,
The get_client_block part of the patch works just fine - haven't had a chance
to confirm the rest of it.
Thanks again.
--
Joe Schaefer
joe@sunstarsys.com
SunStar Systems, Inc.
Re: Apache.xs patch for get_client_block
Posted by Doug MacEachern <do...@covalent.net>.
On 31 Aug 2000 joe@sunstarsys.com wrote:
> Btw: I've been playing with the keep-alive stuff you left lying around
> in Connection.xs. The naive implementation I made seems to work fine,
> once the headers are sent to the client. Are you planning to add them
> in the next mod_perl release?
you mean this?
# int keepalive; /* Are we using HTTP Keep-Alive? */
# int keptalive; /* Did we use HTTP Keep-Alive? */
# int keepalives; /* How many times have we used it? */
i have no plans for adding hooks to those unless they're useful in
someway. have you found a use?
Re: Apache.xs patch for get_client_block
Posted by jo...@sunstarsys.com.
Doug,
Thanks for clearing this up for me - great explanation!
I'll let you know how your patch works out.
Btw: I've been playing with the keep-alive stuff you left lying around
in Connection.xs. The naive implementation I made seems to work fine,
once the headers are sent to the client. Are you planning to add them
in the next mod_perl release?
--
Joe Schaefer
joe@sunstarsys.com
SunStar Systems, Inc.
Re: Apache.xs patch for get_client_block
Posted by Doug MacEachern <do...@covalent.net>.
On 31 Aug 2000 joe@sunstarsys.com wrote:
> Doug,
>
> Sorry to belabor a dull issue, but I'm not sure I'm getting my point across.
no problem, this is important stuff to understand.
> Most of the troubles I've run across in the mod_perl and libapreq code
> have to do with attempts to allocate memory resources for buffers
> at runtime. That's exactly what the BUFF API does, right?
not really, BUFF uses some stack allocation, it's generally up to the
caller to manage i/o buffers.
> What I'm seeing is that you are also resizing/reallocating these buffers
> each time the code is called. For instance, the original
> get_client_block had this line in it:
>
> > - buffer = (char*)palloc(r->pool, bufsiz);
>
> The one you just sent me has this line instead:
>
> > + SvGROW(buffer, bufsiz+1);
>
> I'm not sure what SvGROW does, but if it involves a malloc
> without freeing the old buffer, you've got the same problem as before.
> Each time the perl version of get_client_block is called,
> SvGROW will work it's majic on RAM, and perl won't give that RAM
> back to the OS.
but that "problem" still existed with your patch in this line:
sv_setpvn((SV*)ST(1), buffer, nrd);
same thing happens underneath (see sv.c). we can't just point Perl
variables at buffers allocated on the stack. SvGROW() will only malloc()
(or realloc) if the buffer isn't already large enough to the given length.
Perl hangs onto that allocation as an optimization, so it only has to
malloc() once for a given variable, and maybe realloc() if a later string
assignment is larger. consider this example:
sub foo {
...
my $buff;
while (my $len = $r->get_client_block($buff, 1024)) {
print "$buff\n";
}
...
}
the SvGROW() underneath will only trigger malloc() for $buff _once_, for
the entire lifetime of the process.
Perl will only release the string buffer allocation if you explicity
undef() the variable. this is true for all variables, including those
scoped with my(). clearly, you would not want to undef($buff) inside the
while loop, otherwise malloc() will be called each time through the loop.
outside the loop would not be so bad, then malloc() will only happen for
$buff each time sub foo is called. if it's called often, like for every
request, it might be better to let Perl hang onto the allocation, that's
up to you.
now, what happens when that buffer is released depends on your Perl and
perhaps os. if your Perl is built with Perl's malloc, the free()
triggered by undef() will give the memory back to Perl's pool of memory.
otherwise, free() will probably resolve to libc's free(). on some
platforms that means it is "given back to the os", on others it just means
that chunk of memory is available for reuse in that process the next time
somebody malloc's.
Re: Apache.xs patch for get_client_block
Posted by Billy Donahue <bi...@dadadada.net>.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 31 Aug 2000 joe@sunstarsys.com wrote:
> I'm not sure what SvGROW does, but if it involves a malloc
> without freeing the old buffer, you've got the same problem as before.
> Each time the perl version of get_client_block is called,
> SvGROW will work it's majic on RAM, and perl won't give that RAM
> back to the OS.
I'm ignorant enough on the rest of what you're talking about to keep
my mouth shut, but I should point out that perl can't give RAM back to
the OS. It can only give RAM back to the rest of the perl process.
If you allocate 64M in the first 100msec to initialize your process,
and then you stay alive for 2 weeks, that 64M is missing from the system
for 2 weeks.
- --
"The Funk, the whole Funk, and nothing but the Funk."
Billy Donahue <ma...@dadadada.net>
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.0.2 (GNU/Linux)
Comment: pgpenvelope 2.9.0 - http://pgpenvelope.sourceforge.net/
iD8DBQE5rs4R+2VvpwIZdF0RArPCAJ4ov5QnK7f6Hbecwy9l7+0LehgKLQCeJtNm
j5/H+FuY8WzgNXUH8qnjXEI=
=rNah
-----END PGP SIGNATURE-----
Re: Apache.xs patch for get_client_block
Posted by jo...@sunstarsys.com.
Doug,
Sorry to belabor a dull issue, but I'm not sure I'm getting my point across.
Most of the troubles I've run across in the mod_perl and libapreq code
have to do with attempts to allocate memory resources for buffers
at runtime. That's exactly what the BUFF API does, right?
What I'm seeing is that you are also resizing/reallocating these buffers
each time the code is called. For instance, the original
get_client_block had this line in it:
> - buffer = (char*)palloc(r->pool, bufsiz);
The one you just sent me has this line instead:
> + SvGROW(buffer, bufsiz+1);
I'm not sure what SvGROW does, but if it involves a malloc
without freeing the old buffer, you've got the same problem as before.
Each time the perl version of get_client_block is called,
SvGROW will work it's majic on RAM, and perl won't give that RAM
back to the OS.
Best.
--
Joe Schaefer
joe@sunstarsys.com
SunStar Systems, Inc.
Re: Apache.xs patch for get_client_block
Posted by Doug MacEachern <do...@covalent.net>.
On 31 Aug 2000 joe@sunstarsys.com wrote:
> The mod_perl implementation of get_client_block has a memory leak.
> The following patch should keep it from from pissing in r->pool.
thanks joe. i don't see how allocating from r->pool is a "leak", but
yeah, it is a waste of resources since Perl is going to make it's own
copy. read_client_block() has similar waste which i've been meaning to
fix. this patch trims allocations for both by reading directly into
Perl's buffer:
Index: src/modules/perl/Apache.xs
===================================================================
RCS file: /home/cvs/modperl/src/modules/perl/Apache.xs,v
retrieving revision 1.106
diff -u -r1.106 Apache.xs
--- src/modules/perl/Apache.xs 2000/08/31 05:49:06 1.106
+++ src/modules/perl/Apache.xs 2000/08/31 20:33:03
@@ -940,7 +940,7 @@
void
read_client_block(r, buffer, bufsiz)
Apache r
- char *buffer
+ SV *buffer
int bufsiz
PREINIT:
@@ -948,29 +948,31 @@
int rc;
PPCODE:
- buffer = (char*)safemalloc(bufsiz);
if ((rc = setup_client_block(r, REQUEST_CHUNKED_ERROR)) != OK) {
aplog_error(APLOG_MARK, APLOG_ERR | APLOG_NOERRNO, r->server,
"mod_perl: setup_client_block failed: %d", rc);
XSRETURN_UNDEF;
}
- if(should_client_block(r)) {
- nrd = get_client_block(r, buffer, bufsiz);
- r->read_length = 0;
+ if (should_client_block(r)) {
+ SvUPGRADE(buffer, SVt_PV);
+ SvGROW(buffer, bufsiz+1);
+ nrd = get_client_block(r, SvPVX(buffer), bufsiz);
+ r->read_length = 0;
}
if (nrd > 0) {
- XPUSHs(sv_2mortal(newSViv((long)nrd)));
- sv_setpvn((SV*)ST(1), buffer, nrd);
+ XPUSHs(sv_2mortal(newSViv((long)nrd)));
#ifdef PERL_STASH_POST_DATA
- table_set(r->subprocess_env, "POST_DATA", buffer);
+ table_set(r->subprocess_env, "POST_DATA", SvPVX(buffer));
#endif
- safefree(buffer);
- SvTAINTED_on((SV*)ST(1));
+ SvCUR_set(buffer, nrd);
+ *SvEND(buffer) = '\0';
+ SvPOK_on(buffer);
+ SvTAINTED_on(buffer);
}
else {
- ST(1) = &sv_undef;
+ sv_setsv(buffer, &sv_undef);
}
int
@@ -985,22 +987,25 @@
void
get_client_block(r, buffer, bufsiz)
Apache r
- char *buffer
+ SV *buffer
int bufsiz
PREINIT:
long nrd = 0;
PPCODE:
- buffer = (char*)palloc(r->pool, bufsiz);
- nrd = get_client_block(r, buffer, bufsiz);
+ SvUPGRADE(buffer, SVt_PV);
+ SvGROW(buffer, bufsiz+1);
+ nrd = get_client_block(r, SvPVX(buffer), bufsiz);
if ( nrd > 0 ) {
XPUSHs(sv_2mortal(newSViv((long)nrd)));
- sv_setpvn((SV*)ST(1), buffer, nrd);
- SvTAINTED_on((SV*)ST(1));
+ SvCUR_set(buffer, nrd);
+ *SvEND(buffer) = '\0';
+ SvPOK_on(buffer);
+ SvTAINTED_on(buffer);
}
else {
- ST(1) = &sv_undef;
+ sv_setsv(ST(1), &sv_undef);
}
int