-
Notifications
You must be signed in to change notification settings - Fork 442
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.
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
Optimize memory allocation when rendering partials #591
Conversation
end | ||
|
||
set! name, value | ||
_set_value name, value |
There was a problem hiding this comment.
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
.
lib/jbuilder/jbuilder_template.rb
Outdated
options[:locals].merge! json: self | ||
@context.render options | ||
options[:locals][:json] = self | ||
@context.render options, nil |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
options
merging@@ -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 |
There was a problem hiding this comment.
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.
lib/jbuilder/jbuilder_template.rb
Outdated
options[:locals].merge! json: self | ||
@context.render options | ||
options[:locals][:json] = self | ||
@context.render options, nil |
There was a problem hiding this comment.
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.
@@ -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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
else | ||
locals = ::Hash[options[:as], object] | ||
_scope{ _render_partial_with_options options.merge(locals: locals) } | ||
_scope do |
There was a problem hiding this comment.
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
lib/jbuilder/jbuilder_template.rb
Outdated
@@ -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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The options hash should no longer be mutating, and where it was originally doing it in _render_explicit_partial
should no longer be a problem.
partial! options.merge(collection: collection) | ||
options = options.dup | ||
options[:collection] = collection | ||
_render_partial_with_options options |
There was a problem hiding this comment.
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.
_set_inline_partial name, object, options | ||
_set_inline_partial name, object, options.dup |
There was a problem hiding this comment.
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.
_render_explicit_partial(*args) | ||
options = args.extract_options!.dup | ||
options[:partial] = args.first if args.present? | ||
_render_partial_with_options options |
There was a problem hiding this comment.
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
.
Amazing work! |
We're seeing calls to
reverse_merge!
,merge!
, andmerge
fromJbuilderTemplate
come up as CPU and memory hot spots in our profiles.The changes proposed in this PR are inspired by https://github.com/fastruby/fast-ruby#hashmerge-vs-hash-code, and favours mutating the
options
hash via element assignment over merge methods. This saves on both CPU and memory allocation.Comparing
options[:locals].merge!(json: self)
tooptions[:locals][:json] = self
for example produced:This PR replaces all instances of
reverse_merge!
with[] ||=
, and all instances ofmerge!
with[]=
. Theoptions
were already being mutated so this introduces no change in behaviour.There are a handful of non-mutating calls to
merge
as well that I was hesitant to change, but upon further analysis theoptions
hash ends up being mutated further down the call chain anyways; any instance of theoptions
hash being merged are on code paths that render to partials which already mutate the options.I've run some benchmarks against something simple yet representative of a template structure that would exercise some of the changes being proposed.
The measurements below are for 100 posts, each with a single author.
CPU
Memory
I was surprised to see no difference in IPS given the earlier benchmarks, but that can be explained by
actionview
diluting it; this benchmark includes the entirerender
lifecycle which means that my code changes are only running a couple hundred times per second.The impactful improvements is the ~20% reduction in memory. Note that the memory allocation savings would depend entirely on your template - templates rendering to fewer or no partials would see less of an improvement, templates rendering to more partials could see a much larger improvement. As your API serves requests over time, this improvement would go a long way towards saving on garbage collection cycles.