Everything

php

[RFC][Discussion] Add support for stream-wrapped URLs in opcode cache

php.internals

François Laupretre

9 years ago

Hi,

I don't know if it is still time for 7.2 but here is a new RFC to read
and comment :

https://wiki.php.net/rfc/url-opcode-cache

Regards

François

Dan Ackroyd

9 years ago

On 25 May 2017 at 19:17, François Laupretre <francois@php.net> wrote:

> Hi,
>
> I don't know if it is still time for 7.2 but here is a new RFC to read and
> comment :
>
> https://wiki.php.net/rfc/url-opcode-cache
>

The proposed version is just listed as 'next' - please could this be
specific, as next could mean next bug fix version.

btw I presume https://github.com/php/php-src/pull/1117 is a typo, as
it leads to "Fix ARG_INFO for levenshtein", which seems out of scope
for the RFC?


cheers
Dan

François Laupretre

9 years ago

Hi Dan,

Thanks for your comments. Both are fixed now.

Regards

François

Le 02/06/2017 à 15:37, Dan Ackroyd a écrit :

Dan Ackroyd

9 years ago

On 2 June 2017 at 17:31, François Laupretre <francois@tekwire.net> wrote:

> Hi Dan,
>
> Thanks for your comments. Both are fixed now.
>

Awesome.

btw it's my belief that very boring technical discussions about RFC
pull requests are best done via the github comments, both to reduce
the noise on this list and also so that people are can be precise, as
to which line they are talking about. I can't see any other RFCs that
had mundane code issues discussed on the list.

However as you declined to respond on Github, I'll ask here again:

1) Why is the patch checking for PHP's own version? This doesn't occur
anywhere else in the code

2) Why does the patch modify PHP_API_VERSION, which was already
modified for PHP 7.2

Additionally, the section for 'Backward Incompatible Changes' has
'None' listed - but then it lists some required modifications in "To
Existing Extensions" - won't non-core PHP extensions also be affected
by this RFC?

cheers
Dan

François Laupretre

9 years ago

Hi Dan,

Thanks for your comments.

Le 03/06/2017 à 15:34, Dan Ackroyd a écrit :

> However as you declined to respond on Github, I'll ask here again:

I am sorry to say that I didn't decline anything, as you never commented
the PR. It seems I didn't reply to questions you didn't ask :). I also
prefer technical discussions to occur on github.

> 1) Why is the patch checking for PHP's own version? This doesn't occur
> anywhere else in the code

The PR had no target version and I wanted to show how to make an
extension compatible with older versions. But I agree there's no need
for such compatibility among core components. So, I removed the '#if"
checks and just added a small comment in php_streams.h to make extension
developers' life easier.

> 2) Why does the patch modify PHP_API_VERSION, which was already
> modified for PHP 7.2

Right. As the patch had no target version, I had defined an arbitrary
value to check. This is now reverted to the value defined for 7.2.

> Additionally, the section for 'Backward Incompatible Changes' has
> 'None' listed - but then it lists some required modifications in "To
> Existing Extensions" - won't non-core PHP extensions also be affected
> by this RFC?

Good question. The 'phar' and 'plain' wrappers had to be modified
because their names were hardcoded in the opcache code. Every other
stream wrapper (core or not) is currently considered as 'non-cacheable'
and will remain, as long as it does not implement a 'cache_key'
operation. So, we can be sure that no other extension will be impacted
by the RFC.

Actually, any 3rd-party opcode cache would have to be modified to become
able to cache stream-wrapped URLs (which would not be a BC break). But,
AFAIK, opcache is the only opcode cache supported by PHP 7. So, I don't
think I need to list this in the RFC.

Cheers

François

Sara Golemon

9 years ago

On Thu, May 25, 2017 at 2:17 PM, François Laupretre <francois@php.net> wrote:

> I don't know if it is still time for 7.2 but here is a new RFC to read and
> comment :
>

Official feature freeze (and branch fork) is Jun 20. Ideally get
things merged before then, but there's wiggle room for open/voting
RFCs.

> https://wiki.php.net/rfc/url-opcode-cache
>

I haven't reviewed the PR, but the concept is very +1 from me.  I have
several transpiler concepts I'd want to apply this to.  Big thumbs up
for making this work with userspace streams as well. :D

-Sara

François Laupretre

9 years ago

Hi,

Opening vote for : https://wiki.php.net/rfc/url-opcode-cache

Voting period will end Monday, July 3, 2017, 00:00 UTC.

Regards

François

=?utf-8?B?5rGf5rmW5aSn6Jm+5LuB?=

9 years ago

Hi François,
Good work, but I'm not sure if I missed something because I didn't find any discuss relating to this RFC in my mail box, as well as the PR on GitHub. I have a question about the detail of `cache_key`:

> If the method is not defined, every path associated to this wrapper are  non-cacheable. In order to avoid key value conflicts, the returned key  must start with the same '<scheme>://' prefix as the input path.  If it not the case, an error is raised and the key is ignored.

