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

Revert friends-of-friends follow recommendation query to using a CTE #29619

Merged
merged 1 commit into from Mar 18, 2024

Conversation

ClearlyClaire
Copy link
Contributor

@ClearlyClaire ClearlyClaire commented Mar 15, 2024

Also fixes ordering back to the initial intent.

Note that I have not verified whether this had better performances than the previous version. I also used subqueries instead of the usual approach of using Account.not_excluded_by_account and Account.not_domain_blocked_by_account because:

  1. we never actually verified if that approach had better performance
  2. it's a lot more painful to use without ActiveRecord helpers

Also fixes ordering back to the initial intent
@ClearlyClaire ClearlyClaire added performance Runtime performance build-image Build a container image for this PR labels Mar 15, 2024
@mjankowski
Copy link
Contributor

On the ordering change here -- was this an oversight we missed in review? Or was the intention to change the ordering, but due to the perf issues we're going back to original ordering?

@ClearlyClaire
Copy link
Contributor Author

The change of ordering in my earlier PR was an oversight of mind, I misunderstood the original ordering, and there were no tests to catch the change. I just noticed the ordering had been changed by starting from the old CTE.

@mjankowski
Copy link
Contributor

Cool - given that, I agree that the spec changes here do recapture what the original pre-refactor ordering looked like. I'll defer to you all on the query change / perf implications / etc.

  • Do we want to fix specs and the refactor version (swap the order) first here? just so we have a baseline of a logically correct change? (happy to do this as quick separate PR if you'd like)
  • Other than playing around with local data setup and reviewing some explain/analyze combinations ... is there any way we can benchmark this better?

Copy link
Sponsor Member

@renchap renchap left a comment

Choose a reason for hiding this comment

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

We tested the queries on mastodon.social and perf is similar to the previous code

@ClearlyClaire ClearlyClaire added this pull request to the merge queue Mar 18, 2024
Merged via the queue into main with commit d506307 Mar 18, 2024
53 checks passed
@ClearlyClaire ClearlyClaire deleted the revert-cte branch March 18, 2024 13:02
@ClearlyClaire
Copy link
Contributor Author

We tested the queries on mastodon.social and perf is similar to the previous code

To add to that, we tested multiple versions of the query for multiple sets of active mastodon.social users (random 400 users, 5 with very few follows, 5 with large amounts of follows and blocks) and the results were always pretty consistent: the code in this PR is roughly on par with the old query that was missing filtering, while the code before this PR was way slower (10 to 50 times slower). The code in main...mjankowski:mastodon:fix-order-in-floof-foof-fooferoni-changes was consistently ~1.5× slower than that of this PR, which could be due to some extra work instead of reusing the CTE for filtering.

  • Other than playing around with local data setup and reviewing some explain/analyze combinations ... is there any way we can benchmark this better?

This is a good question. I guess understanding the query plan is the way to go, but even that is specific to what's actually in the database. Also, extracting the query is a bit of a pain depending on how the query is written.

I've been toying with populating fake data, but I think we'd need to take shortcuts in generation for it to be usable.

@mjankowski
Copy link
Contributor

One thing I suspect may have been happening with the poorly performing query is that, for the setup of first_degree, when I was reviewing the changes and thinking about what was being built - I was thinking about that aspect specifically as first grabbing a big array of IDs (which, in itself could have been expensive) before the larger query was built, but what was actually happening is that query is getting included as a subquery in the larger query, which could certainly get expensive depending on the surrounding rows/joins/etc. That said, more than one thing changed at once, so it may be multiple causes.

Before I attempt this -- are we open to repeating the style/compositional improvement of these changes, if we can preserve the current more performant query? (putting aside like, framework sql-quoting style and whatnot). That current WIP branch (that you linked) restores the composed scope approach - but adds an additional join which is not present in this restored/performant version. I suspect that contributes to the ~1.5x perf drop on that branch.

lutoma pushed a commit to ohaisocial/mastodon that referenced this pull request Mar 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build-image Build a container image for this PR performance Runtime performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants