From c6655fb719c75b1db5e5e835e166d39b18aff2c0 Mon Sep 17 00:00:00 2001 From: nielsdos <7771979+nielsdos@users.noreply.github.com> Date: Mon, 29 May 2023 23:14:27 +0200 Subject: [PATCH] Implement dom_get_doc_props_read_only() I was surprised to see that getting the stricterror property showed in in the Callgrind profile of some tests. Turns out we sometimes allocate them. Fix this by returning the default in case no changes were made yet. Closes GH-11345. --- UPGRADING.INTERNALS | 5 +++++ ext/dom/document.c | 31 ++++++++++------------------ ext/dom/php_dom.c | 49 ++++++++++++++++++++++++--------------------- ext/dom/php_dom.h | 1 + 4 files changed, 43 insertions(+), 43 deletions(-) diff --git a/UPGRADING.INTERNALS b/UPGRADING.INTERNALS index b4675e22215..99b609a115b 100644 --- a/UPGRADING.INTERNALS +++ b/UPGRADING.INTERNALS @@ -116,6 +116,11 @@ PHP 8.3 INTERNALS UPGRADE NOTES - The PHPAPI spl_iterator_apply() function now returns zend_result instead of int. There are no functional changes. + f. ext/dom + - A new function dom_get_doc_props_read_only() is added to gather the document + properties in a read-only way. This function avoids allocation when there are + no document properties changed yet. + ======================== 4. OpCode changes ======================== diff --git a/ext/dom/document.c b/ext/dom/document.c index c60198a3be1..13324645e98 100644 --- a/ext/dom/document.c +++ b/ext/dom/document.c @@ -297,7 +297,7 @@ readonly=no int dom_document_format_output_read(dom_object *obj, zval *retval) { if (obj->document) { - dom_doc_propsptr doc_prop = dom_get_doc_props(obj->document); + libxml_doc_props const* doc_prop = dom_get_doc_props_read_only(obj->document); ZVAL_BOOL(retval, doc_prop->formatoutput); } else { ZVAL_FALSE(retval); @@ -322,7 +322,7 @@ readonly=no int dom_document_validate_on_parse_read(dom_object *obj, zval *retval) { if (obj->document) { - dom_doc_propsptr doc_prop = dom_get_doc_props(obj->document); + libxml_doc_props const* doc_prop = dom_get_doc_props_read_only(obj->document); ZVAL_BOOL(retval, doc_prop->validateonparse); } else { ZVAL_FALSE(retval); @@ -347,7 +347,7 @@ readonly=no int dom_document_resolve_externals_read(dom_object *obj, zval *retval) { if (obj->document) { - dom_doc_propsptr doc_prop = dom_get_doc_props(obj->document); + libxml_doc_props const* doc_prop = dom_get_doc_props_read_only(obj->document); ZVAL_BOOL(retval, doc_prop->resolveexternals); } else { ZVAL_FALSE(retval); @@ -372,7 +372,7 @@ readonly=no int dom_document_preserve_whitespace_read(dom_object *obj, zval *retval) { if (obj->document) { - dom_doc_propsptr doc_prop = dom_get_doc_props(obj->document); + libxml_doc_props const* doc_prop = dom_get_doc_props_read_only(obj->document); ZVAL_BOOL(retval, doc_prop->preservewhitespace); } else { ZVAL_FALSE(retval); @@ -397,7 +397,7 @@ readonly=no int dom_document_recover_read(dom_object *obj, zval *retval) { if (obj->document) { - dom_doc_propsptr doc_prop = dom_get_doc_props(obj->document); + libxml_doc_props const* doc_prop = dom_get_doc_props_read_only(obj->document); ZVAL_BOOL(retval, doc_prop->recover); } else { ZVAL_FALSE(retval); @@ -422,7 +422,7 @@ readonly=no int dom_document_substitue_entities_read(dom_object *obj, zval *retval) { if (obj->document) { - dom_doc_propsptr doc_prop = dom_get_doc_props(obj->document); + libxml_doc_props const* doc_prop = dom_get_doc_props_read_only(obj->document); ZVAL_BOOL(retval, doc_prop->substituteentities); } else { ZVAL_FALSE(retval); @@ -1176,7 +1176,6 @@ static xmlDocPtr dom_document_parser(zval *id, int mode, char *source, size_t so { xmlDocPtr ret; xmlParserCtxtPtr ctxt = NULL; - dom_doc_propsptr doc_props; dom_object *intern; php_libxml_ref_obj *document = NULL; int validate, recover, resolve_externals, keep_blanks, substitute_ent; @@ -1189,17 +1188,13 @@ static xmlDocPtr dom_document_parser(zval *id, int mode, char *source, size_t so document = intern->document; } - doc_props = dom_get_doc_props(document); + libxml_doc_props const* doc_props = dom_get_doc_props_read_only(document); validate = doc_props->validateonparse; resolve_externals = doc_props->resolveexternals; keep_blanks = doc_props->preservewhitespace; substitute_ent = doc_props->substituteentities; recover = doc_props->recover; - if (document == NULL) { - efree(doc_props); - } - xmlInitParser(); if (mode == DOM_LOAD_FILE) { @@ -1387,7 +1382,6 @@ PHP_METHOD(DOMDocument, save) size_t file_len = 0; int bytes, format, saveempty = 0; dom_object *intern; - dom_doc_propsptr doc_props; char *file; zend_long options = 0; @@ -1405,7 +1399,7 @@ PHP_METHOD(DOMDocument, save) /* encoding handled by property on doc */ - doc_props = dom_get_doc_props(intern->document); + libxml_doc_props const* doc_props = dom_get_doc_props_read_only(intern->document); format = doc_props->formatoutput; if (options & LIBXML_SAVE_NOEMPTYTAG) { saveempty = xmlSaveNoEmptyTags; @@ -1433,7 +1427,6 @@ PHP_METHOD(DOMDocument, saveXML) xmlBufferPtr buf; xmlChar *mem; dom_object *intern, *nodeobj; - dom_doc_propsptr doc_props; int size, format, saveempty = 0; zend_long options = 0; @@ -1444,7 +1437,7 @@ PHP_METHOD(DOMDocument, saveXML) DOM_GET_OBJ(docp, id, xmlDocPtr, intern); - doc_props = dom_get_doc_props(intern->document); + libxml_doc_props const* doc_props = dom_get_doc_props_read_only(intern->document); format = doc_props->formatoutput; if (nodep != NULL) { @@ -1928,7 +1921,6 @@ PHP_METHOD(DOMDocument, saveHTMLFile) size_t file_len; int bytes, format; dom_object *intern; - dom_doc_propsptr doc_props; char *file; const char *encoding; @@ -1947,7 +1939,7 @@ PHP_METHOD(DOMDocument, saveHTMLFile) encoding = (const char *) htmlGetMetaEncoding(docp); - doc_props = dom_get_doc_props(intern->document); + libxml_doc_props const* doc_props = dom_get_doc_props_read_only(intern->document); format = doc_props->formatoutput; bytes = htmlSaveFileFormat(file, docp, encoding, format); @@ -1969,7 +1961,6 @@ PHP_METHOD(DOMDocument, saveHTML) dom_object *intern, *nodeobj; xmlChar *mem = NULL; int format; - dom_doc_propsptr doc_props; id = ZEND_THIS; if (zend_parse_parameters(ZEND_NUM_ARGS(), @@ -1980,7 +1971,7 @@ PHP_METHOD(DOMDocument, saveHTML) DOM_GET_OBJ(docp, id, xmlDocPtr, intern); - doc_props = dom_get_doc_props(intern->document); + libxml_doc_props const* doc_props = dom_get_doc_props(intern->document); format = doc_props->formatoutput; if (nodep != NULL) { diff --git a/ext/dom/php_dom.c b/ext/dom/php_dom.c index 00725d3fb00..e02b0973291 100644 --- a/ext/dom/php_dom.c +++ b/ext/dom/php_dom.c @@ -140,6 +140,17 @@ int dom_node_children_valid(xmlNodePtr node) { } /* }}} end dom_node_children_valid */ +static const libxml_doc_props default_doc_props = { + .formatoutput = false, + .validateonparse = false, + .resolveexternals = false, + .preservewhitespace = true, + .substituteentities = false, + .stricterror = true, + .recover = false, + .classmap = NULL, +}; + /* {{{ dom_get_doc_props() */ dom_doc_propsptr dom_get_doc_props(php_libxml_ref_obj *document) { @@ -149,28 +160,31 @@ dom_doc_propsptr dom_get_doc_props(php_libxml_ref_obj *document) return document->doc_props; } else { doc_props = emalloc(sizeof(libxml_doc_props)); - doc_props->formatoutput = 0; - doc_props->validateonparse = 0; - doc_props->resolveexternals = 0; - doc_props->preservewhitespace = 1; - doc_props->substituteentities = 0; - doc_props->stricterror = 1; - doc_props->recover = 0; - doc_props->classmap = NULL; + memcpy(doc_props, &default_doc_props, sizeof(libxml_doc_props)); if (document) { document->doc_props = doc_props; } return doc_props; } } +/* }}} */ + +libxml_doc_props const* dom_get_doc_props_read_only(const php_libxml_ref_obj *document) +{ + if (document && document->doc_props) { + return document->doc_props; + } else { + return &default_doc_props; + } +} static void dom_copy_doc_props(php_libxml_ref_obj *source_doc, php_libxml_ref_obj *dest_doc) { - dom_doc_propsptr source, dest; + dom_doc_propsptr dest; if (source_doc && dest_doc) { - source = dom_get_doc_props(source_doc); + libxml_doc_props const* source = dom_get_doc_props_read_only(source_doc); dest = dom_get_doc_props(dest_doc); dest->formatoutput = source->formatoutput; @@ -212,10 +226,8 @@ void dom_set_doc_classmap(php_libxml_ref_obj *document, zend_class_entry *basece zend_class_entry *dom_get_doc_classmap(php_libxml_ref_obj *document, zend_class_entry *basece) { - dom_doc_propsptr doc_props; - if (document) { - doc_props = dom_get_doc_props(document); + libxml_doc_props const* doc_props = dom_get_doc_props_read_only(document); if (doc_props->classmap) { zend_class_entry *ce = zend_hash_find_ptr(doc_props->classmap, basece->name); if (ce) { @@ -230,16 +242,7 @@ zend_class_entry *dom_get_doc_classmap(php_libxml_ref_obj *document, zend_class_ /* {{{ dom_get_strict_error() */ int dom_get_strict_error(php_libxml_ref_obj *document) { - int stricterror; - dom_doc_propsptr doc_props; - - doc_props = dom_get_doc_props(document); - stricterror = doc_props->stricterror; - if (document == NULL) { - efree(doc_props); - } - - return stricterror; + return dom_get_doc_props_read_only(document)->stricterror; } /* }}} */ diff --git a/ext/dom/php_dom.h b/ext/dom/php_dom.h index fdfdd4e7a31..a7ae09384cf 100644 --- a/ext/dom/php_dom.h +++ b/ext/dom/php_dom.h @@ -97,6 +97,7 @@ typedef struct { dom_object *dom_object_get_data(xmlNodePtr obj); dom_doc_propsptr dom_get_doc_props(php_libxml_ref_obj *document); +libxml_doc_props const* dom_get_doc_props_read_only(const php_libxml_ref_obj *document); zend_object *dom_objects_new(zend_class_entry *class_type); zend_object *dom_nnodemap_objects_new(zend_class_entry *class_type); #ifdef LIBXML_XPATH_ENABLED