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

Refactor schedule action in distributions_controller for better preformance #4509

Merged
merged 2 commits into from
Jul 19, 2024

Conversation

grantmca
Copy link
Contributor

@grantmca grantmca commented Jul 12, 2024

Resolves #4502

Description

  • Separated the handling of JSON and HTML requests. The instance variable is not used in the HTML view, eliminating unnecessary data loading.
  • Implemented JSON-specific requests to leverage the start and end parameters from the FullCalendar library. This restrict tags the fetched distributions to the specified dates instead of all historical records.
  • Resolved the N+1 query issue by eager loading the associated partners with the distributions.

Type of change

Performance change meant to preserved the original behavior of the controller action

How Has This Been Tested?

I have tested this locally creating some mock data and testing out the loading speed of the calendar

  • I was able to load 6300 distributions into the application for Pawnee Diaper Bank
  • Loading the calendar page with that many records took 13.27 s
  • Loading the same page after the changes took 1.13 s

I ran both page loads multiple times with the browser cache turned off and took the fastest time for both to ensure we are comparing as apples to apples as possible and both version can take advantage of query plan caching in PostgreSQL.

Screenshots

Before:
Screenshot 2024-07-12 at 5 54 39 PM

After:
Screenshot 2024-07-12 at 5 55 17 PM

…erformance

- Separated the handling of JSON and HTML requests. The instance variable is not used in the HTML view, eliminating unnecessary data loading.
- Implemented JSON-specific requests to leverage the `start` and `end` parameters from the FullCalendar library. This restrict tags the fetched distributions to the specified dates instead of all historical records.
- Resolved the N+1 query issue by eager loading the associated partners with the distributions.
- Add an index on issued_at to speed up the filtering on distributions
@grantmca grantmca force-pushed the 4502-impove-calendar-preformance branch from 71f5e44 to 7905eb5 Compare July 12, 2024 22:38
@grantmca grantmca changed the title WIP: Refactor schedule action in distributions_controller for better preformance Refactor schedule action in distributions_controller for better preformance Jul 12, 2024
@grantmca
Copy link
Contributor Author

Ready for some review on this one, if there are any follow up questions or additional benchmarks that I need to take let me know!

@cielf cielf requested a review from dorner July 13, 2024 20:59
@dorner
Copy link
Collaborator

dorner commented Jul 14, 2024

Looks all good on my end. @cielf did you want to verify the behavior? Also @grantmca there's a conflict that needs to be resolved.

Copy link
Collaborator

@cielf cielf left a comment

Choose a reason for hiding this comment

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

Much faster. No noticeable pause at all when checked with the concerned bank's data on local.

@grantmca
Copy link
Contributor Author

Nice! I can work on resolving that conflict this evening, for this project do we prefer a merge or rebase strategy for getting new changes into this branch? @dorner

@dorner
Copy link
Collaborator

dorner commented Jul 16, 2024

If any review has been done, please merge and don't rebase. If it's prior to a review, rebase is better.

@grantmca
Copy link
Contributor Author

Conflict was resolved and we should be all set, thank you both! @cielf @dorner

@dorner
Copy link
Collaborator

dorner commented Jul 19, 2024

All green - thanks!

@dorner dorner merged commit c283168 into rubyforgood:main Jul 19, 2024
19 checks passed
Copy link
Contributor

@grantmca: Your PR Refactor scheduleaction indistributions_controller for better preformance is part of today's Human Essentials production release: 2024.07.21.
Thank you very much for your contribution!

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.

Speed up pickup and deliveries calendar
3 participants