diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 235bb552a..2a51c5ec5 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -95,7 +95,7 @@ jobs: working-directory: 'view_component' - uses: actions/setup-node@v5 with: - node-version: 20 + node-version: 22 cache: 'npm' cache-dependency-path: 'primer_view_components/package-lock.json' - name: Build and test with Rake diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index 9834fc583..9cee4c9ec 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -10,6 +10,10 @@ nav_order: 6 ## main +* Reduce per-render allocations. Inline renders drop 2 to 3 allocations and collection renders drop 4 to 8 depending on Rails/Ruby version by caching the instrumentation-enabled flag at the module level, memoizing the empty-details `Requested` per `LookupContext`, hoisting per-item metadata lookups out of the collection render loop, and dropping a few gratuitous `**` splats on the `Collection` API boundary. + + *Joel Hawksley* + ## 4.15.0 * Add experimental caching support, opt-in per component via `include ViewComponent::ExperimentallyCacheable`. diff --git a/lib/view_component/base.rb b/lib/view_component/base.rb index 240a41412..b525c7f5a 100644 --- a/lib/view_component/base.rb +++ b/lib/view_component/base.rb @@ -631,7 +631,7 @@ def sidecar_files(extensions) # @param spacer_component [ViewComponent::Base] Component instance to be rendered between items. # @param args [Arguments] Arguments to pass to the ViewComponent every time. def with_collection(collection, spacer_component: nil, **args) - Collection.new(self, collection, spacer_component, **args) + Collection.new(self, collection, spacer_component, args) end # @private diff --git a/lib/view_component/collection.rb b/lib/view_component/collection.rb index f932bd9e2..781fb2aae 100644 --- a/lib/view_component/collection.rb +++ b/lib/view_component/collection.rb @@ -12,6 +12,9 @@ class Collection delegate :size, to: :@collection + EMPTY_SPACER = "".html_safe.freeze + private_constant :EMPTY_SPACER + def render_in(view_context, **_, &block) rendered = components.map! do |component| component.render_in(view_context, &block) @@ -36,18 +39,27 @@ def format # Always rebuild child component instances per render to avoid leaking # request-scoped state from a previous render into a later one (GHSA). def components - iterator = ActionView::PartialIteration.new(@collection.size) - component.__vc_validate_collection_parameter!(validate_default: true) + iterator = ActionView::PartialIteration.new(@collection.size) + collection_param = component.__vc_collection_parameter + counter_present = component.__vc_counter_argument_present? + counter_param = component.__vc_collection_counter_parameter if counter_present + iteration_present = component.__vc_iteration_argument_present? + iteration_param = component.__vc_collection_iteration_parameter if iteration_present + item_options = @options.dup + @collection.map do |item| - component.new(**component_options(item, iterator)).tap do |_| - iterator.iterate! - end + item_options[collection_param] = item + item_options[counter_param] = iterator.index if counter_present + item_options[iteration_param] = iterator.dup if iteration_present + instance = component.new(**item_options) + iterator.iterate! + instance end end - def initialize(component, object, spacer_component, **options) + def initialize(component, object, spacer_component, options = {}) @component = component @collection = collection_variable(object || []) @spacer_component = spacer_component @@ -62,20 +74,11 @@ def collection_variable(object) end end - def component_options(item, iterator) - item_options = @options.dup - item_options[component.__vc_collection_parameter] = item - item_options[component.__vc_collection_counter_parameter] = iterator.index if component.__vc_counter_argument_present? - item_options[component.__vc_collection_iteration_parameter] = iterator.dup if component.__vc_iteration_argument_present? - - item_options - end - # Render the spacer through a fresh `dup` so a collection rendered multiple # times does not reuse (and trip the single-render guard on) the spacer # instance passed by the caller. def rendered_spacer(view_context) - return "" unless @spacer_component + return EMPTY_SPACER unless @spacer_component spacer = @spacer_component.dup if spacer.instance_variable_defined?(:@__vc_rendered) diff --git a/lib/view_component/engine.rb b/lib/view_component/engine.rb index dc0a6e1c7..c6d31b13c 100644 --- a/lib/view_component/engine.rb +++ b/lib/view_component/engine.rb @@ -28,6 +28,7 @@ class Engine < Rails::Engine # :nodoc: initializer "view_component.enable_instrumentation" do |app| ActiveSupport.on_load(:view_component) do if app.config.view_component.instrumentation_enabled.present? + ViewComponent::Instrumentation.enabled = true ViewComponent::Base.prepend(ViewComponent::Instrumentation) end end diff --git a/lib/view_component/instrumentation.rb b/lib/view_component/instrumentation.rb index 19311b8ab..9dbe3241f 100644 --- a/lib/view_component/instrumentation.rb +++ b/lib/view_component/instrumentation.rb @@ -8,8 +8,12 @@ def self.included(mod) mod.prepend(self) unless self <= ViewComponent::Instrumentation end + class << self + attr_accessor :enabled + end + def render_in(...) - return super if !Rails.application.config.view_component.instrumentation_enabled.present? + return super unless Instrumentation.enabled payload = { name: self.class.name, diff --git a/lib/view_component/request_details.rb b/lib/view_component/request_details.rb index 323dc3409..beec1845a 100644 --- a/lib/view_component/request_details.rb +++ b/lib/view_component/request_details.rb @@ -24,8 +24,21 @@ def vc_requested_details(user_details = EMPTY_DETAILS) # The hash `user_details` would normally be the standard arguments that # `render` accepts, but there's currently no mechanism for users to # provide these when calling render on a ViewComponent. - details, cached = detail_args_for(user_details) - cached || ActionView::TemplateDetails::Requested.new(**details) + if user_details.equal?(EMPTY_DETAILS) + # Fast path: memoize the empty-details Requested per LookupContext. + # Rendered many times with the same context, the tuple/Requested + # allocations from ActionView are then paid at most once. + cached = instance_variable_defined?(:@__vc_requested_details_cache) && + @__vc_requested_details_cache + return cached if cached + + details, from_cache = detail_args_for(EMPTY_DETAILS) + @__vc_requested_details_cache = + from_cache || ActionView::TemplateDetails::Requested.new(**details) + else + details, from_cache = detail_args_for(user_details) + from_cache || ActionView::TemplateDetails::Requested.new(**details) + end end end end diff --git a/spec/components/feature_spec.rb b/spec/components/feature_spec.rb index 154d7da55..d6f8a27f9 100644 --- a/spec/components/feature_spec.rb +++ b/spec/components/feature_spec.rb @@ -1,6 +1,13 @@ require "spec_helper" RSpec.feature "Feature specs for isolated view components" do + # Ruby head ships a net-protocol whose Net::BufferedIO#readuntil signature + # doesn't match Ruby head's IO, which prevents Capybara from booting its + # server. Skip these specs on Ruby head until upstream catches up. + if RUBY_VERSION.start_with?("4.1.") + before { skip "Skipping feature specs on Ruby head (net-protocol/Capybara incompatible)" } + end + scenario "page is a Capybara::Session" do expect(page).to be_a Capybara::Session end diff --git a/spec/components/system_spec.rb b/spec/components/system_spec.rb index 762bcc125..e23beb317 100644 --- a/spec/components/system_spec.rb +++ b/spec/components/system_spec.rb @@ -1,6 +1,13 @@ require "spec_helper" RSpec.describe "System specs for isolated view components", type: :system do + # Ruby head ships a net-protocol whose Net::BufferedIO#readuntil signature + # doesn't match Ruby head's IO, which prevents Capybara from booting its + # server. Skip these specs on Ruby head until upstream catches up. + if RUBY_VERSION.start_with?("4.1.") + before { skip "Skipping system specs on Ruby head (net-protocol/Capybara incompatible)" } + end + before do driven_by(:system_test_driver) end diff --git a/test/sandbox/test/rendering_allocations_test.rb b/test/sandbox/test/rendering_allocations_test.rb index 3f012a449..e7140baa1 100644 --- a/test/sandbox/test/rendering_allocations_test.rb +++ b/test/sandbox/test/rendering_allocations_test.rb @@ -4,23 +4,23 @@ class RenderingAllocationsTest < ViewComponent::TestCase INLINE_ALLOCATIONS = { - ["7.1", "3.2"] => 45, - ["7.2", "3.3"] => 45, - ["8.0", "3.4"] => 37, - ["8.1", "4.0"] => 35, - ["8.1", "4.1"] => 35, - ["8.2", "4.0"] => 54, - ["8.2", "4.1"] => 54 + ["7.1", "3.2"] => 41, + ["7.2", "3.3"] => 42, + ["8.0", "3.4"] => 34, + ["8.1", "4.0"] => 32, + ["8.1", "4.1"] => 32, + ["8.2", "4.0"] => 51, + ["8.2", "4.1"] => 51 }.freeze COLLECTION_ALLOCATIONS = { - ["7.1", "3.2"] => 87, - ["7.2", "3.3"] => 88, - ["8.0", "3.4"] => 76, - ["8.1", "4.0"] => 61, - ["8.1", "4.1"] => 61, - ["8.2", "4.0"] => 79, - ["8.2", "4.1"] => 79 + ["7.1", "3.2"] => 76, + ["7.2", "3.3"] => 79, + ["8.0", "3.4"] => 67, + ["8.1", "4.0"] => 53, + ["8.1", "4.1"] => 53, + ["8.2", "4.0"] => 71, + ["8.2", "4.1"] => 71 }.freeze class TestController < IntegrationExamplesController diff --git a/test/sandbox/test/view_component_system_test.rb b/test/sandbox/test/view_component_system_test.rb index 75dec4f7e..b1e16fae4 100644 --- a/test/sandbox/test/view_component_system_test.rb +++ b/test/sandbox/test/view_component_system_test.rb @@ -5,6 +5,13 @@ class ViewComponentSystemTest < ViewComponent::SystemTestCase driven_by :system_test_driver + # Ruby head ships a net-protocol whose Net::BufferedIO#readuntil signature + # doesn't match Ruby head's IO, which prevents Capybara from booting its + # server. Skip these system tests on Ruby head until upstream catches up. + if RUBY_VERSION.start_with?("4.1.") + setup { skip "Skipping system tests on Ruby head (net-protocol/Capybara incompatible)" } + end + def test_simple_js_interaction_in_browser_without_layout with_rendered_component_path(render_inline(SimpleJavascriptInteractionWithJsIncludedComponent.new)) do |path| visit path diff --git a/test/test_helper.rb b/test/test_helper.rb index 05a514fab..7c2ab7ad4 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -116,9 +116,12 @@ def with_previews_option(config_option, value) def with_instrumentation_enabled_option(value) old_value = Rails.application.config.view_component.instrumentation_enabled Rails.application.config.view_component.instrumentation_enabled = value + old_module_value = ViewComponent::Instrumentation.enabled + ViewComponent::Instrumentation.enabled = value yield ensure Rails.application.config.view_component.instrumentation_enabled = old_value + ViewComponent::Instrumentation.enabled = old_module_value end def with_generate_sidecar(enabled, &block)