Skip to content

Optimize memory allocation when rendering partials #591

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.

Sign up for GitHub

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

Merged
merged 11 commits into from
Jul 29, 2025
Merged

Optimize memory allocation when rendering partials #591

Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
86b3946
Optimize options merging
moberegger May 8, 2025
866d289
Call _set_value directly
moberegger May 9, 2025
c6b9bff
Put template_lookup_options to avoid possible breaking change
moberegger May 12, 2025
ba3a627
Save on more merges
moberegger May 12, 2025
9ca4d05
Save memory allocation when calling render
moberegger May 12, 2025
1245776
Merge branch 'main' into moberegger/optimize_options_merges
moberegger Jul 21, 2025
8f9993a
Stop mutating options in array! method
moberegger Jul 21, 2025
8474b41
Remove _partial micro-optimization
moberegger Jul 21, 2025
7e16adf
Stop mutating options in set! method
moberegger Jul 23, 2025
b7b5abb
Stop mutating options in partial! method
moberegger Jul 23, 2025
6fd6c06
Small _set_inline_partial optimization
moberegger Jul 23, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 19 additions & 31 deletions lib/jbuilder/jbuilder_template.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,9 @@ def partial!(*args)
if args.one? && _is_active_model?(args.first)
_render_active_model_partial args.first
else
_render_explicit_partial(*args)
options = args.extract_options!.dup
options[:partial] = args.first if args.present?
_render_partial_with_options options
Comment on lines -58 to +60
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was able to refactor this in such a way that we no longer require _render_explicit_partial. That method was actually already mutating options, either directly via as = locals.delete(:as), or later on when it called _render_partial_with_options.

This new setup prevents that from happening and also saves on a bunch of work in _render_explicit_partial, which was basically doing things that were already handled in _render_partial_with_options.

end
end

Expand Down Expand Up @@ -121,7 +123,9 @@ def array!(collection = [], *args)
options = args.first

if args.one? && _partial_options?(options)
partial! options.merge(collection: collection)
options = options.dup
options[:collection] = collection
Copy link
Member

Choose a reason for hiding this comment

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

This is now mutating the arguments of the method. We should not do it.

_render_partial_with_options options
Comment on lines -124 to +128
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Prevents mutating options on array! in a way that allows us to safely mutate the options in private methods to keep the other optimizations in place.

else
super
end
Expand All @@ -131,7 +135,7 @@ def set!(name, object = BLANK, *args)
options = args.first

if args.one? && _partial_options?(options)
_set_inline_partial name, object, options
_set_inline_partial name, object, options.dup
Comment on lines -134 to +138
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Likewise here, but for set!. We can now safely mutate in _set_inline_partial to keep the other optimizations.

else
super
end
Expand All @@ -142,14 +146,14 @@ def set!(name, object = BLANK, *args)
alias_method :method_missing, :set!

def _render_partial_with_options(options)
options.reverse_merge! locals: options.except(:partial, :as, :collection, :cached)
options.reverse_merge! ::JbuilderTemplate.template_lookup_options
options[:locals] ||= options.except(:partial, :as, :collection, :cached)
options[:handlers] ||= ::JbuilderTemplate.template_lookup_options[:handlers]
as = options[:as]

if as && options.key?(:collection)
collection = options.delete(:collection) || []
partial = options.delete(:partial)
options[:locals].merge!(json: self)
options[:locals][:json] = self
collection = EnumerableCompat.new(collection) if collection.respond_to?(:count) && !collection.respond_to?(:size)

if options.has_key?(:layout)
Expand All @@ -175,7 +179,7 @@ def _render_partial_with_options(options)
end

def _render_partial(options)
options[:locals].merge! json: self
options[:locals][:json] = self
@context.render options
end

Expand Down Expand Up @@ -231,34 +235,18 @@ def _set_inline_partial(name, object, options)
value = if object.nil?
[]
elsif _is_collection?(object)
_scope{ _render_partial_with_options options.merge(collection: object) }
else
locals = ::Hash[options[:as], object]
_scope{ _render_partial_with_options options.merge(locals: locals) }
end

set! name, value
end

def _render_explicit_partial(name_or_options, locals = {})
case name_or_options
when ::Hash
# partial! partial: 'name', foo: 'bar'
options = name_or_options
_scope do
options[:collection] = object
Copy link
Member

Choose a reason for hiding this comment

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

This is now mutating the argument of the method. We should not do it

Copy link
Member

Choose a reason for hiding this comment

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

I realize this is a private method, so this might be ok, but we should make sure we aren't mutating arguments on public methods.

Copy link
Contributor Author

@moberegger moberegger Jul 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

In this case, _set_inline_partial is called in one spot, which is set!. Even though this method is private, it would still effectively mutate the options hash provided to set!.

Is whether or not it's ok to mutate just contingent on public vs private methods? Is this simply convention?

What are we trying to guard against here? Given the documented DSL with something like json.foo partial: 'foo', foo: my_foo, the key args would result in a new Hash object that is safe to mutate without impacting the call site, no? Are we trying to avoid side effects if someone instead does something like

my_options = { partial: 'foo', foo: my_foo }
json.foo my_options

Or perhaps if/when my_options is the result of an action view helper or something like that?

_render_partial_with_options options
end
else
# partial! 'name', locals: {foo: 'bar'}
if locals.one? && (locals.keys.first == :locals)
options = locals.merge(partial: name_or_options)
else
options = { partial: name_or_options, locals: locals }
_scope do
Copy link
Member

Choose a reason for hiding this comment

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

Same here. Argument is being mutated

options[:locals] = { options[:as] => object }
_render_partial_with_options options
end
# partial! 'name', foo: 'bar'
as = locals.delete(:as)
options[:as] = as if as.present?
options[:collection] = locals[:collection] if locals.key?(:collection)
end

_render_partial_with_options options
_set_value name, value
Copy link
Contributor Author

@moberegger moberegger May 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

We can set the value directly here instead of going back in through set! with different options. A call to set! with these parameters will just end up calling _set_value anyways.

This saves a bit in processing and also avoids an extra memory allocation for *args.

end

def _render_active_model_partial(object)
Expand Down