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 5 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
35 changes: 22 additions & 13 deletions lib/jbuilder/jbuilder_template.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,8 @@ def array!(collection = [], *args)
options = args.first

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

@rafaelfranca rafaelfranca Jul 18, 2025

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.

partial! options
else
super
end
Expand All @@ -137,14 +138,14 @@ def set!(name, object = BLANK, *args)
private

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) && CollectionRenderer.supported?
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 Down Expand Up @@ -173,9 +174,10 @@ def _render_partial_with_options(options)
locals = options.delete(:locals)
array! collection do |member|
member_locals = locals.clone
member_locals.merge! collection: collection
member_locals.merge! as => member
_render_partial options.merge(locals: member_locals)
member_locals[:collection] = collection
member_locals[as] = member
options[:locals] = member_locals
_render_partial options
end
else
array!
Expand All @@ -186,8 +188,8 @@ def _render_partial_with_options(options)
end

def _render_partial(options)
options[:locals].merge! json: self
@context.render options
options[:locals][:json] = self
@context.render options, nil
Copy link
Contributor Author

@moberegger moberegger May 12, 2025

Choose a reason for hiding this comment

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

The render helper in rails will default the second parameter to {}. By providing nil here we save on that extra memory allocation.

That second parameter is intended to be the options you provide to the partial if the first param is the partial name (ex: render 'foo', options). Since the partial name is included in the options, that second parameter isn't actually used.

Copy link
Member

@rafaelfranca rafaelfranca Jul 18, 2025

Choose a reason for hiding this comment

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

This is the kind of micro optimization that is unnecessary. Let's just not pass the argument.

end

def _cache_fragment_for(key, options, &block)
Expand Down Expand Up @@ -242,13 +244,19 @@ def _set_inline_partial(name, object, options)
value = if object.nil?
[]
elsif _is_collection?(object)
_scope{ _render_partial_with_options options.merge(collection: object) }
_scope do
options[:collection] = object
Copy link
Member

@rafaelfranca rafaelfranca Jul 18, 2025

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

@rafaelfranca rafaelfranca Jul 21, 2025

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
locals = ::Hash[options[:as], object]
_scope{ _render_partial_with_options options.merge(locals: locals) }
_scope do
Copy link
Member

@rafaelfranca rafaelfranca Jul 18, 2025

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] = locals
_render_partial_with_options options
end
end

set! name, value
_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_explicit_partial(name_or_options, locals = {})
Expand All @@ -259,7 +267,8 @@ def _render_explicit_partial(name_or_options, locals = {})
else
# partial! 'name', locals: {foo: 'bar'}
if locals.one? && (locals.keys.first == :locals)
options = locals.merge(partial: name_or_options)
locals[:partial] = name_or_options
Copy link
Member

@rafaelfranca rafaelfranca Jul 18, 2025

Choose a reason for hiding this comment

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

Mutating the argument. We should avoid it.

Copy link
Contributor Author

@moberegger moberegger Jul 21, 2025

Choose a reason for hiding this comment

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

This one I don't understand. locals is already being mutated in this method with the current logic. Further down below, a locals.delete(:as) is run, and this can happen directly against the locals argument.

Copy link
Member

@rafaelfranca rafaelfranca Jul 21, 2025

Choose a reason for hiding this comment

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

This might be a bug already. If we do:

locals = { something: 1}
partial! "something", locals
partial! "something_else, locals

The call to the second partial should not use the arguments for the first one. That delete should be removed, or we should create a copy of the arguments when entering this method.

Copy link
Contributor Author

@moberegger moberegger Jul 21, 2025

Choose a reason for hiding this comment

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

Ah ok. This was one of the reasons why I thought there were no issues mutating the options since it was technically being done already. Didn't consider that this may not have been deliberate.

I'll try to address this, too.

options = locals
else
options = { partial: name_or_options, locals: locals }
end
Expand Down