Re: [PHP-DEV] Re: [RFC] Deprecate dynamic properties

This is only part of a thread. view whole thread
  116266
October 13, 2021 01:43 tstarling@wikimedia.org (Tim Starling)
On 12/10/21 9:23 pm, Nikita Popov wrote:
> Based on the received feedback, I've updated the RFC to instead provide an > #[AllowDynamicProperties] attribute as a way to opt-in to the use of > dynamic properties. As previously discussed, this won't allow us to > completely remove dynamic properties from the language model anymore, but > it will make the upgrade path smoother by avoiding multiple inheritance > issues. Especially given recent feedback on backwards-incompatible changes, > I think it's prudent to go with the more conservative approach.
I think it would still be the biggest compatibility break since PHP 4->5. Adding a custom property is a common way for an extension to attach data to an object generated by an uncooperative application or library. As the RFC notes, you could migrate to WeakMap, but it will be very difficult to write code that works on both 7.x and 8.2, since both attributes and WeakMap were only introduced in 8.0. In MediaWiki pingback data for the month of September, only 5.2% of users are on PHP 8.0 or later. Also, in the last week, I've been analyzing memory usage of our application. I've come to a new appreciation of the compactness of undeclared properties on classes with sparse data. You can reduce memory usage by removing the declaration of any property that is null on more than about 75% of instances. CPU time may also benefit due to improved L2 cache hit ratio and reduced allocator overhead. So if the point of the RFC is to eventually get rid of property hashtables from the engine, I'm not sure if that would actually be a win for performance. I'm more thinking about the need for a sparse attribute which moves declared properties out of properties_table. I would support an opt-in mechanism, although I think 8.2 is too soon for all core classes to opt in to it. We already have classes that throw an exception in __set(), so it would be nice to have some syntactic sugar for that. -- Tim Starling
  116267
October 13, 2021 08:05 nikita.ppv@gmail.com (Nikita Popov)
On Wed, Oct 13, 2021 at 3:43 AM Tim Starling <tstarling@wikimedia.org>
wrote:

> On 12/10/21 9:23 pm, Nikita Popov wrote: > > Based on the received feedback, I've updated the RFC to instead provide > an > > #[AllowDynamicProperties] attribute as a way to opt-in to the use of > > dynamic properties. As previously discussed, this won't allow us to > > completely remove dynamic properties from the language model anymore, but > > it will make the upgrade path smoother by avoiding multiple inheritance > > issues. Especially given recent feedback on backwards-incompatible > changes, > > I think it's prudent to go with the more conservative approach. > > I think it would still be the biggest compatibility break since PHP > 4->5. Adding a custom property is a common way for an extension to > attach data to an object generated by an uncooperative application or > library. > > As the RFC notes, you could migrate to WeakMap, but it will be very > difficult to write code that works on both 7.x and 8.2, since both > attributes and WeakMap were only introduced in 8.0. In MediaWiki > pingback data for the month of September, only 5.2% of users are on > PHP 8.0 or later. >
Just to be clear on this point: While attributes have only been introduced in 8.0, they can still be used in earlier versions and are simply ignored there. It is safe to use #[AllowDynamicProperties] without version compatibility considerations.
> Also, in the last week, I've been analyzing memory usage of our > application. I've come to a new appreciation of the compactness of > undeclared properties on classes with sparse data. You can reduce > memory usage by removing the declaration of any property that is null > on more than about 75% of instances. CPU time may also benefit due to > improved L2 cache hit ratio and reduced allocator overhead. >
Huh, that's an interesting problem. Usually I only see the reverse situation, where accidentally materializing the dynamic property table (through foreach, array casts, etc) causes issues, because it uses much more memory than declared properties. Based on a quick calculation, the cost of having dynamic properties clocks in at 24 declared properties (376 bytes for the smallest non-empty HT vs 16 bytes per declared property), so that seems like it would usually be a bad trade off unless you already end up materializing dynamic properties for other reasons. Did you make sure that you do not materialize dynamic properties (already before un-declaring some properties)?
> So if the point of the RFC is to eventually get rid of property > hashtables from the engine, I'm not sure if that would actually be a > win for performance. I'm more thinking about the need for a sparse > attribute which moves declared properties out of properties_table. >
The ability to opt-in to dynamic properties would always remain in some form (if only through stdClass extension as originally proposed), so if you have a case where it makes sense, the option would still be there. Regards, Nikita
  116268
