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

Try reverting prefetch changes for project/version listing views #11621

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

agjohnson
Copy link
Contributor

@agjohnson agjohnson commented Sep 26, 2024

This comment was marked as off-topic.

@agjohnson
Copy link
Contributor Author

agjohnson commented Sep 26, 2024

I'm testing this and it's actually not solving the problem with query time. I'm still seeing 6s response time. This is still because of the prefetch query that we were previously using. Something seems like it regressed here and I'm guessing it probably solves the underlying problem with these changes above too.

@agjohnson
Copy link
Contributor Author

agjohnson commented Sep 26, 2024

Noted in chat, but it seems there is a missing foreign key on builds_build for project. I have this locally, but it is not in production:

    "builds_build_project_id_5c35b3fd_fk_projects_project_id" FOREIGN KEY (project_id) REFERENCES projects_project(id) DEFERRABLE INITIALLY DEFERRED

It's definitely not clear if this helps the planner or not though, this might be another dead end.

I would sooner merge in #11620 than this PR as it at least dropped the view time to 3s for my user.

@agjohnson
Copy link
Contributor Author

agjohnson commented Sep 26, 2024

With the revert of the additional prefetch not fixing the response time, I've disabled all prefetching on the dashboard view. If there are no other ideas on resolving the response timing issues here, we can revisit prefetching to reduce query load later.

Undoing the prefetch we've had in place for a while now, the response time is <1s and the query response time is instant:

In [1]: %time a = list(Project.objects.dashboard(User.objects.filter(is_staff=True).first()))
CPU times: user 11 ms, sys: 3.92 ms, total: 14.9 ms
Wall time: 66.3 ms

There are still 120 queries for each dashboard load however.

@agjohnson agjohnson marked this pull request as ready for review September 26, 2024 05:56
@agjohnson agjohnson requested a review from a team as a code owner September 26, 2024 05:56
Copy link
Member

@ericholscher ericholscher left a comment

Choose a reason for hiding this comment

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

Yea, I'm 👍 on rolling this back and thinking about some other approaches here, like perhaps the normal Django cache across processes or similar.

@agjohnson
Copy link
Contributor Author

agjohnson commented Sep 26, 2024

To be clear, this isn't just rolling back my changes. This wasn't enough to reduce response time (it's still 6s response time). This revert is removing all prefetching from the project dashboard view, added years ago here #5637. If there were performance benefits to these, maybe they got lost over time?

Caching is maybe on the table, I might also lean towards making this an explicit model field or relationship.

@ericholscher
Copy link
Member

Yea, denormalizing the latest build ID or similar onto the model is another really obvious answer 👍

@agjohnson agjohnson added the PR: hotfix Pull request applied as hotfix to release label Sep 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: hotfix Pull request applied as hotfix to release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants