[PHP-DEV] Re: [PHP-CVS] com php-src: Don't force rebuild of symbol table, when populating $http_response_header variable by the HTTP stream wrapper: NEWS UPGRADING ext/standard/http_fopen_wrapper.c

2020-10-29 Thread Derick Rethans 
Hi,

On Thu, 29 Oct 2020, Dmitry Stogov wrote:

> On Thu, Oct 29, 2020 at 2:18 PM Derick Rethans  wrote:
> 
> > what is the reason for this change? It breaks some introspection 
> > code that I am working on that expects the http_response_header 
> > variable to always be set for stream wrappers, even if it's not used 
> > in code.
> 
> $http_response_heade makes a lot of troubles for optimization, JIT and 
> performance. It's already proposed to deprecate it in 8.1
> 
> I changed the behavior, because I just found a problem that can't be 
> fixed without performance loss. The current JIT implementation would 
> just leak memory if $http_response_header is not used but magically 
> created. We discussed this with Nikita and decided to be practical and 
> don't populate $http_response_header if it's not used in function 
> scope.
> 
> If EX(symbol_table) already exists, $http_response_header is going to 
> be created as before. If you show your code, I might try to find a 
> workaround for you.

Nikita already suggested a workaround, which is to read the stream meta 
data (through stream->wrapperdata) directly, which also seems like a 
much cleaner solution to me, and it works (even with PHP 5.3 :-) ).

cheers,
Derick

-- 
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: https://www.php.net/unsub.php

[PHP-DEV] Re: [PHP-CVS] com php-src: Don't force rebuild of symbol table, when populating $http_response_header variable by the HTTP stream wrapper: NEWS UPGRADING ext/standard/http_fopen_wrapper.c

2020-10-29 Thread Dmitry Stogov 
Hi Derick,

On Thu, Oct 29, 2020 at 2:18 PM Derick Rethans  wrote:

> Hi Dmitry,
>
> what is the reason for this change? It breaks some introspection code
> that I am working on that expects the http_response_header variable to
> always be set for stream wrappers, even if it's not used in code.
>

$http_response_heade makes a lot of troubles for optimization, JIT and
performance.
It's already proposed to deprecate it in 8.1

I changed the behavior, because I just found a problem that can't be fixed
without performance loss.
The current JIT implementation would just leak memory if
$http_response_header is not used but magically created.
We discussed this with Nikita and decided to be practical and don't
populate $http_response_header if it's not used in function scope.

If EX(symbol_table) already exists, $http_response_header is going to be
created as before.
If you show your code, I might try to find a workaround for you.


>
> As this is a late behavioural changes, I am also wondering whether this
> should have been introduced so late in the RC process.
>

Yeah, I'm not a big fan of breaking things :(

Thanks. Dmitry.




Thanks. Dmitry.