Why don't we simply ask `cache_key` to provide the path, and we combine it with scheme in the core? It's much easier for userland to implement this feature since there is less limits, and we didn't need to worry about the key conflicts between different stream-wrappers any more.

best regards,
CHU Zhaowei

------------------ Original ------------------
From:  "François Laupretre"<francois@tekwire.net>;
Date:  Tue, Jun 20, 2017 02:12 AM
To:  "François Laupretre"<francois@php.net>; "Internals"<internals@lists.php.net>;

Subject:  Re: [PHP-DEV] [RFC][VOTE] Add support for stream-wrapped URLs inopcode cache

Hi,

Opening vote for : https://wiki.php.net/rfc/url-opcode-cache

Voting period will end Monday, July 3, 2017, 00:00 UTC.

Regards

François

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

François Laupretre

9 years ago

Hi,

Le 20/06/2017 à 18:35, 江湖大虾仁 a écrit :

> Hi François,
>   Good work, but I'm not sure if I missed something because I didn't
> find any discuss relating to this RFC in my mail box, as well as the
> PR on GitHub. I have a question about the detail of `cache_key`:

Discussion : http://www.serverphorums.com/read.php?7,1505444

> > If the method is not defined, every path associated to this wrapper
> are non-cacheable. In order to avoid key value conflicts, the returned
> key must start with the same '<scheme>://' prefix as the input path.
> If it not the case, an error is raised and the key is ignored.
>
> Why don't we simply ask `cache_key` to provide the path, and we
> combine it with scheme in the core? It's much easier for userland to
> implement this feature since there is less limits, and we didn't need
> to worry about the key conflicts between different stream-wrappers any
> more.

1. The userspace code needs to get the full URL as input, as a stream
wrapper may be associated with more than one scheme.
2. In the most usual case where the input URL is to be used as key, I
cannot ask developers to compute the returned path by removing the
'<scheme>://' prefix from the input URL. The API is much easier to
understand if they just return the input URL. It is also faster in this
case because the PHP function/method returning its input argument won't
even duplicate the data (it will just increment the zend_string
reference count).

Regards

François

Andrey Andreev

9 years ago

Hi,

On Mon, Jun 19, 2017 at 9:12 PM, François Laupretre
<francois@tekwire.net> wrote:

> Hi,
>
> Opening vote for : https://wiki.php.net/rfc/url-opcode-cache
>
> Voting period will end Monday, July 3, 2017, 00:00 UTC.
>

You should start a new thread to announce the vote start, or people
not actively engaged in the (already boring) discussion might miss it.

Cheers,
Andrey.

Dmitry Stogov

9 years ago

hi François,

The PR is incomplete, so I can't test and even understand the idea completely.

In my opinion, user defined streams can't be cached, because the wrapper code itself may be changed from request to request.

-1

Thanks. Dmitry.

________________________________
From: François Laupretre <francois@tekwire.net>
Sent: Monday, June 19, 2017 9:12:57 PM
To: François Laupretre; Internals
Subject: Re: [PHP-DEV] [RFC][VOTE] Add support for stream-wrapped URLs in opcode cache

Hi,

Opening vote for : https://wiki.php.net/rfc/url-opcode-cache

Voting period will end Monday, July 3, 2017, 00:00 UTC.

Regards

François

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

François Laupretre

9 years ago

Hi Dmitry,

Le 22/06/2017 à 09:42, Dmitry Stogov a écrit :

> The PR is incomplete, so I can't test and even understand the idea
> completely.
>
> In my opinion, user defined streams can't be cached, because the
> wrapper code itself may be changed from request to request.
>

Right. I hadn't imagined the case where two different applications would
use a common wrapper name. So, we should probably restrict the feature
to permanent (C code) wrappers.

Regards

François

Dmitry Stogov

9 years ago

This way should work.

Currently it works only for phar:// and file://, because opcache knows them, but it's possible to use some stream flag.

I suggest stop voting, re-work proposal, provide implementation, and try once again.

Thanks. Dmitry.

________________________________
From: François Laupretre <francois@tekwire.net>
Sent: Thursday, June 22, 2017 4:32:13 PM
To: Dmitry Stogov; Internals
Subject: Re: [PHP-DEV] [RFC][VOTE] Add support for stream-wrapped URLs in opcode cache

Hi Dmitry,

Le 22/06/2017 à 09:42, Dmitry Stogov a écrit :
The PR is incomplete, so I can't test and even understand the idea completely.

In my opinion, user defined streams can't be cached, because the wrapper code itself may be changed from request to request.

Right. I hadn't imagined the case where two different applications would use a common wrapper name. So, we should probably restrict the feature to permanent (C code) wrappers.

Regards

François

François Laupretre

9 years ago

Hi Dmitry,

Vote is closed as RFC needs more work regarding userspace streams.
Anyway, giving up for 7.2.

Regards

François

Le 23/06/2017 à 08:48, Dmitry Stogov a écrit :