Improves caching lookup performance #773
Merged
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.
I've been tracking down some performance issues in our codebase and found a somewhat extreme example where this block of code is eating up around 5 seconds of compute time.
The circumstances are:
Rabl.configuration.view_paths
has over 500 paths in itextends
, and they nest even furtherEven with caching enabled, it still takes nearly 5 seconds to generate all the cache keys as each
extends
allocates an array with 500+ elements and then concatenates it to generate the cache key. In this flame graph you can seeRabl::source_cache
takes a total of 4.84 seconds in this trace and it's all down toArray#join
This change removes the
Rabl.configuration.view_paths
from the path passed in tosource_cache
so that only the customview_path
(if specified) is considered for part of the key. As long asRabl.configuration.view_paths
is only configured once at application start up and not modified on the fly, this should ultimately result in the same caching key behavior but with a lot less info needed to generate each key.After this change the same request spends just 35 ms total executing
Rabl.source_cache
.All existing tests passed, I wasn't sure how else to test this if further tests are needed.