Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:68630
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain gmail.com designates 209.85.215.41 as permitted sender)
MIME-Version: 1.0
Sender: mike.php.net@gmail.com
In-Reply-To: <521D174D.4030802@nebm.ist.utl.pt>
References: <CALJZF3iw7uzUN2z-aBfO34c=x9tY-gCFaDHRRZq-JYpxHgdmrg@mail.gmail.com>
	<521D174D.4030802@nebm.ist.utl.pt>
Date: Wed, 28 Aug 2013 08:14:09 +0200
Message-ID: <CALJZF3hC-bb33n9RvG0zsuMTyBA1SViAjvxmuorhOrbqrsxLHg@mail.gmail.com>
To: Gustavo Lopes <glopes@nebm.ist.utl.pt>
Cc: PHP Internals <internals@lists.php.net>
Content-Type: text/plain; charset=UTF-8
Subject: Re: [PHP-DEV] PROPOSAL: temp stream for post_data
From: mike@php.net (Michael Wallner)

Hi Gustavo, thank you for your review!

On 27 August 2013 23:17, Gustavo Lopes <glopes@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