Speed up resource updates when there are no matching webhooks #7553
Conversation
|
/check |
|
❌ Some checks failed |
|
✔️ All checks completed successfully |
ca4adc7 to
505bbd2
Compare
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## develop #7553 +/- ##
===========================================
+ Coverage 83.53% 83.55% +0.02%
===========================================
Files 372 372
Lines 39661 39710 +49
Branches 3724 3729 +5
===========================================
+ Hits 33130 33180 +50
+ Misses 6531 6530 -1
|
| old_instance = sender.objects.get(pk=instance.pk) | ||
| created = False | ||
| except ObjectDoesNotExist: | ||
| created = True |
There was a problem hiding this comment.
Why do we need to handle it like this? If instance.pk is not None but the instance does not exist anymore, it means that the object was deleted before this code was executed. After initializing created = True the code below will check the wrong event "created:object". IMHO, we should stop handling here (return from function).
There was a problem hiding this comment.
If
instance.pk is not Nonebut the instance does not exist anymore,
I wasn't thinking of this case, but rather of the case where an object is created with a preset public key value. I don't think we do that anywhere right now, but I figured the code should be robust and handle that situation.
But also, in the case you describe, I don't see how my code is incorrect? If the instance was deleted prior the call to save, then .save will re-create it, so created:* seems like the correct event.
cvat/apps/webhooks/signals.py
Outdated
| old_data = instance._webhooks_old_data | ||
| del instance._webhooks_old_data | ||
|
|
||
| if not created and old_data is not None: | ||
| diff = get_instance_diff(old_data=old_data, data=serializer.data) | ||
|
|
||
| if diff: | ||
| data["before_update"] = { | ||
| attr: value["old_value"] | ||
| for attr, value in diff.items() | ||
| } |
There was a problem hiding this comment.
Probably it will be better to set _webhooks_old_data attribute only when object was updated and _webhooks_selected_webhooks is not empty
| old_data = instance._webhooks_old_data | |
| del instance._webhooks_old_data | |
| if not created and old_data is not None: | |
| diff = get_instance_diff(old_data=old_data, data=serializer.data) | |
| if diff: | |
| data["before_update"] = { | |
| attr: value["old_value"] | |
| for attr, value in diff.items() | |
| } | |
| if not created and (old_data := getattr(instance, '_webhooks_old_data', None)): | |
| del instance._webhooks_old_data | |
| if diff := get_instance_diff(old_data=old_data, data=serializer.data): | |
| data["before_update"] = { | |
| attr: value["old_value"] | |
| for attr, value in diff.items() | |
| } |
There was a problem hiding this comment.
I thought through this code again, and I decided to update it in a different way. I want to make sure that the value of created is consistent between the pre_save and post_save handlers, so I now initialize it based on the value of _webhooks_old_data.
The signal handling code in `cvat.apps.webhooks.signals` always fetches the old version of the resource from the database and serializes it. But if no webhook is defined for the current resource, this work is pointless. Select the webhooks first, and only then (if necessary) serialize the old resource. Also, move the diffing of the old and new versions to the `post_save` signal handler, to avoid serializing the new version a second time. This leads to a considerable speedup for resources with complicated serializers, such as tasks and jobs, when no webhooks are attached. This patch also makes the signal handlers more hygienic with the use of temporary attributes. Both the attributes it uses are now prefixed with `_webhooks` to avoid collisions, and removed after they are used.
505bbd2 to
648a842
Compare
Motivation and context
The signal handling code in
cvat.apps.webhooks.signalsalways fetches the old version of the resource from the database and serializes it. But if no webhook is defined for the current resource, this work is pointless.Select the webhooks first, and only then (if necessary) serialize the old resource. Also, move the diffing of the old and new versions to the
post_savesignal handler, to avoid serializing the new version a second time. This leads to a considerable speedup for resources with complicated serializers, such as tasks and jobs, when no webhooks are attached. In my experiments, saving of annotations was sped up by 20-25% with this patch.This patch also makes the signal handlers more hygienic with the use of temporary attributes. Both the attributes it uses are now prefixed with
_webhooksto avoid collisions, and removed after they are used.How has this been tested?
I think the existing tests are sufficient to cover this.
Checklist
developbranch[ ] I have updated the documentation accordingly[ ] I have added tests to cover my changes[ ] I have linked related issues (see GitHub docs)[ ] I have increased versions of npm packages if it is necessary(cvat-canvas,
cvat-core,
cvat-data and
cvat-ui)
License
Feel free to contact the maintainers if that's a concern.