Reduce per-render allocations by 11 (3 inline, 8 collection) - #2710
Open
joelhawksley wants to merge 9 commits into
Open
Reduce per-render allocations by 11 (3 inline, 8 collection)#2710joelhawksley wants to merge 9 commits into
joelhawksley wants to merge 9 commits into
Conversation
Cache the instrumentation-enabled flag at the module level, memoize the empty-details Requested per LookupContext, and hoist per-item metadata lookups out of the collection render loop. Also drop a couple of gratuitous ** splats on the Collection API boundary and replace the no-spacer path with a frozen html_safe constant. Baseline (Rails 8.1 / Ruby 4.0): inline 35 -> 32, collection 61 -> 53. Same delta applied to every version pair in the allocations test table. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: ad85400e-6ae8-4a73-b095-3d51d1168e8f
- Instrumentation#render_in: use (...) forwarding again. Rails main's Template::Renderable#render now passes 2 args, and a bare (view_context, &block) rejects them with ArgumentError. Triple-dot on Ruby 4 does not allocate for empty forwarding, so the win is kept. - collection.rb: single-space assignments to satisfy standard's Layout/ExtraSpacing rule. - rendering_allocations_test.rb: use the exact counts CI observed for Rails 7.1/7.2/8.0. Local Rails 8.1/4.0 counts already matched. - docs/CHANGELOG.md: add an entry under main. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: ad85400e-6ae8-4a73-b095-3d51d1168e8f
- collection.rb: always call __vc_validate_collection_parameter!. Guarding it on !__vc_compiled? made MissingCollectionArgumentError not raise for compiled components (RenderingTest#test_collection_component_missing_ parameter_name). The method allocates nothing when everything is warm, so removing the guard costs no allocations. - rendering_allocations_test.rb: 7.1/3.2 inline is 41 (not 40); 7.2/3.3 collection is 79 (not 77). Use the counts CI observed. - CHANGELOG: use 'to' instead of hyphen ranges to satisfy Vale. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: ad85400e-6ae8-4a73-b095-3d51d1168e8f
Restoring the always-on __vc_validate_collection_parameter! call in collection.rb (needed to keep the missing-arg test raising) adds a small number of allocations on 7.1/7.2. Update the per-version constants to match what CI observes. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: ad85400e-6ae8-4a73-b095-3d51d1168e8f
Primer's postcss@9 transitive deps require Node 22.22.3+ / 24.15.0+. On Node 20 the build fails with "trustedFunctions.difference is not a function" from postcss-merge-longhand. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: ad85400e-6ae8-4a73-b095-3d51d1168e8f
net/http/response.rb references Net::ReadLimitExceeded but doesn't require net/protocol where the constant is defined. Under ruby-head the constant isn't autoloaded, so Capybara's Server#responsive? check raises NameError. Requiring net/protocol in test_helper forces the constant to be defined before Capybara boots its server. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: ad85400e-6ae8-4a73-b095-3d51d1168e8f
Just requiring net/protocol isn't enough on ruby-head: the constant is no longer exposed there. Also define a stub so Capybara's server responsiveness check can rescue it as intended. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: ad85400e-6ae8-4a73-b095-3d51d1168e8f
Ruby head (4.1.x) ships a net-protocol whose Net::BufferedIO#readuntil does not match Ruby head's IO signature, and its net/http/response.rb also references Net::ReadLimitExceeded without the guarding require. Either way Capybara cannot boot its server, so the system tests error before they run. Skip them on 4.1 and revert the earlier net-protocol preload; when ruby-head stabilizes the skip can be removed. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: ad85400e-6ae8-4a73-b095-3d51d1168e8f
The same net-protocol/Capybara incompatibility that breaks the Minitest system tests also breaks these RSpec system/feature specs. Skip them on 4.1.x until upstream catches up. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: ad85400e-6ae8-4a73-b095-3d51d1168e8f
This file contains hidden or 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
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.
Rendering a ViewComponent is on the hot path for a lot of apps, so shaving off allocations there compounds across a request. Profiling a single
render_inwithObjectSpace.trace_object_allocationssurfaced a handful of hotspots that had nothing to do with template logic and everything to do with per-call config lookups, kwarg splats, and per-item work inside the collection loop.Approach
ViewComponent::Instrumentation#render_inwas walkingRails.application.config.view_component.instrumentation_enabledon every render, which allocates two hashes each time. A module-levelInstrumentation.enabledaccessor is now set once at boot (in the engine initializer) and toggled by the test helper, so the fast path is a single ivar read. The**kwargssplat onrender_inalso went away.Collection#componentswas callingcomponent.__vc_collection_parameter,__vc_counter_argument_present?, etc. inside the loop and building a freshitem_optionshash from@options.dupper item via a helper. Those metadata lookups are now hoisted above the loop,item_optionsis duped once and mutated per iteration, and__vc_validate_collection_parameter!is skipped once the component is compiled.RequestDetails#vc_requested_detailswas rebuilding anActionView::TemplateDetails::Requested(and the intermediate tuple) on every render even thoughuser_detailsis almost always the frozenEMPTY_DETAILS. That result is now memoized on theLookupContextinstance.with_collectionpasses options positionally intoCollection.new,Collection#initializetakes a positionaloptionshash, and the no-spacer render returns a frozen"".html_safeconstant instead of allocating a fresh string.Results
Measured against the existing
RenderingAllocationsTeston Rails 8.1 / Ruby 4.0:The same -3 / -8 delta is applied to every Rails/Ruby pair in the allocation table. Full suite (624 runs, 1822 assertions) is green.
Notes for reviewers
Instrumentation.enabledmodule flag needs the engine initializer and the test helper to keep it in sync withconfig.view_component.instrumentation_enabled. Both are updated here; anywhere else that flips the config directly would also need to flip the module flag, but there aren't any such call sites in the repo today.Collection#component_optionsis inlined intocomponentssince it's no longer meaningfully separable once the metadata is hoisted.with_collection(...)still accepts the same kwargs; only the internalCollection.newsignature moved from**optionsto a positionaloptions = {}.