Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:116267
MIME-Version: 1.0
References: <CAF+90c9_qu=PiTS81J2mesPfJdz7n5nSi3qoLNLjhEGN-aTytA@mail.gmail.com>
 <CAF+90c-jw48ymXvO34E+oVua6_FV0NhwE_jnPgBZ=LurthnKeA@mail.gmail.com> <a2719507-7d39-ef12-f87f-0570de0094e2@wikimedia.org>
In-Reply-To: <a2719507-7d39-ef12-f87f-0570de0094e2@wikimedia.org>
Date: Wed, 13 Oct 2021 10:05:36 +0200
Message-ID: <CAF+90c8UsN6tPVn4gBsB2aoJRd=TUdRz+9WJtEuhTEBW3vHGrw@mail.gmail.com>
To: Tim Starling <tstarling@wikimedia.org>
Cc: PHP internals <internals@lists.php.net>
Content-Type: multipart/alternative; boundary="0000000000004f2ac405ce376e53"
Subject: Re: [PHP-DEV] Re: [RFC] Deprecate dynamic properties
From: nikita.ppv@gmail.com (Nikita Popov)

--0000000000004f2ac405ce376e53
Content-Type: text/plain; charset="UTF-8"

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

--0000000000004f2ac405ce376e53--