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

Many handlers perf #3305

Merged
merged 19 commits into from
May 24, 2024
Merged

Many handlers perf #3305

merged 19 commits into from
May 24, 2024

Conversation

Macroz
Copy link
Collaborator

@Macroz Macroz commented May 21, 2024

Tries to fix the many handlers perf problems from #3283

There's two changes here of which one is more subtle.

The original logic from years ago was such that all users rights were
updated whenever there was any change. This was fast for a while.

Last year, updates were implemented to only touch users that were
involved in the applications of the events. This was much faster.
There is however a flaw in this version.

Consider a case where one application was changed but there are 100
potential handlers in the organization. All the rights of these 100
handlers were updated by considering all their applications. Handlers
tend to be members of all applications so that could be 100 times the
application count of updates, e.g. 300K for 3000 applications. This
adds up to a few hundred milliseconds, which is too slow for us now,
,specially after adding more performance test data (10 handlers in
production so 100 handlers in performance test data).

We can avoid this amount of changes by recording how many times you have a
role instead of only whether you have the role. Then we remove the
role once per member of the old application and add 1 per member of new
application. This will total 100 changes for 100 users in one
application, which is as good as it can be. We could also record which
application gives which role, but that is slightly more memory
consuming for no additional gain.

The other subtler change is to cache only ids of applications that a person
has, not the personalized applications. 100 handlers with 3000
applications could result in 300K applications being cached.
Clojure's persistent data structures do help us here and in practice
that would not happen. However, we can avoid this by doing the
personalization in the functions that return results instead and
have less memory use for some runtime cost. We do store the enriched
applications, so this personalization is only in-memory code, so it
should be fast.

These changes seem to be fast for the handled applications case, so it
should be fast enough for the other cases.

Further changes can be done in a new PR also, after we get all the improvement PRs integrated.

Main changes:

  • adds 100 handlers to perf test data
  • fail outbox job immediately on address error
  • faster applications cache updates
  • fetch only active handlers from /handled API

Potential future tasks:

  • fetch only active handlers from other "overview" APIs
  • low level cache for more (user) tables
  • investigate real performance (in dev)

Checklist for author

Remove items that aren't applicable, check items that are done.

Reviewability

  • Link to issue

Backwards compatibility

  • API is backwards compatible or completely new

Documentation

  • Update changelog if necessary

Testing

  • Complex logic is unit tested

Follow-up

  • New tasks are created for pending or remaining tasks (TBD)

Macroz added 10 commits May 21, 2024 14:21
The idea is the same as in db events. Later the commonalities could
be refactored away.
The AddressException happens when there is an invalid character in the
email address (e.g. `;`). Let's drop those from the outbox immediately
so they won't be re-tried.
If there are 100 handlers, this means a lot of unnecessary users are
returned making the response unnecessarily big. We need the active
handlers for the table.

There could be a better API but let's make v2 sometime to return only
slim results and joining an option.
There's two changes here of which one is more subtle.

The original logic from years ago was such that all users rights were
updated whenever there was any change. This was fast for a while.

Last year, updates were implemented to only touch users that were
involved in the applications of the events. This was much faster.
There is however a flaw in this version.

Consider a case where one application was changed but there are 100
potential handlers in the organization. All the rights of these 100
handlers were updated by considering all their applications. Handlers
tend to be members of all applications so that could be 100 times the
application count of updates, e.g. 300K for 3000 applications. This
adds up to a few hundred milliseconds, which is too slow for us now,
,specially after adding more performance test data (10 handlers in
production so 100 handlers in performance test data).

We can avoid this amount of changes by recording how many times you have a
role instead of only whether you have the role. Then we remove the
role once per member of the old application and add 1 per member of new
application. This will total 100 changes for 100 users in one
application, which is as good as it can be. We could also record which
application gives which role, but that is slightly more memory
consuming for no additional gain.

The other subtler change is to cache only ids of applications that a person
has, not the personalized applications. 100 handlers with 3000
applications could result in 300K applications being cached.
Clojure's persistent data structures do help us here and in practice
that would not happen. However, we can avoid this by doing the
personalization in the functions that return results instead and
have less memory use for some runtime cost. We do store the enriched
applications, so this personalization is only in-memory code, so it
should be fast.

These changes seem to be fast for the handled applications case, so it
should be fast enough for the other cases.
@aatkin aatkin mentioned this pull request May 23, 2024
1 task
src/clj/rems/db/applications.clj Show resolved Hide resolved
src/clj/rems/db/applications.clj Show resolved Hide resolved
src/clj/rems/email/core.clj Outdated Show resolved Hide resolved
project.clj Outdated Show resolved Hide resolved
@Macroz Macroz mentioned this pull request May 24, 2024
This makes the functions cleaner and enables us to test the
implementation at least a little bit.
@Macroz Macroz merged commit 997a90c into master May 24, 2024
7 checks passed
@Macroz Macroz deleted the many-handlers-perf branch May 24, 2024 16:53
@Macroz Macroz mentioned this pull request Aug 22, 2024
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants