Memoize node presenters #2172
Merged
Memoize node presenters #2172
+95
−3
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
floehopper
force-pushed
the
memoize-node-presenters
branch
from
cb77e47 to
2c8781e
Compare
|
Based on the suspicion that
|
|
@chrisroos, @erkede: Is there any reason why I shouldn't merge this? I've addressed @erkede's earlier comment to memo-ize node presenters keyed on name in 1a79b4f. |
|
Looks good to me. |
I think this makes the code easier to read.
It's pretty surprising that such a critical class (`FlowPresenter`) had no unit tests prior to this. I want to make changes to the `#presenter_for` method, so I've only added tests for this method for now in order to give myself confidence that my upcoming changes do not break any existing behaviour. Unfortunately I had to add a single question to the flow, because `Flow#start_state` (which is used in calculating `FlowPresenter#current_state`) relies on there being at least one question in the flow. Note that these tests do not endorse the existing behaviour.
`FlowPresenter#presenter_for` is called many times on the same instance within a single request; most notably from within `FlowPresenter#collapsed_questions`. This meant that we were unnecessarily creating many instances of the subclasses of `NodePresenter`. An instance of `SmartAnswer::ErbRenderer` is created for each of these and an instance of `ActionView::Base` is created for each of those. We can reuse the relevant node presenter by memo-izing them in a `Hash` keyed on the node's name. It's safe to use the node's name as the key, because node names are unique for a flow i.e. it's not possible to have different node types with the same name within the same flow (see `Flow#add_node`). There's no need to key on the value returned by `FlowPresenter#current_state` as well, because this is itself memo-ized and is therefore always the same for a given instance of `FlowPresenter`. It's safe to memo-ize the node presenters, because a new `FlowPresenter` instance is created for every request within the `SmartAnswersController#find_smart_answer` method.
I want to make changes to the `#start_node` method, so I've only added a test for this method for now in order to give myself confidence that my upcoming changes do not break any existing behaviour.
`FlowPresenter#start_node` is called several times on the same instance within a single request. This meant that we were unnecessarily creating several instances of `StartNodePresenter`. An instance of `SmartAnswer::ErbRenderer` is created for each of these and an instance of `ActionView::Base` is created for each of those. We can reuse the start node presenter by memo-izing it. It's safe to do this, because a new `FlowPresenter` instance is created in every request within the `SmartAnswersController#find_smart_answer` method.
floehopper
force-pushed
the
memoize-node-presenters
branch
from
2c8781e to
7e020af
Compare
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.
We were creating far more instances of
NodePresenterand its subclasses than we needed. This in turn meant we were creating far more instances ofSmartAnswer::ErbRendererand thenceActionView::Basethan necessary. It may well also have meant that we were unnecessarily re-rendering the same template multiple times.In a recent release, all flows were converted to use ERB rendering for questions instead of YAML I18n rendering. This release coincided with the app using a lot more memory (private link). We have some evidence to suggest that making this change will (at the very least) reduce memory usage of the app somewhat. However, we haven't been able to unambiguously reproduce the memory usage problem we're seeing in production, so it's difficult to know whether this will actually resolve the problem.
In working on this change, it's highlighted how tangled an inefficient this area of the codebase is (e.g.
FlowPresenter#collapsed_questionsindirectly callsFlow#processmultiple times). I'm convinced that the code could be considerably simplified, but that's going to have to wait for another day.