-
Notifications
You must be signed in to change notification settings - Fork 0
Optimize set! DSL
#15
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
Conversation
ec328ef to
5f5c7cb
Compare
b6f2b3e to
e2a3866
Compare
| if _blank?(value) | ||
| # json.comments { ... } | ||
| # { "comments": ... } | ||
| _merge_block(key) { yield self } | ||
| else | ||
| # json.comments @post.comments { |comment| ... } | ||
| # { "comments": [ { ... }, { ... } ] } | ||
| _scope { _array(value) { |element| yield element } } | ||
| end |
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 swapped the if condition here. It originally used to be
if !_blank?(value)
# json.comments @post.comments { |comment| ... }
# { "comments": [ { ... }, { ... } ] }
_scope { _array(value) { |element| yield element } }
else
# json.comments { ... }
# { "comments": ... }
_merge_block(key) { yield self }
endbut it is slightly faster to not negate the original check.
| locals = ::Hash[options[:as], object] | ||
| _scope do | ||
| options[:locals] = locals | ||
| options[:locals] = { options[:as] => 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 was a slightly faster way to assign the hash.
| elsif ::Kernel.block_given? | ||
| _set(name, object, args) { |x| yield x } | ||
| else | ||
| super | ||
| _set(name, object, args) | ||
| end |
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 looks strange, but is the only way I found to avoid having a &block parameter.
An alternative was
elsif ::Kernel.block_given?
super
else
_set(name, object, args)
endbut this would incur an extra memory allocation for *args via the super call whenever a block was provided to set!.
mscrivo
left a comment
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.
🚀
Similar to #14, but for the
set!DSL. Many of the changes are similar:one?. This shows up as a hotspot for us, and it isn't actually needed. The intent here was to check if a single argument was provided toset!to determine whether or not a partial should be rendered. It's actually just faster to check if the first argument is aHashthan it is to call out toone?first, because the latter is an O(n) operation.Jbuilder::set!viasuper, which splats*argsinto an allocatedArrayeach time. The PR introduces_setto be used internally, which saves on this allocation. This is similar to what was done in Optimize internalextract!calls to save on memory allocation #7 forextract!.One difference with #14 that is worth noting are how blocks work. Calls to
::Kernel.block_given?do show up as a hotspot in some of our profiles, which can be optimized away with a simplerif blockstyle check. This makes sense given that methods were already receiving a&blockparameter. While iteratively benchmarking this branch, I learned that simply having a&blockparameter incurs overhead, regardless of whether or not a block is provided:&blockin the method introduced extra latency even thoughblockwould evaluate tonil. I'm unsure of the reason why, but I presume Ruby has to do some extra work here to determine what the value of&blockshould be.&blockwould also result in extra memory allocation when converting the block to aProc. While the number of allocations would be the same, theProcresulted in an increase in memsizeUltimately it was cheaper to keep
&blockout of the method signatures and determine behaviour via::Kernel.block_given?. I can revisit the work done in #14 and do the same.I will annotate some other points of interest below.
Benchmarks for the affected DSLs are below. We see an improvement in all of them!!! (I will commit these with the PR soon).