On Wed, Oct 13, 2021 at 3:43 AM Tim Starling <tstarl...@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
