kaminari disastrous with very large num_pages #137

Closed
jrochkind opened this Issue Jul 13, 2011 · 14 comments

Comments

Projects
None yet
9 participants
@jrochkind

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.

@jrochkind

This comment has been minimized.

Show comment
Hide comment
@jrochkind

jrochkind Jul 14, 2011

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)

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)

@fbjork

This comment has been minimized.

Show comment
Hide comment
@fbjork

fbjork Jul 22, 2011

Any fix coming for this?

fbjork commented Jul 22, 2011

Any fix coming for this?

@jrochkind

This comment has been minimized.

Show comment
Hide comment
@jrochkind

jrochkind Jul 22, 2011

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.

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.

@grioja

This comment has been minimized.

Show comment
Hide comment
@grioja

grioja Jul 25, 2011

+1 to this

grioja commented Jul 25, 2011

+1 to this

@cbeer

This comment has been minimized.

Show comment
Hide comment
@cbeer

cbeer Jul 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.

cbeer commented Jul 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.

@jrochkind

This comment has been minimized.

Show comment
Hide comment
@jrochkind

jrochkind Jul 27, 2011

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

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

@eduardordm

This comment has been minimized.

Show comment
Hide comment
@eduardordm

eduardordm Jul 28, 2011

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

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

@grioja

This comment has been minimized.

Show comment
Hide comment
@grioja

grioja Aug 20, 2011

+1

grioja commented Aug 20, 2011

+1

@tejo

This comment has been minimized.

Show comment
Hide comment
@tejo

tejo Sep 4, 2011

1

tejo commented Sep 4, 2011

1

@jrochkind

This comment has been minimized.

Show comment
Hide comment
@jrochkind

jrochkind Sep 28, 2011

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.

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.

@jeffshantz

This comment has been minimized.

Show comment
Hide comment
@jeffshantz

jeffshantz Oct 2, 2011

+1

+1

@amatsuda

This comment has been minimized.

Show comment
Hide comment
@amatsuda

amatsuda Dec 11, 2011

Member

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.

Member

amatsuda commented Dec 11, 2011

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.

@amatsuda amatsuda closed this Dec 11, 2011

@jrochkind

This comment has been minimized.

Show comment
Hide comment
@jrochkind

jrochkind Dec 11, 2011

Thanks!

On Dec 11, 2011, at 10:10 AM, Akira Matsuda wrote:

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.


Reply to this email directly or view it on GitHub:
amatsuda#137 (comment)

Thanks!

On Dec 11, 2011, at 10:10 AM, Akira Matsuda wrote:

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.


Reply to this email directly or view it on GitHub:
amatsuda#137 (comment)

@zeeshangulzar

This comment has been minimized.

Show comment
Hide comment
@zeeshangulzar

zeeshangulzar Jun 29, 2015

I am still getting this issue at 0.16.1

I am still getting this issue at 0.16.1

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