improve performance in simplexml #2855
Conversation
| Array properties = Array::Create(); | ||
| sxe_get_prop_hash(sxe, true, properties); | ||
| return node != nullptr || properties.size(); | ||
| if (node) { |
There was a problem hiding this comment.
Style nit: it's a bit easier to read to lower the amount of nesting if you remove the else block and just do:
if (node) return true;
Array properties ...
There was a problem hiding this comment.
@danslo I can make this style change for you in the internal pull.
When casting a SimpleXML element to boolean, we're only concerned with the object having an XML node, *or* having attributes. Building attributes is done by sxe_get_prop_hash that also recursively builds the entire element tree starting from that element. We shortcircuit when the object has a valid XML node to prevent excessive amount of object creation when all your program is doing is loading XML strings and doing read/toBool operations. Before: http://i.imgur.com/TFFj97u.gif After: http://i.imgur.com/3kNpwIM.gif
|
@danslo Thanks! From my end, this looks like a nice fix. Excellent. This has been pulled in. Internal Diff Number: D1364022 |
|
@danslo: not directly related to this issue, but how much are you planning on working on libxml stuff? |
|
@fredemmott SimpleXML is a priority for me because its heavily used in Magento. You're right, in php-src there's a shared object between SimpleXML and DOMDocument, stuff like php_libxml_increment_node_ptr. I did not port that over to limit the scope of my work. Between SimpleXMLElement and SimpleXMLElementIterator, I work around the issue by just incReffing and decReffing in the right places. I can work with you on that, but prefer spending time in other areas (currently a bunch of SOAP stuff). I also still need to get around converting SimpleXML to HNI, do the OSX fastcgi and JIT stuff. So much to do, so little time :-) |
This is a follow up to PR facebook#2855. When we need to build our element's child tree (sxe_get_prop_hash) to determine if it casts to true/false, we can get away with bailing after finding the first element. In my small Magento siege tests these result in significant improvements: Before: Transactions: 1551 hits Transaction rate: 51.79 trans/sec Lowest response time: 97ms Callgrind: http://i.imgur.com/npxMo0q.png After: Transactions: 1730 hits Transaction rate: 58.43 trans/sec Lowest response time: 89ms Callgrind: http://i.imgur.com/Dgojb9Y.png
Summary: This is a follow up to PR #2855. When we need to build our element's child tree (sxe_get_prop_hash) to determine if it casts to true/false, we can get away with bailing after finding the first element. In my small Magento siege tests these result in significant improvements: Before: Transactions: 1551 hits Transaction rate: 51.79 trans/sec Lowest response time: 97ms Callgrind: http://i.imgur.com/npxMo0q.png After: Transactions: 1730 hits Transaction rate: 58.43 trans/sec Lowest response time: 89ms Callgrind: http://i.imgur.com/Dgojb9Y.png Closes #2868 Reviewed By: @ptarjan Differential Revision: D1368727 Pulled By: @JoelMarcey
When casting a SimpleXML element to boolean, we're only concerned with
the object having an XML node, or having attributes. Building
attributes is done by sxe_get_prop_hash that also recursively builds the
entire element tree starting from that element.
We shortcircuit when the object has a valid XML node to prevent excessive
amount of object creation when all your program is doing is loading
XML strings and doing read/toBool operations.
Before: http://i.imgur.com/TFFj97u.gif
After: http://i.imgur.com/3kNpwIM.gif