Hi Gustavo, thank you for your review!
On 27 August 2013 23:17, Gustavo Lopes <glo...@nebm.ist.utl.pt> wrote:
> On 27-08-2013 14:08, Michael Wallner wrote:
>>
>> Hi,
>>
>> I prepared a patch to replace sapi_globals' request_info post_data and
>> raw_post_data with a temp stream and remove support for
>> HTTP_RAW_POST_DATA. [1]
>>
>> PROS:
>> * save up to 300% on post_data_len memory (on non-form POSTs)
>> * a local siege (c=512/512, 2.4k form/2.2k json) showed no (negative)
>> performance impact; see attached logs
>> * reusable php://input stream
>> * ...
>>
>> CONS:
>> * no more $HTTP_RAW_POST_DATA, where BC could easily provided with a
>> one-liner:
>> $GLOBALS["HTTP_RAW_POST_DATA"]=file_get_contents("php://input");
>> * memory is cheap
>> * ???
>>
>
> I think it's generally a good idea, but I have some concerns:
>
> * The whole point of PG(enable_post_data_reading) is to be able to read the
> input of the fly. If I read your patch correctly, the data is completely
> gobbled when you open php://input and then the temp file is reminded. This
> will result in a serious performance degradation on large inputs, as
> processing can only start after everything has been read. The behavior
> should be different if PG(enable_post_data_reading) is false.
Ok, *I* thought the whole point of enable_post_data_reading is not to
swamp your memory :) We can have that behaviour of course, but then
the input stream is not reusable.
> * I don't see SG(request_info).request_body being closed anywhere. Is this
> relying just on auto-cleanup? I recall that being problematic in debug mode
> unless you use php_stream_auto_cleanup().
Yes, list destructors take care of that (I wrote that patch in debug mode).
> * The php://input streams simply forward the calls to
> SG(request_info).request_body, so if you open two, the per-stream cursor
> position may not match the position of the wrapped stream (even if it
> matched, we wouldn't want one stream to affect the position of the other). I
True! So my input stream handles concurrency as bad as the current
implementation... 8)
> don't see any easy way out here; all I can think of is is duping the file
> descriptor for the temporary file in these situations. But then the book
> keeping for php://input would be more complicated.
I think that shouldn't be too hard to fix. But it depends on concern no.1 :)
> * The cast added php_stream_get_resource_id() seems unnecessary; may hide
> bugs. If you just want to be able to pass void* values, better to put an
> inline function there with a php_stream* parameter, that way you would have
> a compiler warning if you passed anything but a php_stream* or a void*
> (though this would break taking a pointer).
Thanks, that's a left-over, because I had `void *request_body` in the
first place.
--
Regards,
Mike
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
