Performance issues

[aidansteele]

Hi,

I've been using the library a bit more and have come across a new bottleneck. A significant amount of time is being spent in error checking / assertions. While this is very useful during development, once the app has been verified as "correct" it would be great if it was possible to turn these off.

A few examples (these are the most expensive that I've come across):

https://github.com/evolve75/RubyTree/blob/master/lib/tree.rb#L215
https://github.com/evolve75/RubyTree/blob/master/lib/tree.rb#L219-L220
https://github.com/evolve75/RubyTree/blob/master/lib/tree.rb#L369

The problem is: I'm not quite sure how to solve it. The naïve solution is for me to fork the project and simply delete these lines, but maintaining a fork is lame and it might be an issue that others face. Deleting the lines in your repo would be unacceptable because they are very handy during dev.

Ideally, we could opt out of these checks. Sort of like this:

StandardWarning.disable do
  node = Tree::TreeNode.new 1234
end

But with an addition: if the warnings are disabled, the checks shouldn't happen at all -- instead of having the checks occur and not raising. Do you know what I mean or do I need to clarify?

[evolve75]

Aidan,

One way could be to pass an optional parameter to the tree node during creation (which propagates to its child nodes automatically during additions), that skips the entire validation suite.

We will need to think this through though. I have been thinking for some time to enable a configuration mechanism of some sort (perhaps via a template factory) that generates the nodes to specification, and this could be one of its uses.

[packetmonkey]

In Prawn when creating a document we can pass in a debug flag that will verify all the options that get passed into a method, then internally that boolean guards the checks.

One nice thing about that plan is you can release that defaulting it to true, forcing users to disable it explicitly so you don't break backwards compatibility without making a major release semver style.

That would in theory give us something like Tree::TreeNode.new('name', 'content', { debug: false }), then as part of the addition process we could propagate those options to child nodes allowing you to easily set values on a per-tree basis. It could also set up a future pattern for additional configurations.

My concern with a StandardWarning.disable do style block is that would be less clean to enable in development vs production. Ideally I think we would end up with something similar in functionality to

options = { debug: true} if some_condition_true_an_env_var?
root = Tree::TreeNode.new 'root', 'content', options

That way the development and production code can be identical, we get the full benefits in development of the extra error checking and it gets out of the way when deployed.

[evolve75]

@packetmonkey and @aidansteele

This is definitely worth pursuing. I like the option that Evan has suggested, Do you have any code patch to get this started?