-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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.
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
Switching to closure tree #588 #662
Conversation
Interesting, less changes than I was expecting (though that one test failure seems legit). It would be nice to get some benchmarks in some different situations before/after the change. Something like $X inserts into the children of a taxon, maybe a few of the common queries with $Y taxons. I don't fully understand the plusses and minuses of the change here. |
Hi @cbrunsdon, I am working on some benchmarks, I'll get back to you as soon as possible. |
Hi again, At some points, we were thinking maybe we would make tests for trees which are large (heigh number of nodes in each parent) as well as the profound ones (heigh number of levels). But I guess the stores would never have profound trees, so we skipped this case. Moreover, we tried also Adjacency List Structure that it would give good results since it does not have any extra table, neither there is no need to calculate lft and rgt. The following results are all made under a 27_931 nodes tree, 4 levels and each parent produces 30 nodes.
While generating the whole tree, we see that
As jstree was removed and now it must render the whole tree at once, we find this case is very significant. The winner here is
Here the insertion is made in root at left. We took the average of 10 insertions approximately.
Here we move the root's last descendant to the first child of root in the left. Average of 10 tries approximately.
Here we remove the first root child. Average of 10 deletions approximately. Comparing the last 3 operations Insert/Move/Remove, With kind regards, |
@@ -95,6 +98,23 @@ def child_index=(idx) | |||
move_to_child_with_index(parent, idx.to_i) unless new_record? | |||
end | |||
|
|||
def move_to_child_with_index(node, index) | |||
# closure_tree does not provide any method that move nodes regarding position | |||
# this method overrides move_to_child_with_index offered by awesome_nested_set |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait, "overrides"? Is awesome_nested_set
still around to be overridden, or does the method just follow the signature from awesome_nested_set
?
Also: We tend to have comments above method definitions, along with a little bit of YARD doc. If you could move that once we've come to a decision about this PR, that'd be great.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mamhoff you're right, not really overriding, it just follows the same logic as awesome_nested_set
, which is totally removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you rebase this PR on current master, as well as fix the failing test? Once that's done, this PR will be up for review again. |
Did you run your benchmarks against the whole stack, so that rendering is also measured? As we investigated in AlchemyCMS we found that rendering the tree is the slowest part. After switching to handlebars templates and only returning JSON from the controller we gained a ten fold speed up!! Again, just by rendering in the frontend instead of iterating over rails partials. Alchemy still uses Reference: |
a0202eb
to
b0a7b74
Compare
Closing as stale. Please reopen if you think this is still something we should to tackle. |
No description provided.