Morning Dmitry,
Thanks for your time reviewing the patch, appreciated.
> 1) nullable properties
I agree that we need a way to that, but I would rather see it covered
by nullable types rfc.
> 2) concerns about default values
Implicit defaults would only allow us to reduce read checks, it can't
eliminate them in all cases, magic can return value for declared property,
and an overloaded zend object doesn't necessarily use inline properties at
all, but there can still be a declared type in the user class ... think
class User extends Internal {} where Internal uses object handlers to
change the way properties are stored ...
I'm persuaded by the chance to reduce instructions anyway ... let me
think about that ...
> 3) disable unset
An early version of the patch did disable unset, Marcus came and said
he needed the ability to unset so that magic was invoked ... this is
horrible, I'm aware ...
There doesn't seem to be a consensus on this.
If we have default values, then the problem with optimization goes away
... so maybe 2) is the solution to this and not disabling unset ?
> 4) performance
It got more complicated in the last couple of days, let us asses
performance again when the patch has had some more work.
> because definition and usage are in different PHP scripts.
This is not a limitation of Zend, this is a limitation of opcache ...
which should probably be lifted before we look to do things like inlining
functions, and optimizing access to properties.
Let me think about and discuss all of this with some other people ...
and let me work on the patch some more ...
I'll ping again when I've solved some of these problems ...
Cheers
Joe
On Wed, Apr 6, 2016 at 9:45 AM, Dmitry Stogov <dmi...@zend.com> wrote:
> Hi Jow,
>
>
> First of all, I appreciate the amount of effort you already invested into
> this idea.
>
> Anyway, I still don't agree with the following terms of RFC and
> corresponding implementation:
>
>
> 1) While parameters allow null to be accepted as the default value, null
> is never a valid value for a typed property.
>
>
> I think we must have a way to make properties explicitly nullable.
> Otherwise we won't be able to define even simple binary tree...
>
>
> class Node {
>
> public Node $left = null;
>
> public Node $right = null;
>
> }
>
> 2) The implementation will raise an exception where a typed property is
> accessed before being initialized: <https://3v4l.org/cVkcj/rfc#tabs>
>
>
> This makes unnecessary complication, and opens thousands possibilities to
> cheat the engine.
>
>
> class A {
>
> public int $x;
>
> }
>
> $a = new A;
>
> var_dump($a->x++); // prints NULL (according to RFC, this should raise
> Exception)
>
>
> I propose to initialize typed properties with the value of corresponding
> type (if default value is omitted).
>
> boolean => false
>
> int => 0
>
> double => 0.0
>
> string => ""
>
> array => []
>
> object, resource, callable => null
>
> Actually, I propose implicit default values if they are not specified.
>
> This will always guarantee the type and eliminate run-time checks on read.
>
>
> 3) It is possible to unset typed properties, and return them to the same
> state as a property that was never set.
>
>
> This mean that properties types are not guaranteed, and
> Optimizer/Compilers/JIT won't be able to generate the best code performing
> type check on every read.
>
> I would prefer to disable unset() of typed properties.
>
>
> 4) Checking the type of a zval is an extremely cheap operation, there may
> be some very minor performance impact only when typed properties are used.
>
>
> The patch makes some slowdown even if typed properties are not used (0.5%
> more instruction retired on 100 requests to Wordpress according to
> callgrind)
>
>
> 5) In principle, knowing the type of a variable in advance allows you to
> make optimizations, and in the long term, there is good reason to think
> that typed properties will allow us to improve performance.
>
>
> This is an optimistic assumption [image: 😊]
>
> For now typed-parameters make slowdown except of expected speedup,
> properties are not going to change this.
>
>
> Actually the type of property may make some gain only if we know the type
> of object and its definition at compile-time, but in most cases we do't
> know the class definition, because definition and usage are in different
> PHP scripts.
>
> Especially with (2) and (3), even if we know type of object and property,
> we'll still have to check if property is initialised on each usage.
>
>
> Thanks. Dmitry.
>
>
> ------------------------------
> *From:* Dmitry Stogov
> *Sent:* Wednesday, April 6, 2016 10:32
> *To:* Joe Watkins
> *Subject:* Typed properties patch
>
>
> Ho Joe,
>
>
> I see some tests failures (SIGSEGV)
>
>
> > Test typed properties float widen at runtime
> [Zend/tests/type_declarations/typed_properties_027.phpt]
> > Test typed properties respect strict types (off)
> [Zend/tests/type_declarations/typed_properties_028.phpt]
> > Test typed properties unset __get magical magic
> [Zend/tests/type_declarations/typed_properties_030.phpt]
>
>
> valgrind shows problems on every 4-rd test at Zend/tests (454 of 1668
> tests), but it seems like a single problem
>
>
> $ cat ../Zend/tests/add_002.mem
> ==26053== Conditional jump or move depends on uninitialised value(s)
> ==26053== at 0x8627895: _zend_object_has_type_hints (zend_execute.h:367)
> ==26053== by 0x8629098: zend_std_write_property
> (zend_object_handlers.c:674)
> ==26053== by 0x85FA1B5: zend_update_property (zend_API.c:3894)
> ==26053== by 0x85FA38E: zend_update_property_string (zend_API.c:3952)
> ==26053== by 0x860F8DB: zend_default_exception_new_ex
> (zend_exceptions.c:222)
> ==26053== by 0x860F96F: zend_default_exception_new
> (zend_exceptions.c:236)
> ==26053== by 0x85F347E: _object_and_properties_init (zend_API.c:1303)
> ==26053== by 0x85F34B5: _object_init_ex (zend_API.c:1311)
> ==26053== by 0x86124F4: zend_throw_exception (zend_exceptions.c:881)
> ==26053== by 0x85EF256: zend_throw_error (zend.c:1316)
> ==26053== by 0x85E4622: add_function (zend_operators.c:969)
> ==26053== by 0x868824E: ZEND_ADD_SPEC_CV_CV_HANDLER
> (zend_vm_execute.h:44246)
>
>
> Thanks. Dmitry.
>
>
