Improve performance of some path expressions #303
Merged
Conversation
This file contains 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
When parsing a file with NagiosObjects and many objects inside, a large
tree forms like this:
/files/etc/nagios/objects/services.cfg/service[1]
/files/etc/nagios/objects/services.cfg/service[2]
...
/files/etc/nagios/objects/services.cfg/service[5000]
aug_get performance is pretty terrible at this size when using a path
above, due to the evaluation of the [5000] predicate. Paths without a
predicate (e.g. using seq or uniquely labelled nodes) are retrieved
quickly.
The time taken to set and get 5,000 such nodes is recorded in
tests/test-perf.log and is currently about:
testPerfPredicate = 66636ms
A helper to run Valgrind's callgrind tool is added to src/try.
When we evaluate path expressions, we need to construct nodesets, and they truly need to be sets, i.e. contain each node at most once. This leads to problems when a node has lots of children with the same name. The duplicate check in ns_add lead to O(n^2) behavior for nodes with n children. We simplify this by adding a flag to each tree node that we use only around repeated calls of ns_add when we construct a node set. This is possible since the construction of nodesets is very local, and it is therefore easy to determine where we need to put the call to ns_clear_added when we are done building the nodeset.
We used to remove nodes that did not match one-by-one by calling memmove
for each of them. Now we batch runs of non-matching nodes and call memmove
only once for each run. The common case of a predicate that matches only
one node at a certain position ('service[17]') now requires two memmoves
rather than size(ns)-1 many memmoves.
This leads to a drastic performance improvement for large nodesets.
|
That's excellent, thanks for taking this up. The optimisations all make sense, and work very well with the test case (68064ms, 18069ms, 1139ms, 318ms per respective commit). Even without the special case it's such a significant improvement that it should be usable for other predicates. 👍 |
…ions We know that in a path expression like 'name[42]', only one node can possibly match, and that we simply need to step through all the nodes and count until we've reached the 42nd matching node. This greatly simplifies how we construct the resulting nodeset, and is done in the new function position_filter.
|
Thanks for all the work in figuring out where the perf bottlenecks are - that made writing this a ton easier ! |
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.
Path expressions of the form
service[75]were very slow when there were a large number ofservicenodes. These patches try to speed that up.Tons of thanks to @domcleal for figuring out why this was slow, writing a test that demonstrates the problem and providing lots of detail on what exactly was going on.