Cache visitor dispatch on a per-visitor basis #204
Merged
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.
Currently, visitor dispatch (via the
DISPATCH
) constant is cached on a global basis. This means that, for instance,Arel::Nodes::Union
will work fine when you use visitors implementingvisit_Arel_Nodes_Union
directly, likeVisitors::ToSql
.But if a visitor needs to go up the class hierarchy looking for the method, the dispatch table gets mutated. This is fine as a cache for a specific visitor class, assuming methods don't get added dynamically to the visitor classes (anybody doing that should probably expect problems anyway). But for this mutation to ruin other visitors' dispatch cache seems bad.
So this patch turns
DISPATCH
into a 2-level hash, first for the visitor class, then for the node class.The alternatives I see are:
Arel::Visitors::Visitor::DISPATCH.clear
(bad; they'd lose the perf benefits and have ugly code)Thoughts?