-
Notifications
You must be signed in to change notification settings - Fork 0
Optimize internal extract!
calls to save on memory allocation
#7
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
extract!
to save on memory allocationextract!
calls to save on memory allocation
lib/jbuilder.rb
Outdated
def _extract_hash_values(object, attributes) | ||
attributes.each{ |key| _set_value key, _format_keys(object.fetch(key)) } | ||
attributes.each{ |key| _set_value key, _format_keys(object[key]) } |
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.
Also using []
instead of fetch
. It's a bit faster.
ruby 3.4.4 (2025-05-14 revision a38531fd3f) +YJIT +PRISM [arm64-darwin24]
Warming up --------------------------------------
[] 1.582M i/100ms
fetch 1.627M i/100ms
Calculating -------------------------------------
[] 26.015M (± 4.2%) i/s (38.44 ns/i) - 129.730M in 4.999980s
fetch 21.967M (± 4.2%) i/s (45.52 ns/i) - 110.641M in 5.047802s
Comparison:
[]: 26014972.6 i/s
fetch: 21966990.7 i/s - 1.18x slower
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.
Is the behaviour identical on missing keys?
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.
Good question. fetch
actually throws if the key doesn't exist, vs returning nil
. Not clear if this is the intended behaviour or just happenstance. The coding style for the project seems to prefer fetch
. I git blamed back 10+ years to see if there was any reasoning for this, but saw nothing of note before losing interest.
Mixed feelings on this. The current implementation would result in 500 errors if/when this occurs, which is not something I would expect or desire from our tooling. But given my intent here was simply performance - not a behavioural change - I will switch this back to fetch
; the overall savings still fall within the margin of error (according to ips) and not worth the risk and drift from the upstream project.
lib/jbuilder.rb
Outdated
def _extract_hash_values(object, attributes) | ||
attributes.each{ |key| _set_value key, _format_keys(object.fetch(key)) } | ||
attributes.each{ |key| _set_value key, _format_keys(object[key]) } |
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.
Is the behaviour identical on missing keys?
Simply optimizes internal calls to
extract!
to avoid extra memory allocations to splat the*attributes
. Both_extract_hash_values
and_extract_method_values
accept an array forattributes
, so no need to re-splat it by callingextract!
, which then splats it again.Solution is to implement a private
_extract
that just accepts an array, which can be used internally.A simple way to compare it is with the
call
DSL, which was just runningextract!
underneath.This change has no impact in using
json.extract!
directly.Filed upstream: rails#598