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

Use followers counter cache when available #6383

Merged
merged 1 commit into from
Aug 13, 2020

Conversation

sauloperez
Copy link
Contributor

@sauloperez sauloperez commented Aug 6, 2020

🎩 What? Why?

Investigating proposals' performance together with @mrcasals we found out that a considerable amount of the time is spent rendering coauthorships (all that complexity comes at a cost). Among that, a big issue was an N+1 fetching the authors' followers.

We thought a counter cache had to be added but it turns out it already existed 🎉 ! However, Proposals::Proposal doesn't implement it and thus, we need to provide a fallback for now. It can be added in a separate PR (I'm not sure I'll get it done before going on holidays)

In my machine, this gets from

Screenshot from 2020-08-06 09-18-41

to

Screenshot from 2020-08-06 09-17-08

I assume the impact will be noticeable in a production env more than we see here ☝️

📌 Related Issues

  • Related to #? Proposals performance
  • Fixes #?

📋 Subtasks

  • Add CHANGELOG upgrade notes, if required
  • If there's a new public field, add it to GraphQL API
  • Add documentation regarding the feature
  • Add/modify seeds
  • Add tests
  • Another subtask

📷 Screenshots (optional)

Description

The proposal model does not have a `followers_count` column so we need
to provide a fallback. Other models avoid an N+1.

Then, we need to find out why proposals don't have followers
counter-cached. If they did, we would avoid another N+1.
@mrcasals
Copy link
Contributor

@decidim/core can you review this, please? It improves the performance of the FollowButton cell, thus improving the performance of all indexs pages rendering it.

Thanks!

@mrcasals mrcasals requested a review from a team August 10, 2020 10:11
@Leusev Leusev self-requested a review August 12, 2020 14:25
@Leusev Leusev self-assigned this Aug 12, 2020
Copy link
Contributor

@agustibr agustibr left a comment

Choose a reason for hiding this comment

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

👏 Simple and effective

Copy link
Contributor

@tramuntanal tramuntanal left a comment

Choose a reason for hiding this comment

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

Good catch!

I approve but would like to get rid of followers_count method would you implement proposal.followers_count counter cache please? 🙏

@mrcasals
Copy link
Contributor

I approve but would like to get rid of followers_count method would you implement proposal.followers_count counter cache please? 🙏

@tramuntanal can we do that in a future PR, please? @sauloperez is on holidays and we don't have enough people to take over this PR right now, and this might end up forgotten :(

@Leusev
Copy link
Contributor

Leusev commented Aug 13, 2020

Yes @mrcasals , I think it wouldn't be a problem. By the moment, I accept this PR and we'll wait for another PR later with the correction requested by @tramuntanal

Copy link
Contributor

@Leusev Leusev left a comment

Choose a reason for hiding this comment

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

Good catch @sauloperez 👍

@Leusev Leusev merged commit 32d6f73 into decidim:develop Aug 13, 2020
Quentinchampenois pushed a commit to Quentinchampenois/decidim that referenced this pull request Aug 14, 2020
The proposal model does not have a `followers_count` column so we need
to provide a fallback. Other models avoid an N+1.

Then, we need to find out why proposals don't have followers
counter-cached. If they did, we would avoid another N+1.
@sauloperez sauloperez deleted the use-followers-count-in-proposals branch August 24, 2020 07:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants