Improve performance generating outcome statuses #3262
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This adds a Rake task which we can use to quickly generate fake data suitable for some basic performance testing. It skips the import process and instead generates data directly in the database.
thomasleese
force-pushed
the
improve-performance
branch
from
b6cfe83 to
6927b4c
Compare
thomasleese
force-pushed
the
improve-performance
branch
from
6927b4c to
ce45a8b
Compare
thomasleese
force-pushed
the
improve-performance
branch
from
ce45a8b to
4734102
Compare
thomasleese
force-pushed
the
improve-performance
branch
from
4734102 to
42b77a1
Compare
thomasleese
force-pushed
the
improve-performance
branch
from
42b77a1 to
1d7a02e
Compare
thomasleese
force-pushed
the
improve-performance
branch
from
1d7a02e to
2d2fa80
Compare
thomasleese
force-pushed
the
improve-performance
branch
from
2d2fa80 to
1517e33
Compare
thomasleese
force-pushed
the
improve-performance
branch
from
0d37cbc to
405a78e
Compare
thomasleese
force-pushed
the
improve-performance
branch
from
405a78e to
3880f97
Compare
thomasleese
force-pushed
the
improve-performance
branch
from
bf9c0fa to
f654e60
Compare
thomasleese
force-pushed
the
improve-performance
branch
from
f654e60 to
d411390
Compare
This makes the profiling Gem available in development which can be used to check why parts of the code are slow.
thomasleese
force-pushed
the
improve-performance
branch
from
d411390 to
0e24485
Compare
thomasleese
force-pushed
the
improve-performance
branch
from
0e24485 to
54655bd
Compare
This refactors the `SearchForm` in preparation for some future performance optimisations, to make it easier to integrate with those upcoming changes.
This refactors the `SessionOutcome` class to accept a scope of patient sessions and determine the outcome of the patient sessions in one go by running a query that plucks only the necessary data and avoids the need to instanciate lots of ActiveRecord model objects.
This refactors the `RegisterOutcome` class to accept a scope of patient sessions and determine the outcome of the patient sessions in one go by running a query that plucks only the necessary data and avoids the need to instanciate lots of ActiveRecord model objects.
This refactors a number of classes to accept a scope of patients and determine the outcome of the patients in one go by running a query that plucks only the necessary data and avoids the need to instanciate lots of ActiveRecord model objects. Ideally the three classes would be refactored in separate commits, but they are closely tied together where changing one requires all of them to be changed.
This refactors the `ConsentOutcome` class to accept a scope of patients and determine the outcome of the patients in one go by running a query that plucks only the necessary data and avoids the need to instanciate lots of ActiveRecord model objects.
This refactors the `NextActivity` class to use the new `Outcomes` class introduced in the earlier commits.
This removes a scope on the `PatientSession` that was previously used to preload all the data necessary to generate an outcome status. Instead, we should preload only the data necessary at the point of fetching the patient sessions to avoid fetching unnecessary data.
This refactors the component to fetch all the patients sessions for all the sessions at once to avoid N+1 issues.
thomasleese
force-pushed
the
improve-performance
branch
from
54655bd to
f4c8f35
Compare
|
jhenderson
reviewed
Mar 21, 2025
|
|
||
| def no_response_count(session:) | ||
| number_stat(session:) { it.patient.consent_outcome.no_response?(programme) } | ||
| number_stat(session:) do |
There was a problem hiding this comment.
I'm a bit concerned by the it syntax (not just here). It's not apparent what it stands for. I had to jump into the method to see what it returns. I think we should use a block variable with a suitable name to make the code more readable (where appropriate).
thomasleese
added a commit
that referenced
this pull request
Mar 28, 2025
This introduces a new model, `Patient::ConsentStatus`, which stores the status of a patient-programme pair in the database so it can queried and rendered quickly. Locally, I'm seeing the consent tab on sessions with 50,000 patients load almost instantly even when filtering on statuses. The session overview page has a slight improvement, but that will only load quickly once the other statuses are cached. I've decided not to implement a state machine as had been previously discussed, but this could be built on top of this change to enhancement the functionality. For now, this effectively caches the existing logic that runs on the fly to the database. I've also decided not to implement this using Rails callbacks and instead be explicit where the status is refreshed. This allows us to optimise for bulk refreshes, as is necessary when adding new programmes to sessions, running the seeds, or generally updating the status of an entire session. This is proposed as an alternative to #3262 for consent statuses, we'd need to apply the same functionality to triage, programmes, sessions and next actions. If we're happy with this approach, we can migrate the statuses one by one to avoid a big bang approach unlike #3262.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This refactors the way outcome statuses are generated to avoid instantiating ActiveRecord model objects and instead pluck values from the database directly and process the raw values.
Locally, I’m seeing session pages load in about half the time than they would normally with 10,000, 50,000 and 100,000 patients. Further work is necessary to improve performance, likely requiring some sort of state machine or caching of the various statuses so they can be queried directly in SQL.
I’ve tried to make the commits in this PR as small as possible while avoiding breaking the functionality between the commits, but in general the refactors are wide reaching and therefore touch lots of part of the code. The feature tests haven’t required any changes.