🚑 Fix performance form definition update #5085
Merged
Conversation
This file contains 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
sergei-maertens
added
the
needs-backport
|
Performance numbers on my local dev environment: Before patch
After patch
In this case, nothing was actually changed in the formio configuration, so the UPSERT query is essentially skipped (which made the bad performance all the more worse in the old situation before the patch). |
We noticed 3min wall clock time when updating a form definition that results in 7000 form variables being updated (local dev environment), which is completely unacceptable. Isolating the culprit/hot code path revealed that for 1000 form variables about 10-12 seconds was being spent, so a conservative time limit of 5 seconds is set up in tests to catch future performance regressions. However, realistically this should really take less than a second, but nobody like flaky tests so we pick something in the middle to account for different hardware in CI. Updates with small diffs should even take substantially less time, as we can ignore work that needs not to be done and avoid creating instances in the first place.
There are a number of aspects to this performance patch.
1. Avoid database load
The UPSERT query when sending all of the desired form variables
leads to hefty queries (250ms for 1100 variables, 1100ms for ~7K).
Splitting this up in work that can be ignore brings down the query
duration to about 50-60ms for 1000 variables.
2. Avoid expensive formio definition processing
By far the most expensive operation is scanning whether a component
is inside an editgrid ("repeating group") through the utility
component_in_editgrid, which was parsing the same configuration over
and over again. Instead, we can use the already-existing flag of
iter_components to not recurse into edit grids in the first place.
This fixes most of the time spent in Python.
3. Replace deepcopy with shallow copy
This is probably the most controversial one - when deepcopying
a django model instance, it goes through all the motions to serialize
it for picking, which means that it must figure out the reconstruction
function to use and capture all the necessary data, and deepcopy
recurses, which means it also goes into the related form_definition
and thus the formio configuration object which is full of lists/dicts
that are expensive to deep copy.
The replacement with a shallow copy should be fine because:
* we're not copying any existing database instances (pk=None)
* all the kwargs in initial instantiation are calculated and
provided explicitly, there is no reliance on mutation
* when 'copying' the desired variables for each form, we assign
the optimized form_id attribute and don't do any mutations,
i.e. all operations remain on the shallow level
This is covered with some tests to hopefully prevent future
regressions.
Other ideas considered:
* don't store FormVariables in the database, but instead create them
in memory on the fly. This will be a problem once we no longer store
prefill configuration in the component, we will require actual DB
instances. It's also not very intuitive
* offload to celery again. This is what we initially patched as it
was leading to race conditions and dead locks and general performance
issues too. It's may also strangely affect existing submissions.
Given the complexity and the performance gains, this was not further
explored.
On my local machine, this brings the worst case insert in the test
(1000 form variables from a form definition with 1000 components)
from 10+ seconds down to 400ms, so about a factor 25 improvement.
sergei-maertens
force-pushed
the
issue/5084-fix-performance-form-definition-update
branch
from
4fcbe35 to
86b83cc
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5085 +/- ##
=======================================
Coverage 96.73% 96.73%
=======================================
Files 771 771
Lines 26597 26619 +22
Branches 3460 3463 +3
=======================================
+ Hits 25729 25751 +22
Misses 606 606
Partials 262 262 ☔ View full report in Codecov by Sentry. |
vaszig
approved these changes
Feb 6, 2025
sergei-maertens
deleted the
issue/5084-fix-performance-form-definition-update
branch
sergei-maertens
added a commit
that referenced
this pull request
Feb 6, 2025
There are a number of aspects to this performance patch.
1. Avoid database load
The UPSERT query when sending all of the desired form variables
leads to hefty queries (250ms for 1100 variables, 1100ms for ~7K).
Splitting this up in work that can be ignore brings down the query
duration to about 50-60ms for 1000 variables.
2. Avoid expensive formio definition processing
By far the most expensive operation is scanning whether a component
is inside an editgrid ("repeating group") through the utility
component_in_editgrid, which was parsing the same configuration over
and over again. Instead, we can use the already-existing flag of
iter_components to not recurse into edit grids in the first place.
This fixes most of the time spent in Python.
3. Replace deepcopy with shallow copy
This is probably the most controversial one - when deepcopying
a django model instance, it goes through all the motions to serialize
it for picking, which means that it must figure out the reconstruction
function to use and capture all the necessary data, and deepcopy
recurses, which means it also goes into the related form_definition
and thus the formio configuration object which is full of lists/dicts
that are expensive to deep copy.
The replacement with a shallow copy should be fine because:
* we're not copying any existing database instances (pk=None)
* all the kwargs in initial instantiation are calculated and
provided explicitly, there is no reliance on mutation
* when 'copying' the desired variables for each form, we assign
the optimized form_id attribute and don't do any mutations,
i.e. all operations remain on the shallow level
This is covered with some tests to hopefully prevent future
regressions.
Other ideas considered:
* don't store FormVariables in the database, but instead create them
in memory on the fly. This will be a problem once we no longer store
prefill configuration in the component, we will require actual DB
instances. It's also not very intuitive
* offload to celery again. This is what we initially patched as it
was leading to race conditions and dead locks and general performance
issues too. It's may also strangely affect existing submissions.
Given the complexity and the performance gains, this was not further
explored.
On my local machine, this brings the worst case insert in the test
(1000 form variables from a form definition with 1000 components)
from 10+ seconds down to 400ms, so about a factor 25 improvement.
Backport-of: #5085
sergei-maertens
added a commit
that referenced
this pull request
Feb 6, 2025
There are a number of aspects to this performance patch.
1. Avoid database load
The UPSERT query when sending all of the desired form variables
leads to hefty queries (250ms for 1100 variables, 1100ms for ~7K).
Splitting this up in work that can be ignore brings down the query
duration to about 50-60ms for 1000 variables.
2. Avoid expensive formio definition processing
By far the most expensive operation is scanning whether a component
is inside an editgrid ("repeating group") through the utility
component_in_editgrid, which was parsing the same configuration over
and over again. Instead, we can use the already-existing flag of
iter_components to not recurse into edit grids in the first place.
This fixes most of the time spent in Python.
3. Replace deepcopy with shallow copy
This is probably the most controversial one - when deepcopying
a django model instance, it goes through all the motions to serialize
it for picking, which means that it must figure out the reconstruction
function to use and capture all the necessary data, and deepcopy
recurses, which means it also goes into the related form_definition
and thus the formio configuration object which is full of lists/dicts
that are expensive to deep copy.
The replacement with a shallow copy should be fine because:
* we're not copying any existing database instances (pk=None)
* all the kwargs in initial instantiation are calculated and
provided explicitly, there is no reliance on mutation
* when 'copying' the desired variables for each form, we assign
the optimized form_id attribute and don't do any mutations,
i.e. all operations remain on the shallow level
This is covered with some tests to hopefully prevent future
regressions.
Other ideas considered:
* don't store FormVariables in the database, but instead create them
in memory on the fly. This will be a problem once we no longer store
prefill configuration in the component, we will require actual DB
instances. It's also not very intuitive
* offload to celery again. This is what we initially patched as it
was leading to race conditions and dead locks and general performance
issues too. It's may also strangely affect existing submissions.
Given the complexity and the performance gains, this was not further
explored.
On my local machine, this brings the worst case insert in the test
(1000 form variables from a form definition with 1000 components)
from 10+ seconds down to 400ms, so about a factor 25 improvement.
Backport-of: #5085
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.
Closes #5084
Changes
Checklist
Check off the items that are completed or not relevant.
Impact on features
Release management
I have updated the translations assets (you do NOT need to provide translations)
./bin/makemessages_js.sh./bin/compilemessages_js.shDockerfile/scripts
./binfolderCommit hygiene