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 perf2 #3313

Closed
wants to merge 8 commits into from
Closed

Many handlers perf2 #3313

wants to merge 8 commits into from

Conversation

Macroz
Copy link
Collaborator

@Macroz Macroz commented Jun 11, 2024

Checklist for author

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

Reviewability

  • Link to issue
  • Note if PR is on top of other PR
  • Note if related change in rems-deploy repo
  • Consider adding screenshots for ease of review

Backwards compatibility

  • API is backwards compatible or completely new
  • Events are backwards compatible or have migrations
  • Config is backwards compatible
  • Feature appears correctly in PDF print

Documentation

  • Update changelog if necessary
  • API is documented and shows up in Swagger UI
  • Components are added to guide page
  • Update docs/ (if applicable)
  • Update manual/ (if applicable)
  • ADR for major architectural decisions or experiments
  • New config options in config-defaults.edn

Testing

  • Complex logic is unit tested
  • Valuable features are integration / browser / acceptance tested automatically

Follow-up

  • New tasks are created for pending or remaining tasks

Macroz and others added 8 commits June 11, 2024 11:06
If we are only going to return "first 50" from the API for our UI, we
can keep the pipe lazy so that only 50 rows ever get processed. This
makes the handled applications opening a bit faster.

The `get-handled-applications` is also extracted for convenience of
calling in e.g. a performance test.
This makes it easier to see over time if we are slower or faster. We
would like this to be as fast as possible.
Let's use a single SQL statement to send many emails instead of many
calls. NB, we must change the mocking to fake the new method.
We can cache the user attributes like we do user settings.

Ideas for later:
- We cache user settings separately from user cache, when we should likely only
  have one cache for all user attributes, settings and roles.
- Changing user settings should invalidate user cache because it
  contains settings too (`:notification-email`)
- We should really now remove format/unformat user.
- All other parts of REMS outside of tests should rather use complete
  user and any "raw attributes" should be internal to user ns or gone
  completely.
- We should consider what removing a user means. The function that
  exists should not be needed as it is.
- using `reduce` is much faster than `mapcat`
- `if-some` is cleaner here
Schema checking of large results is very slow. We should consider
leaving it out of some APIs where performance matters.
@Macroz Macroz mentioned this pull request Aug 22, 2024
6 tasks
@Macroz Macroz closed this Aug 23, 2024
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