Re: [PHP-DEV] [RFC] [VOTE] FFI - Foreign Function Interface

[Christoph M. Becker]

On 24.12.2018 at 08:38, Dmitry Stogov wrote:

> On 12/22/18 1:32 PM, Nikita Popov wrote:
>
>> My main concern here is that this is a very new extension and I think 
>> that apart from you barely anyone had a chance to actually implement 
>> something based on it and gain experience using the proposed API. 
>> Bundling an extension with PHP means that the API becomes frozen in time 
>> and it becomes very hard to change anything.
>> I think that having FFI support is important, but I'm afraid that once 
>> we bundle this extension and it will see more usage, it will quickly 
>> turn out that the API needs to change to make it easier to use or 
>> accommodate more use-cases.
> 
> The API almost completely repeats LuaJIT and Python CFFI (that are 
> stable). Something might need to be improved, of course, but only in 
> case the extension is going to be widely used.

If we are concerned about the API stability, we could mark the extension
as experimental and exempt it from our usual BC guarantee, as suggested
by Niklas[1].  On the other hand, if PHP 8 will be next after PHP 7.4,
this may not even be necessary.

> The main reason, I like to have this in core - future integration with JIT.

It seems to me that JIT integration of FFI isn't the whole story, but
rather an intermediate step to be able to offer built-in functionality
written in PHP in the long run, which could be a huge enhancement.  Am I
correct?

Anyhow, my main concerns regarding the proposal are that people could
easily be led into thinking that FFI is *the* new way to interface with
external code, rendering classic extensions obsolete, and that
interfacing on the ABI level is even more error-prone than interfacing
on the C API level.  Both concerns could be alleviated by good
documentation, though.

[1] 

-- 
Christoph M. Becker

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

Re: [PHP-DEV] [RFC][Vote] Covariant Returns and ContravariantParameters

[Christoph M. Becker]

On 24.12.2018 at 13:20, Dmitry Stogov wrote:

> - The patch is incompatible with opcache (crashes on Wordpress, Drupal, and 
> probably any real-life app).
> 
> - the incompatibility with opcache, doesn't allow to check the performance 
> implication of the patch
> 
> - the patch has merge conflicts and travis tests doesn't run.
> 
> 
> Personally, I'm not against the proposal, but I'm definitely against this 
> implementation.
> 
> Probably, it's better to cancel the voting and restart when the 
> implementation is ready.
> 
> 
> It's pity to see, that nobody tries the implementation but blindly vote...

Well, one may argue that the voting is about the concept, but not about
a (preliminary) implementation.  The main concern, in my opinion, is
rather whether it is possible to implement the proposal without (much)
drawbacks (such as general performance regression).  A working patch
would be helpful to prevent cases where we accept a proposal, but later
face difficulties implementing it and/or can't agree on some of the
details[1][2].  We may consider to augment the RFC process to clearly
require a working implementation before starting the vote.

[1] 
[2] 

-- 
Christoph M. Becker

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

Re: [PHP-DEV] [RFC][Vote] Covariant Returns and Contravariant Parameters

[Dmitry Stogov]

Hi Levi,


I made just few tests, to understand that the implementation is at least 
incomplete.

The warning message depends on class declaration order and may be emitted or 
not.


  1.  This test produces a warning (as RFC proposes)





2. But this test misses warning





- The patch is incompatible with opcache (crashes on Wordpress, Drupal, and 
probably any real-life app).

- the incompatibility with opcache, doesn't allow to check the performance 
implication of the patch

- the patch has merge conflicts and travis tests doesn't run.


Personally, I'm not against the proposal, but I'm definitely against this 
implementation.

Probably, it's better to cancel the voting and restart when the implementation 
is ready.


It's pity to see, that nobody tries the implementation but blindly vote...


Thanks. Dmitry.


From: Dmitry Stogov 
Sent: Friday, December 21, 2018 10:28:05 AM
To: Levi Morrison
Cc: internals
Subject: Re: [PHP-DEV] [RFC][Vote] Covariant Returns and Contravariant 
Parameters



