-
-
Notifications
You must be signed in to change notification settings - Fork 83
Performance: Cache config #276
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
base: main
Are you sure you want to change the base?
Changes from all commits
e0b89ac
e86197b
31fb0c4
4c064ed
26e7020
43ab174
3c59899
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -540,12 +540,6 @@ def self.scope(scope_class = nil, &block) | |||||
|
|
||||||
| # @!endgroup | ||||||
|
|
||||||
| # @api private | ||||||
| # @since 2.1.0 | ||||||
| def self.layout_path(layout) | ||||||
| File.join(*[config.layouts_dir, layout].compact) | ||||||
| end | ||||||
|
|
||||||
| # @api private | ||||||
| # @since 2.1.0 | ||||||
| def self.cache | ||||||
|
|
@@ -563,6 +557,7 @@ def initialize | |||||
| self.class.config.finalize! | ||||||
| ensure_config | ||||||
|
|
||||||
| @config_data = config.to_data | ||||||
| @exposures = self.class.exposures.bind(self) | ||||||
| end | ||||||
|
|
||||||
|
|
@@ -595,16 +590,20 @@ def exposures # rubocop:disable Style/TrivialAccessors | |||||
| # | ||||||
| # @api public | ||||||
| # @since 2.1.0 | ||||||
| def call(format: config.default_format, context: config.default_context, layout: config.layout, **input) | ||||||
| def call(format: config_data.default_format, | ||||||
| context: config_data.default_context, | ||||||
| layout: config_data.layout, | ||||||
| **input) | ||||||
| rendering = self.rendering(format: format, context: context) | ||||||
| scope_class = config_data.scope | ||||||
|
|
||||||
| locals = locals(rendering, input) | ||||||
| output = rendering.template(config.template, rendering.scope(config.scope, locals)) | ||||||
| output = rendering.template(config_data.template, rendering.scope(scope_class, locals)) | ||||||
|
|
||||||
| if layout | ||||||
| output = rendering.template( | ||||||
| self.class.layout_path(layout), | ||||||
| rendering.scope(config.scope, layout_locals(locals)) | ||||||
| File.join(*[config_data.layouts_dir, layout].compact), | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we leave the layout_path method back as it was? I think it makes this code here (which is already doing a lot!) easier to follow. One fewer piece of detailed implementation inlined into the method. |
||||||
| rendering.scope(scope_class, layout_locals(locals)) | ||||||
| ) { output } | ||||||
| end | ||||||
|
|
||||||
|
|
@@ -613,20 +612,26 @@ def call(format: config.default_format, context: config.default_context, layout: | |||||
|
|
||||||
| # @api private | ||||||
| # @since 2.1.0 | ||||||
| def rendering(format: config.default_format, context: config.default_context) | ||||||
| Rendering.new(config: config, format: format, context: context) | ||||||
| def rendering(format: config_data.default_format, context: config_data.default_context) | ||||||
| Rendering.new(config_data:, format:, context:) | ||||||
| end | ||||||
|
|
||||||
| private | ||||||
|
|
||||||
| # Frozen Data snapshot of the view's resolved configuration values. | ||||||
| # Used for fast hot-path reads during rendering. | ||||||
| # | ||||||
| # @since 2.3.0 | ||||||
|
Comment on lines
+623
to
+624
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Another since we can drop. This is not just "@api private", it's fully private :) |
||||||
| attr_reader :config_data | ||||||
|
|
||||||
| def ensure_config | ||||||
| raise UndefinedConfigError, :paths unless Array(config.paths).any? | ||||||
| raise UndefinedConfigError, :template unless config.template | ||||||
| end | ||||||
|
|
||||||
| def locals(rendering, input) | ||||||
| exposures.(context: rendering.context, **input) do |value, exposure| | ||||||
| if exposure.decorate?(default: config.decorate_exposures) && value | ||||||
| if exposure.decorate?(default: config_data.decorate_exposures) && value | ||||||
| rendering.part(exposure.name, value, as: exposure.options[:as]) | ||||||
| else | ||||||
| value | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,12 +21,8 @@ class Renderer | |
|
|
||
| # @api private | ||
| # @since 2.1.0 | ||
| attr_reader :config, :prefixes | ||
|
|
||
| # @api private | ||
| # @since 2.1.0 | ||
| def initialize(config) | ||
| @config = config | ||
| def initialize(config_data) | ||
| @config_data = config_data | ||
| @prefixes = [CURRENT_PATH_PREFIX] | ||
| end | ||
|
|
||
|
|
@@ -37,7 +33,7 @@ def template(name, format, scope, &block) | |
|
|
||
| template_path = lookup(name, format) | ||
|
|
||
| raise TemplateNotFoundError.new(name, format, config.paths) unless template_path | ||
| raise TemplateNotFoundError.new(name, format, config_data.paths) unless template_path | ||
|
|
||
| new_prefix = File.dirname(name) | ||
| @prefixes << new_prefix unless @prefixes.include?(new_prefix) | ||
|
|
@@ -55,10 +51,12 @@ def partial(name, format, scope, &block) | |
|
|
||
| private | ||
|
|
||
| attr_reader :config_data, :prefixes | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just a note on why these were public before: I've never really cared about leaving public attr readers on objects, even if they're not formally "public API". It's Ruby, so if anyone really wanted to get to some object, there's always a way they can do it, so I never really tried to fight it. This is not to say I'm opposed to this change, but I thought I'd just share some of the rationale I held when putting these things together in the first place :) Happy for this change to go in as part of this PR, anyway :) |
||
|
|
||
| def lookup(name, format) | ||
| View.cache.fetch_or_store(:lookup, name, format, config, prefixes) { | ||
| View.cache.fetch_or_store(:lookup, name, format, config_data.object_id, prefixes) { | ||
| catch :found do | ||
| config.paths.reduce(nil) do |_, path| | ||
| config_data.paths.reduce(nil) do |_, path| | ||
| prefixes.reduce(nil) do |_, prefix| | ||
| result = path.lookup(prefix, name, format) | ||
| throw :found, result if result | ||
|
|
@@ -79,8 +77,8 @@ def render(path, scope, &block) | |
| end | ||
|
|
||
| def tilt(path) | ||
| View.cache.fetch_or_store(:tilt, path, config) { | ||
| Hanami::View::Tilt[path, config.renderer_engine_mapping, config.renderer_options] | ||
| View.cache.fetch_or_store(:tilt, path, config_data.object_id) { | ||
| Hanami::View::Tilt[path, config_data.renderer_engine_mapping, config_data.renderer_options] | ||
| } | ||
| end | ||
| end | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -7,28 +7,43 @@ class View | |||||
| class Rendering | ||||||
| # @api private | ||||||
| # @since 2.1.0 | ||||||
| attr_reader :config, :format | ||||||
| attr_reader :format | ||||||
|
|
||||||
| # @api private | ||||||
| # @since 2.1.0 | ||||||
| attr_reader :inflector, :part_builder, :scope_builder | ||||||
|
|
||||||
| # @api private | ||||||
| # @since 2.3.0 | ||||||
| attr_reader :part_class, :part_namespace, :scope_class, :scope_namespace | ||||||
|
|
||||||
| # Stable identity for the underlying config snapshot, suitable as a cache-key component. | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Since this is already called |
||||||
| # | ||||||
| # @api private | ||||||
| # @since 2.3.0 | ||||||
|
Comment on lines
+22
to
+23
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's stop putting I need to make a post about this, but in the meantime I'm trying to stop it proliferating, and removing the tags whenever I edit a file.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (Also, for any "since" that is legitimately needed here — now for "@api public" methods only! — it should be ... though I think most of your changes are to private API, AFAICT |
||||||
| attr_reader :cache_key | ||||||
|
|
||||||
| # @api private | ||||||
| # @since 2.1.0 | ||||||
| attr_reader :context, :renderer | ||||||
| attr_reader :context | ||||||
|
|
||||||
| # @api private | ||||||
| # @since 2.1.0 | ||||||
| def initialize(config:, format:, context:) | ||||||
| @config = config | ||||||
| def initialize(config_data:, format:, context:) | ||||||
| @format = format | ||||||
|
|
||||||
| @inflector = config.inflector | ||||||
| @part_builder = config.part_builder | ||||||
| @scope_builder = config.scope_builder | ||||||
| @inflector = config_data.inflector | ||||||
| @part_builder = config_data.part_builder | ||||||
| @scope_builder = config_data.scope_builder | ||||||
|
|
||||||
| @part_class = config_data.part_class | ||||||
| @part_namespace = config_data.part_namespace | ||||||
| @scope_class = config_data.scope_class | ||||||
| @scope_namespace = config_data.scope_namespace | ||||||
| @cache_key = config_data.object_id | ||||||
|
|
||||||
| @context = context.dup_for_rendering(self) | ||||||
| @renderer = Renderer.new(config) | ||||||
| @renderer = Renderer.new(config_data) | ||||||
| end | ||||||
|
|
||||||
| # @api private | ||||||
|
|
@@ -54,6 +69,10 @@ def part(name, value, as: nil) | |||||
| def scope(name = nil, locals) # rubocop:disable Style/OptionalArguments | ||||||
| scope_builder.(name, locals: locals, rendering: self) | ||||||
| end | ||||||
|
|
||||||
| private | ||||||
|
|
||||||
| attr_reader :renderer | ||||||
|
Comment on lines
+72
to
+75
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I mean, this is probably why I don't like private attr_readers. As a maintainer looking at this class, I see a bunch at the top of the file, and then we have this random one hanging out all the way at the bottom. It's disjointed and it's hard to get a good overview of the class this way. Maybe we could just leave them at the top, since attr privacy is not really the purpose of this PR? |
||||||
| end | ||||||
| end | ||||||
| end | ||||||
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.
Are these not strictly needed for this PR?