Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix #79700: Bad performance with namespaced nodes due to wrong libxml assumption #11376

Closed
wants to merge 4 commits into from
Closed

Fix #79700: Bad performance with namespaced nodes due to wrong libxml assumption #11376

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
75b58bd
Use a prepending strategy instead of appending in dom_set_old_ns()
nielsdos Jun 5, 2023
84284a4
Reuse namespaces from doc->oldNs if possible in dom_get_ns()
nielsdos Jun 5, 2023
726beeb
Add a test for reconciling a reused namespace
nielsdos Jun 7, 2023
4547312
Explain why there can't be a cycle between oldNs and nsDef
nielsdos Jun 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
53 changes: 37 additions & 16 deletions ext/dom/php_dom.c
View file
Open in desktop
Expand Up @@ -1369,11 +1369,16 @@ void dom_normalize (xmlNodePtr nodep)

/* {{{ void dom_set_old_ns(xmlDoc *doc, xmlNs *ns) */
void dom_set_old_ns(xmlDoc *doc, xmlNs *ns) {
xmlNs *cur;

if (doc == NULL)
return;

ZEND_ASSERT(ns->next == NULL);
nielsdos marked this conversation as resolved.
Show resolved Hide resolved

/* Note: we'll use a prepend strategy instead of append to
* make sure we don't lose performance when the list is long.
* As libxml2 could assume the xml node is the first one, we'll place our
* new entries after the first one. */

if (doc->oldNs == NULL) {
doc->oldNs = (xmlNsPtr) xmlMalloc(sizeof(xmlNs));
if (doc->oldNs == NULL) {
Expand All @@ -1383,13 +1388,10 @@ void dom_set_old_ns(xmlDoc *doc, xmlNs *ns) {
doc->oldNs->type = XML_LOCAL_NAMESPACE;
doc->oldNs->href = xmlStrdup(XML_XML_NAMESPACE);
doc->oldNs->prefix = xmlStrdup((const xmlChar *)"xml");
} else {
ns->next = doc->oldNs->next;
}

cur = doc->oldNs;
while (cur->next != NULL) {
cur = cur->next;
}
cur->next = ns;
doc->oldNs->next = ns;
}
/* }}} end dom_set_old_ns */

Expand All @@ -1411,6 +1413,9 @@ static void dom_reconcile_ns_internal(xmlDocPtr doc, xmlNodePtr nodep)
} else {
prevns->next = nsdftptr;
}
/* Note: we can't get here if the ns is already on the oldNs list.
* This is because in that case the definition won't be on the node, and
* therefore won't be in the nodep->nsDef list. */
dom_set_old_ns(doc, curns);
curns = prevns;
}
Expand Down Expand Up @@ -1509,22 +1514,38 @@ NAMESPACE_ERR: Raised if

/* {{{ xmlNsPtr dom_get_ns(xmlNodePtr nodep, char *uri, int *errorcode, char *prefix) */
xmlNsPtr dom_get_ns(xmlNodePtr nodep, char *uri, int *errorcode, char *prefix) {
xmlNsPtr nsptr = NULL;

*errorcode = 0;
xmlNsPtr nsptr;

if (! ((prefix && !strcmp (prefix, "xml") && strcmp(uri, (char *)XML_XML_NAMESPACE)) ||
(prefix && !strcmp (prefix, "xmlns") && strcmp(uri, (char *)DOM_XMLNS_NAMESPACE)) ||
(prefix && !strcmp(uri, (char *)DOM_XMLNS_NAMESPACE) && strcmp (prefix, "xmlns")))) {
/* Reuse the old namespaces from doc->oldNs if possible, before creating a new one.
* This will prevent the oldNs list from growing with duplicates. */
xmlDocPtr doc = nodep->doc;
if (doc && doc->oldNs != NULL) {
nsptr = doc->oldNs;
do {
if (xmlStrEqual(nsptr->prefix, (xmlChar *)prefix) && xmlStrEqual(nsptr->href, (xmlChar *)uri)) {
goto out;
}
Comment on lines +1528 to +1530
Copy link
Member

Choose a reason for hiding this comment

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

Is this because there is nothing to do? As the node is "equal"?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. Note that the loop variable is the same as the return value. So when we found an exact match in the loop the loop variable is returned.

nsptr = nsptr->next;
} while (nsptr);
}
/* Couldn't reuse one, create a new one. */
nsptr = xmlNewNs(nodep, (xmlChar *)uri, (xmlChar *)prefix);
if (UNEXPECTED(nsptr == NULL)) {
goto err;
}
} else {
goto err;
}

if (nsptr == NULL) {
*errorcode = NAMESPACE_ERR;
}

out:
*errorcode = 0;
return nsptr;

err:
*errorcode = NAMESPACE_ERR;
return NULL;
}
/* }}} end dom_get_ns */

Expand Down
42 changes: 42 additions & 0 deletions ext/dom/tests/reconcile_reused_namespace.phpt
View file
Open in desktop
@@ -0,0 +1,42 @@
--TEST--
Reconcile a reused namespace from doc->oldNs
--EXTENSIONS--
dom
--FILE--
<?php

$dom = new DOMDocument();
$root = $dom->createElementNS('http://www.w3.org/2000/xhtml', 'html');

$dom->loadXML(<<<XML
<?xml version="1.0"?>
<html
xmlns="http://www.w3.org/2000/xhtml"
xmlns:a="http://example.com/A"
xmlns:b="http://example.com/B"
/>
XML);
$root = $dom->firstElementChild;

echo "Add first\n";
$element = $dom->createElementNS('http://example.com/B', 'p', 'Hello World');
$root->appendChild($element);

echo "Add second\n";
$element = $dom->createElementNS('http://example.com/A', 'p', 'Hello World');
$root->appendChild($element);

echo "Add third\n";
$element = $dom->createElementNS('http://example.com/A', 'p', 'Hello World');
$root->appendChild($element);

var_dump($dom->saveXML());

?>
--EXPECT--
Add first
Add second
Add third
string(201) "<?xml version="1.0"?>
<html xmlns="http://www.w3.org/2000/xhtml" xmlns:a="http://example.com/A" xmlns:b="http://example.com/B"><b:p>Hello World</b:p><a:p>Hello World</a:p><a:p>Hello World</a:p></html>
"