On 12/21/18 3:12 AM, Levi Morrison wrote:
> On Thu, Dec 20, 2018 at 3:35 PM Dmitry Stogov  wrote:
>>
>> Hi Levi,
>>
>>
>> It looks like the patch broke something related to opcache.
>>
>> It crashes at least on Wordpress and Drupal.
>>
>> The backtrace 
>> https://gist.github.com/dstogov/a2305381a5c9982cceca9e4e252d26c7 shows 
>> use-after-free in opcache (works fine with master).
>>
>> Inability to work with opcache, doesn't allow to check the performance 
>> impact.
>>
>>
>> It also broke few tests. Some crash. Some produce different warning/errors.
>>
>>
>> $ make test TESTS="Zend tests"
>>
>> ...
>>
>> =
>> FAILED TEST SUMMARY
>> -
>> ZE2 ArrayAccess and ArrayAccessReferenceProxy with references to main array 
>> [tests/classes/array_access_011.phpt]
>> Bug #21478 (Zend/zend_alloc.c :: shutdown_memory_manager produces segfault) 
>> [Zend/tests/bug21478.phpt]
>> Generator methods can yield by reference 
>> [Zend/tests/generators/generator_method_by_ref.phpt]
>> Testing to implement Serializable interface by traits 
>> [Zend/tests/traits/interface_003.phpt]
>> Handling of public fields with traits needs to have same semantics as with 
>> normal inheritance, however, we do add strict warnings since it is easier to 
>> run into something unexpeted with changing traits. 
>> [Zend/tests/traits/property009.phpt]
>> iterable type#004 - Parameter covariance 
>> [Zend/tests/type_declarations/iterable_004.phpt]
>> iterable type#005 - Return type covariance 
>> [Zend/tests/type_declarations/iterable_005.phpt]
>> =
>>
>> I'll try to play with patch and make a full code review on next week.
>>
>>
>> It would be great, if you fix opcache compatibility.
>>
>> If it can't be done in reasonable time, it's probably better to cancel 
>> voting and restart when ready.
>
> What OS and compiler are these on?

Linux (Fedora 28), GCC 8.2.1.

> How are you ensuring that opcache
> is on when these tests are run? I have not been experiencing these
> issues, so maybe I am not running it correctly. If I cannot reproduce
> them soon then I will agree to cancel the voting.

You should enable opcache in your php.ini

zend_extension=opcache.so
opcache.enable=1
opcache.enable_cli=1
opcache.optimization_level=-1
opcache.protect_memory=1 ; this is for testing only


Memory corruption bugs may lead to crash or not because of "luck", but
it's possible to see them using valgrind.

$ make test TESTS="-m tests/classes/array_access_011.phpt"
$ cat tests/classes/array_access_011.mem

This shows almost the same backtrace, as I already published.
Looks like an incorrect reference-counting on some string.

