Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:116266
To: Nikita Popov <nikita.ppv@gmail.com>,
 PHP internals <internals@lists.php.net>
References: <CAF+90c9_qu=PiTS81J2mesPfJdz7n5nSi3qoLNLjhEGN-aTytA@mail.gmail.com>
 <CAF+90c-jw48ymXvO34E+oVua6_FV0NhwE_jnPgBZ=LurthnKeA@mail.gmail.com>
Message-ID: <a2719507-7d39-ef12-f87f-0570de0094e2@wikimedia.org>
Date: Wed, 13 Oct 2021 12:43:33 +1100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.13.0
MIME-Version: 1.0
In-Reply-To: <CAF+90c-jw48ymXvO34E+oVua6_FV0NhwE_jnPgBZ=LurthnKeA@mail.gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
Content-Language: en-US
Subject: Re: [PHP-DEV] Re: [RFC] Deprecate dynamic properties
From: 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