October 13, 2021 08:19 rowan.collins@gmail.com (Rowan Tommins)
On 13/10/2021 02:43, Tim Starling wrote:
> I think it would still be the biggest compatibility break since PHP > 4->5.
I think this is a rather large exaggeration. It's certainly a use case we need to consider, but it's not on the scale of call-time pass-by-reference, or removing the mysql extension, or a dozen other changes that have happened over the years.
> As the RFC notes, you could migrate to WeakMap, but it will be very > difficult to write code that works on both 7.x and 8.2, since both > attributes and WeakMap were only introduced in 8.0.
Firstly, writing code that works on both 7.x and 8.2 will be easy: ignore the deprecation notices. As discussed elsewhere, treating a deprecation as a breaking change makes a nonsense of deprecating anything. Secondly, I would be surprised if libraries using this trick don't already have helper functions for doing it, in which case changing those to use WeakMap and falling back to the dynamic property implementation on old versions is easy. Maybe I'm missing some difficulty here? if ( class_exists('\\WeakMap') ) {     class Attacher {         private static $map;         public static function setAttachment(object $object, string $name, $value): void         {             self::$map ??= new WeakMap;             $attachments = self::$map[$object] ?? [];             $attachments[$name] = $value;             self::$map[$object] = $attachments;         }         public static function getAttachment(object $object, string $name)         {             self::$map ??= new WeakMap;             return self::$map[$object][$name] ?? null;         }     } } else {     class Attacher {         public static function setAttachment(object $object, string $name, $value): void         {             $attachments = $object->__attached_values__ ?? [];             $attachments[$name] = $value;             $object->__attached_values__ = $attachments;         }         public static function getAttachment(object $object, string $name)         {             return $object->__attached_values__[$name] ?? null;         }     } } $foo = new Foo; Attacher::setAttachment($foo, 'hello', 'world'); echo Attacher::getAttachment($foo, 'hello'), "\n";
> I would support an opt-in mechanism, although I think 8.2 is too soon > for all core classes to opt in to it. We already have classes that > throw an exception in __set(), so it would be nice to have some > syntactic sugar for that.
I proposed "locked classes" a while ago, but consensus was that this was the wrong way around. One suggestion that did come up there was a block-scopable declare which would allow manipulating dynamic properties, even without the target class's co-operation. However, I think WeakMap (with a helper and a fallback like above) is probably a superior solution for that, because adding properties outside the class always carries risk of name collision, interaction with __get, etc Regards, -- Rowan Tommins [IMSoP]
  116269
October 13, 2021 09:17 guilliam.xavier@gmail.com (Guilliam Xavier)
Off-topic:

if ( class_exists('\\WeakMap') ) {
>
People should stop using a leading slash in FQCN *strings*. \Foo::class is "Foo", not "\Foo". It might work generally but that's asking for problems (e.g. for autoloaders). (Sorry.)
  116270
October 13, 2021 09:21 guilliam.xavier@gmail.com (Guilliam Xavier)
On Wed, Oct 13, 2021 at 11:17 AM Guilliam Xavier xavier@gmail.com>
wrote:

> Off-topic: > > if ( class_exists('\\WeakMap') ) { >> > > People should stop using a leading slash in FQCN *strings*. \Foo::class > is "Foo", not "\Foo". It might work generally but that's asking for > problems (e.g. for autoloaders). > > (Sorry.) >
And of course I mistyped "leading backslash"... Also externals.io seems to strip them, so: \\Foo::class is "Foo", not "\\Foo".
  116271
October 13, 2021 09:52 pierre-php@processus.org (Pierre)
Le 13/10/2021 à 11:17, Guilliam Xavier a écrit :
> Off-topic: > > if ( class_exists('\\WeakMap') ) { > People should stop using a leading slash in FQCN *strings*. \Foo::class is > "Foo", not "\Foo". It might work generally but that's asking for problems > (e.g. for autoloaders). > > (Sorry.) > And why not ? using the FQDN is 100% valid, autoloaders should take care
of this, not the end user. For what it worth, I'm using both, depending upon context, both can make sense. Regards, -- Pierre
  116272
October 13, 2021 12:42 rowan.collins@gmail.com (Rowan Tommins)
On 13/10/2021 10:17, Guilliam Xavier wrote:
> Off-topic: > > if ( class_exists('\\WeakMap') ) { > > > People should stop using a leading slash in FQCN *strings*.  > \Foo::class is "Foo", not "\Foo".  It might work generally but that's > asking for problems (e.g. for autoloaders). > > (Sorry.)
Hah, good point. I should probably just have just used ::class anyway (in which case the leading backslash is the quickest way of the sample code being pasteable into some other file): if ( class_exists(\WeakMap::class) ) { There's probably a bunch of other style no-noes in that example, but I did at least test it worked. :) Regards, -- Rowan Tommins [IMSoP]
  116273
October 13, 2021 13:18 pierre-php@processus.org (Pierre)
Le 13/10/2021 à 14:42, Rowan Tommins a écrit :
> On 13/10/2021 10:17, Guilliam Xavier wrote: >> Off-topic: >> >>     if ( class_exists('\\WeakMap') ) { >> >> >> People should stop using a leading slash in FQCN *strings*. >> \Foo::class is "Foo", not "\Foo".  It might work generally but that's >> asking for problems (e.g. for autoloaders). >> >> (Sorry.) > > > Hah, good point. I should probably just have just used ::class anyway > (in which case the leading backslash is the quickest way of the sample > code being pasteable into some other file): > > if ( class_exists(\WeakMap::class) ) { > > There's probably a bunch of other style no-noes in that example, but I > did at least test it worked. :) > > > Regards, > I am surprised that:
if ( class_exists(\WeakMap::class) ) { Actually works when the class does not exist. It's not really surprising, I mean the FQDN::class in the end only resolve to a simple string no matter that the class exists or not, but, I'm being the devil's advocate here, there are use case were you would want the engine to crash when you explicitly reference/use a non existing class, even when it's only its name. I'm sorry this is off-topic. -- Pierre