>
> There are some known issues outside of Zend. Notably some internal
> classes do not have valid method signatures with regards to
> inheritance which this patch exposed. These need fixed regardless of
> this RFC and I have begun work on some of them (see
> https://github.com/php/php-src/pull/3686 for one example).

That PR looks fine.

There is also a problem that this PR has merge conflict with master and
travis doesn't run tests.

Thanks. Dmitry.

Re: [PHP-DEV] [RFC] [VOTE] FFI - Foreign Function Interface

[Dmitry Stogov]

On 12/24/18 4:50 PM, Christoph M. Becker wrote:
> On 24.12.2018 at 08:38, Dmitry Stogov wrote:
> 
>> On 12/22/18 1:32 PM, Nikita Popov wrote:
>>
>>> My main concern here is that this is a very new extension and I think
>>> that apart from you barely anyone had a chance to actually implement
>>> something based on it and gain experience using the proposed API.
>>> Bundling an extension with PHP means that the API becomes frozen in time
>>> and it becomes very hard to change anything.
>>> I think that having FFI support is important, but I'm afraid that once
>>> we bundle this extension and it will see more usage, it will quickly
>>> turn out that the API needs to change to make it easier to use or
>>> accommodate more use-cases.
>>
>> The API almost completely repeats LuaJIT and Python CFFI (that are
>> stable). Something might need to be improved, of course, but only in
>> case the extension is going to be widely used.
> 
> If we are concerned about the API stability, we could mark the extension
> as experimental and exempt it from our usual BC guarantee, as suggested
> by Niklas[1].  On the other hand, if PHP 8 will be next after PHP 7.4,
> this may not even be necessary.

Right.

> 
>> The main reason, I like to have this in core - future integration with JIT.
> 
> It seems to me that JIT integration of FFI isn't the whole story, but
> rather an intermediate step to be able to offer built-in functionality
> written in PHP in the long run, which could be a huge enhancement.  Am I
> correct?

In current state FFI is more like a great toy. It allows to quickly 
prototype on top of binary interfaces, but the resulting performance is 
going to be poor, because of interpretation overhead.

However, LuaJIT proved that it's possible to integrate JIT and FFI and 
achieve almost C performance. This could open a real door for writing 
PHP extensions in PHP itslef.

> Anyhow, my main concerns regarding the proposal are that people could
> easily be led into thinking that FFI is *the* new way to interface with
> external code, rendering classic extensions obsolete, and that
> interfacing on the ABI level is even more error-prone than interfacing
> on the C API level.  Both concerns could be alleviated by good
> documentation, though.

Agree. FFI is dangerous, especially for programmers without C 
background. Without FFI they just can't make a lot of harm.
However, the proposal offers to enable FFI only for preloaded files (and 
CLI), by default.

Thanks. Dmitry.

> 
> [1] 
> 

Re: [PHP-DEV] [RFC] [VOTE] FFI - Foreign Function Interface

[Jefferson Gonzalez]

Is sad to see people voting against this feature, which would help move 
PHP into territories outside of the web development landscape. For those 
developers not interested in learning the zend engine (lack of time) but 
with enough knowledge about C, this extension would surely help a lot 
when in need of writing a php cli application that needs access to some 
C library. This would further help move PHP into the general purpose 
scripting language arena which is currently dominated by Python and 
others. And I know many people have this mindset of "use the right tool 
for the job" but, actually a programming language is already a set of 
tools, so why buy a fully brand new tool set when you can clearly extend 
the existing one by adding the missing pieces.


Can people explain why they are voting no to a feature that seems really 
well put and thought? Without disrespecting anyone but, in my opinion 
seems to me like a lack of vision.


I think that shipping this extension as part of the 7.4 release would be 
an opportunity for people to easily test it before the JIT/8.0 milestone 
is reached, giving people with time the opportunity to start writing 
stuff that then would be optimized(more practical) when JIT/8.0 lands, 
heck, this implementation is even faster than Python one and you can 
still find lots of stuff on the Python world that uses FFI...


I just hope such a feature which is a core part of most scripting 
languages is not kept as just an optional and forgotten pecl option.


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

Re: [PHP-DEV] [RFC] [VOTE] FFI - Foreign Function Interface

[Pierre Joye]

Hi,

I agree with the comments about safety here however I still vote yes. FFI
and Jit integration will be a critical part of php in a near future. As I
have seen in the past, it is extremely time consuming to maintain such
implementation outside the core, especially if performance and tight
integration are required. That's why I prefer to give this project more
chances to succeed.

best
Pierre

On Thu, Dec 20, 2018, 10:13 PM Dmitry Stogov  Hi internals,
>
>
> The FFI RFC is turned into voting state.
>
>
> https://wiki.php.net/rfc/ffi
>
>
> There were very few minor changes since the initial proposal, e.g.
> renaming FFI:array_type() into FFI::arrayType()
>
>
> Thanks. Dmitry.
>
>