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

Optimize api/v2/events/ N+1 #3586

Open
DeD1rk opened this issue Feb 6, 2024 · 5 comments · May be fixed by #3883
Open

Optimize api/v2/events/ N+1 #3586

DeD1rk opened this issue Feb 6, 2024 · 5 comments · May be fixed by #3883
Assignees
Labels
app:activemembers Issues regarding the activemembers-app app:events Issues regarding the events-app optimization Issues regarding slowness priority: medium A new feature or a bugfix that is non-critical.

Comments

@DeD1rk
Copy link
Member

DeD1rk commented Feb 6, 2024

What?

https://thalia.nu/api/v2/events/ is slow, probably at least partially because of https://thalia.sentry.io/issues/4580718277/. We should see if we can get rid of that N+1 issue (and possibly any other occurrences of the same thing.

Why?

More performance = more happy. It might help with high server load as well.

How?

Probably some prefetching or annotation.

@DeD1rk DeD1rk added priority: medium A new feature or a bugfix that is non-critical. app:activemembers Issues regarding the activemembers-app app:events Issues regarding the events-app optimization Issues regarding slowness labels Feb 6, 2024
@ColonelPhantom ColonelPhantom self-assigned this Oct 9, 2024
@ColonelPhantom
Copy link
Contributor

I'm having some difficulty reproducing this; I think it only happens in certain edge cases? And I don't remember the exact cases when it happens. Maybe it's related to needing to be in the queue? On Sentry it seems like the queries are related to activemembers.

@DeD1rk
Copy link
Member Author

DeD1rk commented Oct 9, 2024

The user may need to be in (organizing) committees?

@DeD1rk
Copy link
Member Author

DeD1rk commented Oct 9, 2024

I suspect it's something with gathering permissions that you get for being organizer

@ColonelPhantom
Copy link
Contributor

Yeah, probably. It's unfortunate that unlike DJDT, Sentry does not give a backtrace saying where the query originated.

@ColonelPhantom
Copy link
Contributor

I suspect it's something with gathering permissions that you get for being organizer

This is correct. I managed to get a backtrace by creating a non-superuser who is an active member:

/home/quinten/code/thalia/concrexit/website/events/api/v2/serializers/event.py in _user_permissions(110)
  return services.event_permissions(member, instance, registration_prefetch=True)

/home/quinten/code/thalia/concrexit/website/events/services.py in event_permissions(184)
  "manage_event": is_organiser(member, event),

/home/quinten/code/thalia/concrexit/website/events/services.py in is_organiser(278)
  .exists()

The duplicated query is quite ugly as it also involves a big JOIN. I already managed to get rid of most of it with the following code:

            organisers = set(event.organisers.all())
            member_groups = set(member.get_member_groups())
            return len(organisers.intersection(member_groups)) > 0

but I'm not sure that's optimal either, and as-is it re-retrieves the get_member_groups so that would need to be optimized too. For that, I'm not sure about the correct way to do so , since it is too complex to be trivially prefetchable. Maybe the ugly 'named prefetch' thing needs to be done again?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
app:activemembers Issues regarding the activemembers-app app:events Issues regarding the events-app optimization Issues regarding slowness priority: medium A new feature or a bugfix that is non-critical.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants