Optimize key formatter #597
Open
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Makes a few optimizations to the
Jbuilder::KeyFormatter
integration:*args
and**kwargs
to handle the provided format symbols. This saves on having to compute a list of formatters from the providedoptions
, and saves on some memory allocations for the empty[]
arrays that represented "no parameters". While this is definitely a micro-optimization (ie. it would only be a hot code path of a template itself usesjson.key_format!
) it does make things easier to read..each
over.inject
when computing cache keys, which is slightly faster.Jbuilder
orJbuilderTemplate
with nooptions
, which is what the template handler does (json||=JbuilderTemplate.new(self)
). No need to allocation an emptyHash
for this.Jbuilder
initialization by using key arguments instead offetch
when grabbing values fromoptions
key_format!
andself.key_format
by simply passing args through toKeyFormatter.new
. This is a micro-optimization, but can be a hotter code path if a template usesjson.key_format!
.Now the big change here is that the key formatter cache is now no longer clobbered between template renders. What was originally happening was that when you configured a global key formatter with something like
Jbuilder.key_format = camelize: :lower
, it would be.clone
d whenever a newJbuilder
was initialized, which happens before each template render. When it was cloned, the cache would be wiped it whenKeyFormatter#initialize_copy
ran. This meant that each template render - and thus each API request - would start with a fresh cache. This meant that you would have to re-pay the cost to format the keys all over again.I'm not sure why it was done this way. When configuring a global key formatter for Jbuilder, I would expect that to be used across requests so that the cost to generate the keys could amortize as the service runs. This behaviour was originally added twelve years ago, specifically in this commit, but it's not clear to me what the intent or motivation was to do that.
The other difference is that a
Mutex
has been added to the cache lookup to keep it thread safe. This allows us to have a single shared cache perKeyFormatter
. The previous implementation was thread safe because each render would have its own instance of the cache (and perhaps that's why the cache would be a freshHash
wheneverKeyFormatter
wasclone
d), and the addition of theMutex
respects that behaviour.