Skip to content
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

Refactor compiler to use ActionView's partial sorting algorithm #2158

Open
wants to merge 1 commit into
base: v4
Choose a base branch
from

Conversation

sfnelson
Copy link

@sfnelson sfnelson commented Nov 6, 2024

What are you trying to accomplish?

Rails uses ActionView::TemplateDetails::Requested to sort available templates to find the best match for a given LookupContext, taking in to account the acceptable content types, variants, locale, and handlers available. ViewComponent implements a subset of this logic that leaves some gaps, i.e. #2154 #2128

This PR aims to bring the ViewComponent template selection logic in line with how Rails chooses a partial, and ensure that it won't drift in the future by using Rails' internal structures for the calculation.

What approach did you choose and why?

  1. I'm using refine to provide access to Rails' internal @details ivar from LookupContext that memoizes details about the rendering context for use in partial/template resolution. Using the Rails internal logic for template resolution ensures that ViewComponents will resolve templates in a consistent way and remain consistent in the future.
  2. I'm using Procs instead of methods to compile render_template_for. This approach allows closing over the templates available to the component, which would not be possible with a compiled method body as a string. The benefit of this approach is that the code is easy to read and maintain, and it's equivalent or slightly faster to render components. An alternative approach would be to generate constants that hold the data required for the template selection and refer to them in the method body, but I think the code would be harder to read and maintain.

I have run the included benchmarks and the CPU performance is marginally better vs v4. See attached.

v4.benchmark.txt
2128-variant.benchmark.txt

The number of allocated objects per render is slightly better than v4 because it's using the LookupContext cached resolve structures instead of storing its own format and variants. There is some additional memory retained per component as the Template objects remain in scope after compilation, but the Template objects are small and the overhead is proportional to the number of component templates, it won't grow during application lifecycle.

My team has added this code to one of our larger projects and it will go to production in the next launch. I'll provide feedback, if any, once we have it.

Anything you want to highlight for special attention from reviewers?

I intend that this change has minimal impact on the observable API of ViewComponents. The benefits would be better alignment with Rails' behaviour, in line with the goal of seamless integration.

I've discussed this approach with @joelhawksley but I'm keen for feedback from other members of the community.

@sfnelson sfnelson marked this pull request as draft November 6, 2024 02:29
@sfnelson sfnelson force-pushed the 2128-variant branch 5 times, most recently from 4b3679e to 5e13764 Compare November 8, 2024 04:42
@sfnelson sfnelson marked this pull request as ready for review November 8, 2024 05:05
@sfnelson
Copy link
Author

sfnelson commented Nov 8, 2024

@joelhawksley great to see you've merged my previous PR. I've updated this one, and I think it's ready for review. I discovered the reason why I was seeing more allocated objects and I've fixed the issue (I wasn't using the ActionView::LookupContext cache correctly).

#
# @private
def format
@__vc_variant if defined?(@__vc_variant)
nil
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've spent some time investigating why this code returned the variant and I still don't really understand why. It was added during 1.x and it's been refactored numerous times. I don't think this has any meaningful contribution to caching anymore as Rails cache already uses the LookupContext@details ivar as an entry point to caching. Any cache generated from a ViewComponent render would already be keyed by the specific LookupContext details including locale, variants, formats, and handlers.

@@ -328,7 +330,7 @@ def content_evaluated?
end

def maybe_escape_html(text)
return text if __vc_request && !__vc_request.format.html?
return text if @current_template && !@current_template.html?
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that current_template is set by render_template_for now, so this logic takes into account the format picked by the renderer (e.g. if the chosen template is json then the before/after strings won't be HTML sanitised).

@reeganviljoen
Copy link
Collaborator

reeganviljoen commented Nov 19, 2024

@joelhawksley how does this look for v4

@sfnelson
Copy link
Author

We deployed this branch to production yesterday. No perceivable difference in memory utilisation or CPU since deployment. I'll continue monitoring and report any issues we encounter (@joelhawksley as discussed).

@reeganviljoen
Copy link
Collaborator

reeganviljoen commented Nov 21, 2024

@joelhawksley any further reservationd or ideas to move this forward

@sfnelson
Copy link
Author

@joelhawksley could you please give this PR a review?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants