Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Significant speed increase for large collections #15

Merged
merged 1 commit into from Mar 24, 2013
Merged

Significant speed increase for large collections #15

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 2 additions & 8 deletions lib/multi_fetch_fragments.rb
View file
Open in desktop
Expand Up @@ -21,9 +21,6 @@ def render_collection_with_multi_fetch_cache
additional_cache_options = @options.fetch(:cache_options, {})
keys_to_collection_map = {}

# clone the original collection so we can manipulate it without affecting the original
@collection = @collection.clone

@collection.each do |item|
key = @options[:cache].is_a?(Proc) ? @options[:cache].call(item) : item

Expand All @@ -46,11 +43,8 @@ def render_collection_with_multi_fetch_cache

# if we had a cached value, we don't need to render that object from the collection.
# if it wasn't cached, we need to render those objects as before
result_hash.each do |key, value|
if value
collection_item = keys_to_collection_map[key]
@collection.delete(collection_item)
end
@collection = (keys_to_collection_map.keys - result_hash.keys).map do |key|
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might want to follow the result_hash assignment a couple lines up with .delete_if { |k, v| v.nil? } to preserve the check for value not being nil.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could not find any cases where value actually is nil, unless you've explicitly set it to that value. If the key is not found in the cache store, it is simply not included in the hash.

You would think that the original if value check is there for a reason though. But i can't find it. Do you have any idea why it's there?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't seen any nils with Dalli either, but there's a lot of cache stores out there, and I don't know how all of them behave.

It was already there with the first commit (1eff047), so @n8 is probably the only one who knows.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@n8 Do you have any comments on this?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, i'm not seeing why I had this behavior this way. It doesn't seem like Daali, or Rails default cache store returns nils for missing cache entries. I think we can remove it.

keys_to_collection_map[key]
end

non_cached_results = []
Expand Down