>
> cheers,
> Derick
>
>
> On Wed, 28 Oct 2020, Dmitry Stogov wrote:
>
> > Commit:2693f799be862bcaddd4204c10fb1e82156bb603
> > Author:Dmitry Stogov  Wed, 28 Oct 2020
> 12:59:00 +0300
> > Parents:   47a56208f0902ecb95d879197a7ed9a3ca9a7e61
> > Branches:  PHP-8.0 master
> >
> > Link:
> http://git.php.net/?p=php-src.git;a=commitdiff;h=2693f799be862bcaddd4204c10fb1e82156bb603
> >
> > Log:
> > Don't force rebuild of symbol table, when populating
> $http_response_header variable by the HTTP stream wrapper
> >
> > Changed paths:
> >   M  NEWS
> >   M  UPGRADING
> >   M  ext/standard/http_fopen_wrapper.c
> >
> >
> > Diff:
> > diff --git a/NEWS b/NEWS
> > index 6030664f35c..1c3eee15354 100644
> > --- a/NEWS
> > +++ b/NEWS
> > @@ -2,6 +2,10 @@ PHP
> NEWS
> >
> |||
> >  ?? ??? , PHP 8.0.0RC4
> >
> > +- Standard:
> > +
> > +  . Don't force rebuild of symbol table, when populating
> $http_response_header
> > +variable by the HTTP stream wrapper. (Dmitry)
> >
> >  29 Oct 2020, PHP 8.0.0RC3
> >
> > diff --git a/UPGRADING b/UPGRADING
> > index 1a270ba4155..579424fbef4 100644
> > --- a/UPGRADING
> > +++ b/UPGRADING
> > @@ -617,6 +617,8 @@ PHP 8.0 UPGRADE NOTES
> >. substr(), mb_substr(), iconv_substr() and grapheme_substr() now
> consistently
> >  clamp out-of-bounds offsets to the string boundary. Previously,
> false was
> >  returned instead of the empty string in some cases.
> > +  . Populating $http_response_header variable by the HTTP stream wrapper
> > +doesn't force rebuilding of symbol table anymore.
> >
> >  - Sysvmsg:
> >. msg_get_queue() will now return an SysvMessageQueue object rather
> than a
> > diff --git a/ext/standard/http_fopen_wrapper.c
> b/ext/standard/http_fopen_wrapper.c
> > index 50758ad0f4a..d865d7e2f97 100644
> > --- a/ext/standard/http_fopen_wrapper.c
> > +++ b/ext/standard/http_fopen_wrapper.c
> > @@ -981,7 +981,7 @@ php_stream
> *php_stream_url_wrap_http(php_stream_wrapper *wrapper, const char *pa
> >
> >   if (!Z_ISUNDEF(headers)) {
> >   if (FAILURE == zend_set_local_var_str(
> > - "http_response_header",
> sizeof("http_response_header")-1, , 1)) {
> > + "http_response_header",
> sizeof("http_response_header")-1, , 0)) {
> >   zval_ptr_dtor();
> >   }
> >   }
> >
> >
> > --
> > PHP CVS Mailing List (http://www.php.net/)
> > To unsubscribe, visit: http://www.php.net/unsub.php
> >
>
> --
> PHP 7.4 Release Manager
> Host of PHP Internals News: https://phpinternals.news
> Like Xdebug? Consider supporting me: https://xdebug.org/support
> https://derickrethans.nl | https://xdebug.org | https://dram.io
> twitter: @derickr and @xdebug
>

[PHP-DEV] Re: [PHP-CVS] com php-src: Don't force rebuild of symbol table, when populating $http_response_header variable by the HTTP stream wrapper: NEWS UPGRADING ext/standard/http_fopen_wrapper.c

2020-10-29 Thread Derick Rethans 
Hi Dmitry,

what is the reason for this change? It breaks some introspection code 
that I am working on that expects the http_response_header variable to 
always be set for stream wrappers, even if it's not used in code.

As this is a late behavioural changes, I am also wondering whether this 
should have been introduced so late in the RC process.

cheers,
Derick


On Wed, 28 Oct 2020, Dmitry Stogov wrote:

> Commit:2693f799be862bcaddd4204c10fb1e82156bb603
> Author:Dmitry Stogov  Wed, 28 Oct 2020 12:59:00 
> +0300
> Parents:   47a56208f0902ecb95d879197a7ed9a3ca9a7e61
> Branches:  PHP-8.0 master
> 
> Link:   
> http://git.php.net/?p=php-src.git;a=commitdiff;h=2693f799be862bcaddd4204c10fb1e82156bb603
> 
> Log:
> Don't force rebuild of symbol table, when populating $http_response_header 
> variable by the HTTP stream wrapper
> 
> Changed paths:
>   M  NEWS
>   M  UPGRADING
>   M  ext/standard/http_fopen_wrapper.c
> 
> 
> Diff:
> diff --git a/NEWS b/NEWS
> index 6030664f35c..1c3eee15354 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -2,6 +2,10 @@ PHP  
>   NEWS
>  
> |||
>  ?? ??? , PHP 8.0.0RC4
>  
> +- Standard:
> +
> +  . Don't force rebuild of symbol table, when populating 
> $http_response_header
> +variable by the HTTP stream wrapper. (Dmitry)
>  
>  29 Oct 2020, PHP 8.0.0RC3
>  
> diff --git a/UPGRADING b/UPGRADING
> index 1a270ba4155..579424fbef4 100644
> --- a/UPGRADING
> +++ b/UPGRADING
> @@ -617,6 +617,8 @@ PHP 8.0 UPGRADE NOTES
>. substr(), mb_substr(), iconv_substr() and grapheme_substr() now 
> consistently
>  clamp out-of-bounds offsets to the string boundary. Previously, false was
>  returned instead of the empty string in some cases.
> +  . Populating $http_response_header variable by the HTTP stream wrapper
> +doesn't force rebuilding of symbol table anymore.
>  
>  - Sysvmsg:
>. msg_get_queue() will now return an SysvMessageQueue object rather than a
> diff --git a/ext/standard/http_fopen_wrapper.c 
> b/ext/standard/http_fopen_wrapper.c
> index 50758ad0f4a..d865d7e2f97 100644
> --- a/ext/standard/http_fopen_wrapper.c
> +++ b/ext/standard/http_fopen_wrapper.c
> @@ -981,7 +981,7 @@ php_stream *php_stream_url_wrap_http(php_stream_wrapper 
> *wrapper, const char *pa
>  
>   if (!Z_ISUNDEF(headers)) {
>   if (FAILURE == zend_set_local_var_str(
> - "http_response_header", 
> sizeof("http_response_header")-1, , 1)) {
> + "http_response_header", 
> sizeof("http_response_header")-1, , 0)) {
>   zval_ptr_dtor();
>   }
>   }
> 
> 
> --
> PHP CVS Mailing List (http://www.php.net/)
> To unsubscribe, visit: http://www.php.net/unsub.php
> 

-- 
PHP 7.4 Release Manager
Host of PHP Internals News: https://phpinternals.news
Like Xdebug? Consider supporting me: https://xdebug.org/support
https://derickrethans.nl | https://xdebug.org | https://dram.io
twitter: @derickr and @xdebug

-- 
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: https://www.php.net/unsub.php