Hi,
Currently there is no way to fetch a big message/attachment through
ext/imap if memory_limit is enabled. I'd like to kindly ask for
consideration of the following patch:
http://dev.iworks.at/PATCHES/imap_savebody.patch.txt
The patch introduces a new function
imap_savebody(rsrc imap, string file, int msgno, string sect[, int flags])
which will save the specified body section to a file.
Thanks,
Michael - < mike(@)php.net
#define GETS_FETCH_SIZE 2048000
That size doesn't seem a bit excessive to you? I thought the whole point of
dumping it to a file was to avoid huge memory chunks. Not to mention the
fact that reserving 2MB of stack space makes me a little twitchy...
/* {{{ proto bool imap_savebody(resource stream_id,
string file, int msg_no, string section[, int options])
I'd make section option with the default being whatever it is that just
gives you the whole body. Come to that it probably makes sense to be able
to save the entire message (including headers).
I'd also suggest either a second function, or an overloaded second parameter
so that an existing stream could be used. This way a scripter could shuttle
multiple messages off to a spool file, or open one for appending and add a
message, or dump it to network socket (maybe they're implementing their own
POP3=>IMAP gateway (god only knows why).
-Sara
Sara Golemon wrote:
#define GETS_FETCH_SIZE 2048000
That size doesn't seem a bit excessive to you? I thought the whole point of
dumping it to a file was to avoid huge memory chunks. Not to mention the
fact that reserving 2MB of stack space makes me a little twitchy...
Indeed, 2 megs of stack space seems a bit much, not to mention
completely bypassed PHP's memory limit. I'd recommend allocating 1
megabyte buffer via emalloc(), a bit slower, but much "nicer" IMHO.
Ilia
Use 8192 chunk size; anything bigger than that is a complete waste of memory.
--Wez.
Sara Golemon wrote:
#define GETS_FETCH_SIZE 2048000
That size doesn't seem a bit excessive to you? I thought the whole point of
dumping it to a file was to avoid huge memory chunks. Not to mention the
fact that reserving 2MB of stack space makes me a little twitchy...Indeed, 2 megs of stack space seems a bit much, not to mention
completely bypassed PHP's memory limit. I'd recommend allocating 1
megabyte buffer via emalloc(), a bit slower, but much "nicer" IMHO.Ilia
Yep, I agree with that, and some past tests of mine showed that
depending on the system, anything bigger than about 8-64K, will give
marginal performance increase (and possibly decrease depending on
memory allocation); so it's really not worth the hassle. It's also a
size which we can allocate on the stack.
Andi
At 09:52 AM 9/22/2005, Wez Furlong wrote:
Use 8192 chunk size; anything bigger than that is a complete waste of memory.
--Wez.
Sara Golemon wrote:
#define GETS_FETCH_SIZE 2048000
That size doesn't seem a bit excessive to you? I thought the
whole point of
dumping it to a file was to avoid huge memory chunks. Not to mention the
fact that reserving 2MB of stack space makes me a little twitchy...Indeed, 2 megs of stack space seems a bit much, not to mention
completely bypassed PHP's memory limit. I'd recommend allocating 1
megabyte buffer via emalloc(), a bit slower, but much "nicer" IMHO.Ilia
I wrote:
Currently there is no way to fetch a big message/attachment through
ext/imap if memory_limit is enabled. I'd like to kindly ask for
consideration of the following patch:
Thanks a lot for review; the patch has been updated with your suggestions.
Additionally the whole thing is ifdef'd out in ZTS mode as the c-client
gets function is a global variable. I tested the function with a 17M
message saving to a file as well to an already opened stream.
Thanks,
Michael - < mike(@)php.net
Thanks a lot for review; the patch has been updated with your suggestions.
Additionally the whole thing is ifdef'd out in ZTS mode as the c-client
gets function is a global variable. I tested the function with a 17M
message saving to a file as well to an already opened stream.
Getting there, but you can make it work for ZTS mode. Just create a proxy
for the gets function which is always set, then have the proxy do a
TSRMLS_FETCH() and examine an extension global (e.g. IMAPG(gets_mode) or
whatever) to determine whether it's in savebody() and should shuttle to your
new gets function, or if it's not in savebody() and should dispatch to the
"original" method.
Come to that, might as well just make IMAPG(real_gets) as a dynamic
function.... Not that I've looked at ext/imap enough to know what the role
of gets is precisely.
-Sara
Hi Sara Golemon, you wrote:
Getting there, but you can make it work for ZTS mode. Just create a proxy
for the gets function which is always set, then have the proxy do a
TSRMLS_FETCH() and examine an extension global (e.g. IMAPG(gets_mode) or
whatever) to determine whether it's in savebody() and should shuttle to your
new gets function, or if it's not in savebody() and should dispatch to the
"original" method.
Thanks for your input, Sara. The new patch can be found at:
http://dev.iworks.at/PATCHES/imap_savebody_TS.patch.txt
Regards,
Michael - < mike(@)php.net
Looks basicly alright (with a slight disagreement on how the read loop is
implemented (below). In general you've got my vote (for what it's worth
given my lack of experience with libc-client.
unsigned long i, rest = size % GETS_FETCH_SIZE;
char *buf = emalloc(GETS_FETCH_SIZE+1);
for (i = GETS_FETCH_SIZE; i < size; i += GETS_FETCH_SIZE) {
memset(buf, 0, GETS_FETCH_SIZE+1);
f(stream, GETS_FETCH_SIZE, buf);
php_stream_write_string(IMAPG(gets_stream), buf);
}
if (rest) {
memset(buf, 0, GETS_FETCH_SIZE+1);
f(stream, rest, buf);
php_stream_write_string(IMAPG(gets_stream), buf);
}
efree(buf);
return NULL;
Here's how I'd write this block, it assumed that f returns the number of
bytes actually read, but according to the proto for readfn_t it should do
that. Feel free to ignore:
char buf[GETS_FETCH_SIZE];
unsigned long ret = 1;
while (ret > 0) {
ret = f(stream, GETS_FETCH_SIZE, buf);
php_stream_write_stringl(IMAPG(gets_stream), buf, ret);
}
return NULL;
Hi Sara Golemon, you wrote:
Here's how I'd write this block, it assumed that f returns the number of
bytes actually read, but according to the proto for readfn_t it should
do that.
Well, unfortunately not. It seems that net_get_buffer() in mail.c is
used as standard readfn_t which only returns NIL/T i.e. 0/1.
But the patch's implementation is by no means mandatory, it's just
about getting the data out with enabled memory_limit, so I actually
wouldn't mind if it would get implemented in a totally different way ;)
Regards,
Michael - < mike(@)php.net
I'd recommend adopting Sara's suggestion; those memset()s you have
there shouldn't be needed:
char buf[GETS_FETCH_SIZE];
while (ret > 0) {
if (!f(stream, sizeof(buf), buf))
break;
php_stream_write_string(IMAPG(gets_stream), buf);
}
return NULL;
--Wez.
Hi Sara Golemon, you wrote:
Here's how I'd write this block, it assumed that f returns the number of
bytes actually read, but according to the proto for readfn_t it should
do that.Well, unfortunately not. It seems that net_get_buffer() in mail.c is
used as standard readfn_t which only returns NIL/T i.e. 0/1.But the patch's implementation is by no means mandatory, it's just
about getting the data out with enabled memory_limit, so I actually
wouldn't mind if it would get implemented in a totally different way ;)Regards,
Michael - < mike(@)php.net
Well, unfortunately not. It seems that net_get_buffer() in mail.c is
used as standard readfn_t which only returns NIL/T i.e. 0/1.
Okay, take two:
char buf[GETS_FETCH_SIZE];
while (size > 0) {
in toread = (size > GETS_FETCH_SIZE) ? GETS_FETCH_SIZE : size);
if (!f(stream, toread, buf)) break;
php_stream_write_stringl(IMAPG(gets_stream), buf, toread);
size -= toread;
}
return NULL;
Ok, here we go:
http://dev.iworks.at/PATCHES/imap_savebody_TS.patch.txt
Regards,
Michael - < mike(@)php.net