Closed henribru closed 1 year ago
Thanks for this @henribru ! I was intitially not convinced by the rename of '+' into subtasks as it generates a migration ... even if the name made more sense for me. But it seems it is mandatory to have it working.
For the ChangeList, it works very well.
It works very well for me, with and without the :
@cached_property
def executing_dt(self):
if hasattr(self, "_executing_dt"):
return self._executing_dt
So I'm not completely sure about the use of this. Could you please explain?
For the ChangeForm, I still have n+1 query on TaskModel.state
.
Do you have them as well ?
In fact I don't really know what is the specificity & role of execute_dt
. I personally use create_dt
and update_dt
Could you clarify it for me?
In fact I don't really know what is the specificity & role of execute_dt. I personally use create_dt and update_dt
executing_dt
is used in elapsed_sec
which is used in human_throughput
, and human_throughput
is used in the admin, that's why I've had to optimize it.
It works very well for me, with and without the : So I'm not completely sure about the use of this. Could you please explain?
The default implementation of executing_dt
leads to a query per task, so if we don't annotate _executing_dt
and then use it in executing_dt
we still have n+1. The reason I'm doing the ugly hasattr
trick is so the old implementation is still available for backwards compatibility for anyone using this outside of the admin.
For the ChangeForm, I still have n+1 query on
TaskModel.state
. Do you have them as well ?
Probably, I didn't focus on optimizing ChangeForm
just because I've never had particularly noticeable performance issues with it.
I've been doing some testing on a production app where there are millions of TaskModel
and SignalInfoModel
. For TaskModel
these changes definitely help, though it's still quite slow. One interesting thing I noticed is that a lot of the time is actually spent calculating the filters. Indexing TaskModel.name
, SignalInfoModel.signal_name
and SignalInfoModel.hostname
seems to help with that issue, so it could be worth considering adding that.
The select_related
on task
for SignalInfoModel
seems to cause issues on such a large data set though, to the point where the change list isn't even loading in any time I've bothered waiting. I haven't investigated precisely which query it hangs on yet, but it seems like perhaps this join ends up being a lot slower than just doing n+1 queries. I don't really understand why given that the select_related
s on TaskModel
are improving performance. Maybe it will be more performant to do prefetch_related
since that gets rid of the join at the cost of 1 additional query. I need to do some more investigation
For the ChangeForm, I still have n+1 query on
TaskModel.state
. Do you have them as well ?Probably, I didn't focus on optimizing
ChangeForm
just because I've never had particularly noticeable performance issues with it.
So I checked, the calls responsible for the ChangeForm
n+1 calls are not related to the calls made by ChangList
. Therefore it does not add any benefit to move the get_queryset from TaskModelChangeList
to TaskModelAdmin
But as you say, we can live with those ChangeForm
n+1 calls
It works very well for me, with and without the : So I'm not completely sure about the use of this. Could you please explain?
The default implementation of
executing_dt
leads to a query per task, so if we don't annotate_executing_dt
and then use it inexecuting_dt
we still have n+1. The reason I'm doing the uglyhasattr
trick is so the old implementation is still available for backwards compatibility for anyone using this outside of the admin.
What about implementing your idea, but without declaring _executing_dt
? executing_dt
is anyway declared as a cached_property
. So we can directly set it with the get_queryset annotation:
qs = (
qs.filter(parent_task__isnull=True)
.prefetch_related(
Prefetch(
"sub_tasks",
queryset=TaskModel.objects.select_related("state")
.annotate(executing_dt=executing_dt)
.order_by('-create_dt'),
)
)
.annotate(executing_dt=executing_dt)
)
and then we don't need the extra 2 lines:
if hasattr(self, "_executing_dt"):
return self._executing_dt
It seems to work fine for me What do you think?
Ah, I see what you mean. I thought maybe if I annotated executing_dt
the property method would take precedence and still be called, but you're probably right that it's the opposite
Ah, I see what you mean. I thought maybe if I annotated
executing_dt
the property method would take precedence and still be called, but you're probably right that it's the opposite
because of the cached_property decorator, the system first looks into the cache if there is already a value. If there is, it returns it if there isn't, then it runs the code as defined in the property definition, and store the result into the cache for further use
When you annotate, you store data for a given property into the cache ... so you fill up the cache
Merging #110 (28e728c) into master (c5a741e) will increase coverage by
0.18%
. The diff coverage is84.61%
.
@@ Coverage Diff @@
## master #110 +/- ##
==========================================
+ Coverage 83.92% 84.11% +0.18%
==========================================
Files 40 41 +1
Lines 840 850 +10
==========================================
+ Hits 705 715 +10
Misses 135 135
Impacted Files | Coverage Δ | |
---|---|---|
huey_monitor/models.py | 93.10% <50.00%> (-0.76%) |
:arrow_down: |
huey_monitor/admin.py | 86.90% <83.33%> (+1.71%) |
:arrow_up: |
...tor/migrations/0010_alter_taskmodel_parent_task.py | 100.00% <100.00%> (ø) |
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
@henribru i used your code changes and create a new PR with rebased code here: https://github.com/boxine/django-huey-monitor/pull/114
@henribru i used your code changes and create a new PR with rebased code here: https://github.com/boxine/django-huey-monitor/pull/114
Okay, great! Though if you could add me as a co-author on the commit with the syntax shown in https://docs.github.com/en/pull-requests/committing-changes-to-your-project/creating-and-editing-commits/creating-a-commit-with-multiple-authors that'd be nice
Good idea: Done see: https://github.com/boxine/django-huey-monitor/pull/114/commits
@jedie Don't think it quite worked, when it does it should show both me and you as authors, random example from another repo:
I think it's because you used a link to my profile in the brackets instead of an email address. You can use the one from my commits in this branch, i.e. 6639509+henribru@users.noreply.github.com
fixed:
Thanks, appreciate it!
Fixes https://github.com/boxine/django-huey-monitor/issues/88
With these changes the number of queries should now be constant and independent of the number and nature of the tasks.
I must say, the make file in this repository is excellent! It makes contributing very simple.
Before:
![Screenshot from 2022-11-25 13-35-00](https://user-images.githubusercontent.com/6639509/203988052-dad02366-77a6-4abe-aa77-4ba855e46a36.png)
After:
![Screenshot from 2022-11-25 13-34-37](https://user-images.githubusercontent.com/6639509/203988122-f6325984-938f-4bd5-b80d-0b34ecb60a1d.png)