Fix for lotus/lotus issue 185 - Performance issue with rendering a partial view #86
Conversation
The framework configuration object should hold a collection of all the partials in the file system for the given load paths.
view_test.rb doesn't actually load the framework
This seems to be failing because other tests are overwritting the `configuration.root` path causing a side effect in this test. Solution is to explicitly reload the configuration with a known root path value.
Also add some tests to assert that the cache is actually being used rather than re-reading the partial template files
|
@stevehook thanks for the PR. Some remarks/questions:
|
|
@stevehook great work, would you be able to provide a benchmark too? |
|
Thanks for the comments @pascalbetz.
|
|
@stevehook i think it's a good start and its good to start small. |
|
I made a small example to test this, my findings: i never had a cache hit, the Can you check again? Or perhaps share your sample app? About the "unified" mechanism for template loading: i think this is something that @jodosha, @joneslee85, @AlfonsoUceda would need to chime in. What do you guys think? |
| # @since 0.6.0 | ||
| # @api private | ||
| def find_cached_template | ||
| partials = Lotus::View.configuration.partials[[view_template_dir, template_name].join(separator)] || |
There was a problem hiding this comment.
@stevehook Referencing Lotus::View here is unsafe. Multiple "copies" of the framework, with different configuration instances are allowed within the same Ruby process.
In Lotus projects we can have Web::View, or Admin::View, with different set of partials. Lotus::View shouldn't have partials in its registry.
To solve this problem, you should reference @view.configuration (see #find in this class).
There was a problem hiding this comment.
@stevehook This is Inappropriate Intimacy of Configuration. This method knows that partials are stored as a Hash and the structure of keys and values.
It could be opportune to expose Configuration#partial_for, we can pass all the needed arguments to this new method and let find_cached_template to just invoke it.
There was a problem hiding this comment.
Thanks for all the helpful comments @jodosha
This is the part that I found most difficult to understand. I've changed this to use Lotus::View::Configuration.for(@view). Not sure if it is quite right yet.
CC @pascalbetz I think this is the reason that the sample application you created would have not benefitted from the cache.
|
@pascalbetz I'll check my sample app as soon as I get chance. One thing I did have to do was run it with the |
|
@stevehook Thank you for this PR. It will give an perf boost to Lotus apps!! 👍 We probably need to add an integration test to prove that the whole stack works as expected. We have
|
|
@stevehook The next version of Can you please replace all the |
| # @since 0.6.0 | ||
| # @api private | ||
| def load_partials! | ||
| Lotus::View::Rendering::PartialTemplatesFinder.new.find_partials(root).each { |key, format, template| add_partial(key, format, template) } |
There was a problem hiding this comment.
Could you move this to multiple lines with do/end? i think this would be beter for readability.
I'm still not sure I fully understand the configuration hierarchy but I think this handles the case in which there are multiple applications in the same process. Also force the format keys to be symbols.
We need to explicitly reload the configuration in some of the tests to make this work. The common code is factored out into reload_configuration_helper.rb.
|
I've updated the PR and addressed some of the comments. I still need to get some performance figures and look at the integration tests among other things. |
| # @since x.x.x | ||
| # @api private | ||
| def load_partials! | ||
| Lotus::View::Rendering::PartialTemplatesFinder.new.find_partials(root).each do |key, format, template| |
There was a problem hiding this comment.
what do you think about module or class method instead?
There was a problem hiding this comment.
👍
PartialTemplatesFinder doesn't have any state so makes sense to me.
|
@stevehook i'll try and unify template loading on top of your changes. |
|
@stevehook can you run a benchmark with/without partials, with/without your changes so we can be sure that the change has the desired effect? |
|
I've done some rough benchmarks, using the technique that @smt116 used to diagnose the original problem. The test is hitting a very simple test application (that renders a single partial - https://github.com/stevehook/lotus-test). Seems to me there is a measurable difference especially with Puma. branch master with Webbrick: branch issue_185 with Webbrick: branch master with Puma: branch issue_185 with Puma: |
Also fix inconsistency in Configuration#load_partials!
|
nice, I love what I am seeing 👍 |
|
@stevehook 💯 Thank you very much! I just need some more time to review the entire PR. I want to merge this after we'll release v0.6.0 (next Tue). |
|
Thanks @jodosha - I didn't get change to add those integration tests yet. I can try to do that over the next few days. |
|
@stevehook Please do 😄 |
Created two new applications in fixtures because extending Store and CardDeck caused too many side-effects in/from existing tests
PartialFinder shouldn't have knowledge of the Configuration objects internal data structures
|
@jodosha I was having some trouble with those integration tests. My first attempt was to extend the existing fixtures as you suggested. stevehook/view@issue_185...stevehook:issue_185_integration_tests I extended the fixtures for I needed to add a layout template file to render So to avoid these side-effects I ended up creating two new small apps in fixtures (App1 and App2) just to test partials. These seem to work but there may be a simpler way to do this that I'm not seeing at the moment. |
You took the right decision then. 👍 These problems for our builds are due to global state ( |
|
|
||
| # @since x.x.x | ||
| # @api private | ||
| def find_cached_template |
There was a problem hiding this comment.
@stevehook This abstraction is not needed because #find only invokes it and don't do anything else. So #find_cached_template can become #find instead.
Add default encoding to the partial template Introduce `PartialFile` class to eliminate data clump Instantiate `PartialTemplatesFinder` with instance of configuration Remove redundant `PartialFinder#find_cached_template` method
|
I've rerun the simple performance benchmark using latest master and issue_185 branches, results below. It looks like the results are pretty consistent with the original measurements I took before the latest changes, neither master or this branch have got any better or worse. branch master with Webrick: branch issue_185 with Webrick: branch master with Puma: branch issue_185 with Puma: |
|
@stevehook I've finally merged this. Thank you very much! 👍 |
|
thank you @jodosha and love the new project name by the way! |
…context Rename controller `context` setting to `default_context`
This is a potential fix for hanami/hanami#185
Partial templates are loaded on start-up and cached in the framework configuration object - Lotus::View.configuration and pulled from there by the PartialFinder at runtime.
I'm not too sure about the cache structure. It's a two-level hash at the moment, so that you can lookup the templates for a given path and then pick the one with the required format.
I have left the old implementation as a fallback for now. Are there any good reasons not to remove it completely (other than the fact that there are a few tests I would need to fix)?