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.

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

Replace will_paginate with kaminari #1807

Closed
wants to merge 3 commits into from

Conversation

3 participants
@sonalkr132
Copy link
Member

sonalkr132 commented Oct 21, 2018

COUNT(DISTINCT ..) used in pagination for page number links is very expensive
in this case. This change will only show next and prev page link.

response time of /gems/rails/reverse_dependencies (tested locally)
before: 0m1.564s
after: 0m0.708s

Related: #1798

@sonalkr132 sonalkr132 force-pushed the sonalkr132:reverse-dep-perf branch from 96c6276 to 7123995 Oct 21, 2018

@dwradcliffe
Copy link
Member

dwradcliffe left a comment

Seems like a good change 👍

@dwradcliffe
Copy link
Member

dwradcliffe left a comment

Seems like a good change 👍

@dwradcliffe
Copy link
Member

dwradcliffe left a comment

Seems like a good change 👍

@dwradcliffe
Copy link
Member

dwradcliffe left a comment

Seems like a good change 👍

@@ -10,8 +10,10 @@
<div class="reverse__dependencies">
<%= render @reverse_dependencies %>
</div>

<%= will_paginate @reverse_dependencies %>
<nav class="pagination">

This comment has been minimized.

Copy link
@simi

simi Oct 22, 2018

Contributor

What about to move this to partial/helper (or combination of both) ?

@dwradcliffe
Copy link
Member

dwradcliffe left a comment

Seems like a good change 👍

@dwradcliffe

This comment has been minimized.

Copy link
Member

dwradcliffe commented Oct 22, 2018

🤣 I guess this is super approved since I tried to approve during the GitHub outage...

sonalkr132 added some commits Oct 21, 2018

Replace will_paginate with kaminari
We need option to avoid COUNT(DISTINCT ..) used to calculate
total_entries or total pages.
will_paginate doesn't support it.
Remove pagination page links from reverse deps
COUNT(DISTINCT ..) used in pagination for page number links is very expensive
in this case. This change will only show `next` and `prev` page link.

change of includes to preload:
kaminari overrides load method to load one extra record, which is used
to check if there is a need for next page. `includes` was returning
only per_page (30) entries instead of expected per_page + 1.
In hindsight, preload also takes load off main query which finds all
dependant gems and should have been used from start.

response time of /gems/rails/reverse_dependencies (tested locally)
before: 0m1.564s
after: 0m0.708s

@sonalkr132 sonalkr132 force-pushed the sonalkr132:reverse-dep-perf branch from 7123995 to 0e019e0 Oct 22, 2018

@sonalkr132

This comment has been minimized.

Copy link
Member Author

sonalkr132 commented Oct 22, 2018

@simi updated!

@@ -51,4 +51,8 @@ def search_form_class
def active?(path)
"is-active" if request.path_info == path
end

def without_count_paginate(items)

This comment has been minimized.

Copy link
@simi

simi Oct 22, 2018

Contributor

I think paginate_without_count looks better. But both describes actual implementation detail and not the expected output of this helper (having pagination missing numbered steps -> prev/next links only). What about some more generic name? Maybe simple_paginate (and leave a comment about performance in here)?

@sonalkr132 sonalkr132 force-pushed the sonalkr132:reverse-dep-perf branch from 0e019e0 to 761eded Oct 23, 2018

@sonalkr132

This comment has been minimized.

Copy link
Member Author

sonalkr132 commented Oct 23, 2018

What about some more generic name? Maybe simple_paginate (and leave a comment about performance in here)?

Used plain_paginate and added comment.

plain
/pleɪn/Submit
adjective

  1. not decorated or elaborate; simple or basic in character.
    "good plain food"
    synonyms: | simple, ordinary, unadorned, undecorated, unembellished

pun-dog-seo-expert

@sonalkr132

This comment has been minimized.

Copy link
Member Author

sonalkr132 commented Oct 23, 2018

Merged locally, had to update vendor.

@sonalkr132 sonalkr132 closed this Oct 23, 2018

@sonalkr132 sonalkr132 deleted the sonalkr132:reverse-dep-perf branch Oct 23, 2018

@simi

This comment has been minimized.

Copy link
Contributor

simi commented Oct 23, 2018

Great @sonalkr132!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.