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.

Sign up for GitHub

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

Jump to bottom

Fix performance on instances list in admin UI #15282

Merged
merged 1 commit into from
Dec 14, 2020
Merged

Fix performance on instances list in admin UI #15282

merged 1 commit into from
Dec 14, 2020

Conversation

Gargron
Copy link
Member

@Gargron Gargron commented Dec 6, 2020 •
edited
Loading

  • Reduce duplicate queries
  • Remove n+1 queries
  • Add accounts count to detailed view
  • Add separate action log entry for updating existing domain blocks

This PR also adds a materialized view in PostgreSQL which simplifies queries and logic around getting a list of domains from the database by treating them as a read-only model.

@Gargron Gargron added the performance Runtime performance label Dec 6, 2020
@Gargron Gargron force-pushed the fix-admin-instances-performance branch 3 times, most recently from 5afdcc4 to 61af347 Compare December 6, 2020 23:16
@Gargron Gargron added the moderation Administration and moderation tooling label Dec 6, 2020
@Gargron Gargron force-pushed the fix-admin-instances-performance branch 5 times, most recently from 10506ae to c75d957 Compare December 7, 2020 04:13
@Gargron Gargron requested a review from ClearlyClaire December 8, 2020 21:45
ClearlyClaire
ClearlyClaire requested changes Dec 9, 2020
View reviewed changes
Copy link
Contributor

@ClearlyClaire ClearlyClaire left a comment

Choose a reason for hiding this comment

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

I appreciate the performance improvements here, but:

  • so far, instances are sorted by number of known accounts, which won't be the case anymore. Not sure how helpful that was, so it might be fine to drop this, but i'm not sure.
  • do we have to use SQL views here? I'm not fundamentally against that, but I'm unsure about adding another dependency for migrations
  • lib/mastodon/domains_cli.rb needs to be updated because Account#domains got removed

app/controllers/admin/domain_blocks_controller.rb Show resolved Hide resolved
app/controllers/api/v1/instances/peers_controller.rb Outdated Show resolved Hide resolved
app/models/instance.rb Outdated Show resolved Hide resolved
config/brakeman.ignore Show resolved Hide resolved
@Gargron
Copy link
Member Author

Gargron commented Dec 9, 2020

so far, instances are sorted by number of known accounts, which won't be the case anymore. Not sure how helpful that was, so it might be fine to drop this, but i'm not sure

Personally, I have never browsed the unfiltered list of instances since adding it. Certainly it did not matter to me how many accounts were at the top, it was just frustrating how slow the page loads.

do we have to use SQL views here? I'm not fundamentally against that, but I'm unsure about adding another dependency for migrations

The refactor hinges on SQL views simplifying all queries, allowing preloads etc. I think we might want to make more use of that part of PostgreSQL in the future.

@Gargron Gargron force-pushed the fix-admin-instances-performance branch from c75d957 to efedec5 Compare December 10, 2020 02:51
@Gargron Gargron force-pushed the fix-admin-instances-performance branch 4 times, most recently from fee93ef to 9d14291 Compare December 14, 2020 02:02
ClearlyClaire
ClearlyClaire requested changes Dec 14, 2020
View reviewed changes
Copy link
Contributor

@ClearlyClaire ClearlyClaire left a comment

Choose a reason for hiding this comment

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

Not actually tested yet, but apart from the remaining inlined comments it looks good to me now.

app/workers/scheduler/pghero_scheduler.rb Outdated Show resolved Hide resolved
app/models/instance.rb Outdated Show resolved Hide resolved
@Gargron Gargron force-pushed the fix-admin-instances-performance branch from 9d14291 to 7feb95d Compare December 14, 2020 07:14
@Gargron
Fix performance on instances list in admin UI
ff7c86c 
- Reduce duplicate queries
- Remove n+1 queries
- Add accounts count to detailed view
- Add separate action log entry for updating existing domain blocks
@Gargron Gargron force-pushed the fix-admin-instances-performance branch from 7feb95d to ff7c86c Compare December 14, 2020 07:15
@Gargron Gargron merged commit 216b85b into master Dec 14, 2020
@Gargron Gargron deleted the fix-admin-instances-performance branch December 14, 2020 08:07
@saper
Copy link
Contributor

saper commented Jan 10, 2021

Here is a report from the user seeing ERROR: materialized view "instances" has not been populated errors from PostgreSQL after upgrade:

https://discourse.joinmastodon.org/t/api-errors-after-upgrade-to-v3-3-0/3209/6?u=saper

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

@ClearlyClaire ClearlyClaire ClearlyClaire approved these changes

Assignees
No one assigned
Labels
moderation Administration and moderation tooling performance Runtime performance
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Gargron @saper @ClearlyClaire