Skip to content

Virtual props xmlreader #15125

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Aug 26, 2024
Merged

Virtual props xmlreader #15125

merged 3 commits into from
Aug 26, 2024

Conversation

nielsdos
Copy link
Member

Allow ZEND_ACC_VIRTUAL to be used to not have property backing storage without resorting to hooks

This is useful to reduce the memory usage of objects that don't actually
use the backing storage. Examples are XMLReader and DOM. When the
properties were added to the stubs, these objects became much much
bigger, which is a waste of memory.

Closes #11644.

Works towards #13988.

Use @virtual annotation in XMLReader

All properties of XMLReader are virtual and therefore don't need backing
storage.

@nielsdos nielsdos force-pushed the virtual-props-xmlreader branch from 56b8726 to c3a9905 Compare August 2, 2024 16:30
@nielsdos nielsdos requested a review from iluuu1994 August 23, 2024 08:48
Copy link
Member

@iluuu1994 iluuu1994 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked through all the usages of ZEND_ACC_VIRTUAL. The only thing I found was a sub-optimal error message. I don't see any other issues.

diff --git a/ext/standard/var_unserializer.re b/ext/standard/var_unserializer.re
index 69dd3f84d3a..52fb9e304c5 100644
--- a/ext/standard/var_unserializer.re
+++ b/ext/standard/var_unserializer.re
@@ -561,7 +561,7 @@ static int is_property_visibility_changed(zend_class_entry *ce, zval *key)
 				return 1;
 			} else {
 				php_error_docref(NULL, E_WARNING,
-					"Cannot unserialize value for hooked property %s::$%s",
+					"Cannot unserialize value for virtual property %s::$%s",
 					ZSTR_VAL(existing_propinfo->ce->name), Z_STRVAL_P(key));
 				zval_ptr_dtor_str(key);
 				return -1;

…e without resorting to hooks

This is useful to reduce the memory usage of objects that don't actually
use the backing storage. Examples are XMLReader and DOM. When the
properties were added to the stubs, these objects became much much
bigger, which is a waste of memory.

Closes phpGH-11644.

Work towards phpGH-13988.
All properties of XMLReader are virtual and therefore don't need backing
storage.
@nielsdos nielsdos force-pushed the virtual-props-xmlreader branch from c3a9905 to fd0d738 Compare August 23, 2024 16:53
@nielsdos nielsdos requested a review from bukka as a code owner August 23, 2024 16:53
Co-authored-by: Ilija Tovilo <ilija.tovilo@me.com>
@nielsdos
Copy link
Member Author

@iluuu1994 Thanks, I rebased this and applied your patch too.

@ericmann @SakiTakamachi @NattyNarwhal I'm asking RM permission to merge this into master as this is a new internal feature (not user-facing). It adds the ability to use the virtual annotation in the stub generator to tell the engine to avoid generating backing storage for the property where the annotation is defined upon.

@NattyNarwhal
Copy link
Member

Tested and it seems to work and seems low-impact. I trust your judgement here for a merge.

@nielsdos nielsdos merged commit baac01f into php:master Aug 26, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Declare typed properties for internal classes without backing storage
5 participants