Fix #79700: Bad performance with namespaced nodes due to wrong libxml assumption #5719
+52
−29
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The patch looks good to me. Thanks! |
cmb69
reviewed
Jun 15, 2020
| $root = $dom->createElementNS('http://www.w3.org/2000/xhtml', 'html'); | ||
| $dom->appendChild($root); | ||
|
|
||
| $s = microtime(true); |
There was a problem hiding this comment.
Perhaps better use hrtime() instead?
|
Nice catch! |
|
I have asked for feedback on the libxml list as well, hopefully we'll hear something: https://mail.gnome.org/archives/xml/2020-June/msg00000.html |
|
I fixed this last year differently. This PR is no longer applicable. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
ext/dom was wrongly using xmlDoc->oldNs as a linked list of all namespaces, without deduplicating already existing namespaces in the document. This could lead to the oldNs linked list becoming extremely large and performance degrading exponentially in the number of nodes with namespaces.
The bug https://bugs.php.net/bug.php?id=79700 has a test script showing the performance loss.
libxml2 uses
oldNsonly to store thexmlnamespace property on, which is an implicit namespace that does not need to be declared on a node. It is only ever used when searching for the "xml" prefix or the "http://www.w3.org/XML/1998/namespace" namespace. When its being used, its automatically created by libxml and does not need to be created by ext/dom.Removing the function
dom_set_old_nsdoes not break any tests, including the newly created using thexmlnamespace. Performance of the test script gets each batch of 10000 elements roughly the same append performance.Merge is suggested for master only, as i am a bit unsure about potential side effects that I can't see with oldNs, even after digging into libxml code for hours, I am not 100% sure about this.
/cc @cmb69 @ThomasWeinert