Skip to content
This repository

kaminari disastrous with very large num_pages #137

Closed
jrochkind opened this Issue July 13, 2011 · 13 comments

8 participants

Jonathan Rochkind Akira Matsuda Fredrik Björk Jacob Hollenbeck Chris Beer Eduardo Mourao Matteo Parmi Jeff Shantz
Jonathan Rochkind

Let's say you have a page size of 10, and the total number of hits is 3 million. That would mean 300k hypothetical pages.

https://github.com/amatsuda/kaminari/blob/master/app/views/kaminari/_paginator.html.erb

calls #each_page (https://github.com/amatsuda/kaminari/blob/master/lib/kaminari/helpers/paginator.rb#L34), which will iterate through each of these 300k hypothetical pages, AND create a PageProxy for each of them. Even though only a handful of them will actually be shown.

This is disastrous for performance. Creating those 300k PageProxy objects (plus any subsidiary String and other such little objects they reference as state) is a horrible problem for ruby Garbage Collection alone, not to mention possible CPU implications.

This took me a LONG while to figure out what was going wrong with my app that included kaminari, giving it pathological performance. If a clever way can't be found to deal with this, then at the very least I'd strongly suggest a prominent warning is placed in Kaminari README saying that you should not use it with a large number of total pages in a response. Unless I'm misunderstanding the problem, but I think this is it.

Jonathan Rochkind

Using memprof to verify what seemed true in the code. With a per_page of 10 and a total count of 3,095,569 , kaminari indeed creates hundreds of thousands of objects in #paginate. (Actually over a million objects total, counting all classes)

 418328 /home/rochkind/.rvm/gems/ree-1.8.7-2011.03/gems/kaminari-0.12.4/lib/kaminari/helpers/paginator.rb:37:__varmap__
 209164 /home/rochkind/.rvm/gems/ree-1.8.7-2011.03/gems/kaminari-0.12.4/lib/kaminari/helpers/paginator.rb:79:Array
 209164 /home/rochkind/.rvm/gems/ree-1.8.7-2011.03/gems/kaminari-0.12.4/lib/kaminari/helpers/paginator.rb:38:Kaminari::Helpers::Paginator::PageProxy
 209164 /home/rochkind/.rvm/gems/ree-1.8.7-2011.03/gems/kaminari-0.12.4/lib/kaminari/helpers/paginator.rb:38:Hash

No surprise that this leads to disastrous performance.

(Not quite sure what the __varmap__ thing is, some kind of anonymous class? Not sure why it memprof logs 209164 PageProxy's created, instead of what I'd expect from a quick read of the code, total_pages (~300k), but at any rate it's the same order of magnitude, and clearly problematic)

Fredrik Björk
fbjork commented July 22, 2011

Any fix coming for this?

Jonathan Rochkind

I too am interested with the committers think. Here's a really ugly hacky workaround/fix you can implement in your own theme, by colleague cbeer: https://gist.github.com/b7590aed87ea7671da63

If there's any indication this is still a maintained project and the maintainers would be interested in a fix, I could try to find time to work out a pull request with a better fix (add an #each_relevant_page instead (better name welcome) of just #each_page, and make the default theme call that.) Not entirely sure when I'd get to it though, definitely no motivation without any feedback from owner/committer that they'd even be open to such a pull request.

Jacob Hollenbeck
grioja commented July 25, 2011

+1 to this

Chris Beer cbeer referenced this issue from a commit July 27, 2011
address issue #137 by adding #each_relevant_page, an iterator that on…
…ly visits pages in the inner or outer windows, and Kaminari::Helpers::Paginator::Windows (should be renamed) that does the heavy lifting
ab8a450
Chris Beer
cbeer commented July 27, 2011

I've added a patch in my fork that is a start to dealing with this issue properly. From the commit message:

address issue #137 by adding #each_relevant_page, an iterator that only visits pages in the inner or outer windows, and Kaminari::Helpers::Paginator::Windows (should be renamed) that does the heavy lifting

The solution probably could and should be improved, but hopefully the core logic is useful and can be adapted appropriately.

Jonathan Rochkind

Is Kaminari a maintained project? I'm going to start assuming it's not if we don't hear anything from owner/committers?

Eduardo Mourao

Yes, it is. This is conf-season, let's just email them folks. :D @a_matsuda

Jacob Hollenbeck

+1

Matteo Parmi

1

Jonathan Rochkind

tried contacting kaminari author through github a couple weeks ago, still haven't heard back. A bit frustrating that we can't get any feedback on this, even when offering a pull request.

For now, we are monkey patching kaminari in our app to fix.

Akira Matsuda
Owner

I'm sorry for leaving this issue open for half a year, but I finally pulled @cbeer's patch as I realized that the patch certainly solves the performance problem.
The fix will be included in the next 0.13.0 gem coming soon.
Thank you all for your attention.

Akira Matsuda amatsuda closed this December 11, 2011
Jonathan Rochkind
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.