Significant speed increase for large collections #15

Merged
merged 1 commit into from Mar 24, 2013

Projects

None yet

3 participants

@yoricksijsling
Contributor

This little loop was causing performance issues for me.

For a collection of 1517 items. With an empty cache, render time was about 1800ms. When the cache was filled, render time was 4000ms! With this change, i brought it down to less than 40ms.

I suspect that the .delete call is the cause of this. The searching and removing of the element is both O(n). This issue might also have been solved by using a Set for the collection, but i haven't tried that.

@DouweM DouweM commented on the diff Mar 12, 2013
lib/multi_fetch_fragments.rb
@@ -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|
@DouweM
DouweM Mar 12, 2013

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.

@yoricksijsling
yoricksijsling Mar 12, 2013 Contributor

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?

@DouweM
DouweM Mar 12, 2013

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.

@yoricksijsling
yoricksijsling Mar 18, 2013 Contributor

@n8 Do you have any comments on this?

@n8
n8 Mar 23, 2013 Owner

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.

@n8
Owner
n8 commented Mar 23, 2013

Sorry I've been slacking on this. Life has overloaded me with other support requests :) I'm looking at these pull requests now.

@n8 n8 merged commit 02783f3 into n8:master Mar 24, 2013
@yoricksijsling
Contributor

Thanks @n8 ! Now I can switch to your version of the gem again 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment