Re: [PATCH 02/11] 4x performance improvement for viewing patch with many comments
Stephen Finucane writes: > On Fri, 2018-08-10 at 18:00 +1000, Stewart Smith wrote: >> Using the example of id:20180720035941.6844-1-khand...@linux.vnet.ibm.com >> with my test dataset of a chunk of a variety of mailing lists, has >> this cover letter have 67 comments from a variety of people. Thus, >> it's on the larger side of things. >> >> Originally, displaying the /patch/550/ for this (redirected to /cover) >> would take 81 SQL queries in ~60ms on my laptop. >> >> After this optimisation, it's down to 14 queries in 14ms. >> >> When the cache is cold, it's down to 32ms from 83ms. >> >> The effect of this patch is to execute a join in the database to >> get the submitter information for each comment at the same time as >> getting all the comments rather than doing a one-by-one lookup after >> the fact. >> >> Signed-off-by: Stewart Smith > > Looks good to me. Daniel's already pointed out the comma-space thing > (blame pep8) but we'll fix at merge time. > > Reviewed-by: Stephen Finucane > > I think the learning here, and for other, related patches in the > series, is that we should be more careful using any model relationships > in templates. Yeah, that was a bit of a theme for some of the performance things I was looking at. -- Stewart Smith OPAL Architect, IBM. ___ Patchwork mailing list Patchwork@lists.ozlabs.org https://lists.ozlabs.org/listinfo/patchwork
Re: [PATCH 02/11] 4x performance improvement for viewing patch with many comments
On Fri, 2018-08-10 at 18:00 +1000, Stewart Smith wrote:
> Using the example of id:20180720035941.6844-1-khand...@linux.vnet.ibm.com
> with my test dataset of a chunk of a variety of mailing lists, has
> this cover letter have 67 comments from a variety of people. Thus,
> it's on the larger side of things.
>
> Originally, displaying the /patch/550/ for this (redirected to /cover)
> would take 81 SQL queries in ~60ms on my laptop.
>
> After this optimisation, it's down to 14 queries in 14ms.
>
> When the cache is cold, it's down to 32ms from 83ms.
>
> The effect of this patch is to execute a join in the database to
> get the submitter information for each comment at the same time as
> getting all the comments rather than doing a one-by-one lookup after
> the fact.
>
> Signed-off-by: Stewart Smith
Looks good to me. Daniel's already pointed out the comma-space thing
(blame pep8) but we'll fix at merge time.
Reviewed-by: Stephen Finucane
I think the learning here, and for other, related patches in the
series, is that we should be more careful using any model relationships
in templates.
Stephen
> ---
> patchwork/templates/patchwork/submission.html | 2 +-
> patchwork/views/cover.py | 5 +
> patchwork/views/patch.py | 5 +
> 3 files changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/patchwork/templates/patchwork/submission.html
> b/patchwork/templates/patchwork/submission.html
> index e817713f7079..2f69735d6925 100644
> --- a/patchwork/templates/patchwork/submission.html
> +++ b/patchwork/templates/patchwork/submission.html
> @@ -262,7 +262,7 @@ function toggle_div(link_id, headers_id)
>
>
>
> -{% for item in submission.comments.all %}
> +{% for item in comments %}
> {% if forloop.first %}
> Comments
> {% endif %}
> diff --git a/patchwork/views/cover.py b/patchwork/views/cover.py
> index 73f83cb99b99..edad90bc694d 100644
> --- a/patchwork/views/cover.py
> +++ b/patchwork/views/cover.py
> @@ -45,6 +45,11 @@ def cover_detail(request, cover_id):
> 'project': cover.project,
> }
>
> +comments = cover.comments.all()
> +comments = comments.select_related('submitter')
> +comments = comments.only('submitter','date','id','content','submission')
> +context['comments'] = comments
> +
> return render_to_response('patchwork/submission.html', context)
>
>
> diff --git a/patchwork/views/patch.py b/patchwork/views/patch.py
> index cbd4ec395d99..f43fbecd9a4d 100644
> --- a/patchwork/views/patch.py
> +++ b/patchwork/views/patch.py
> @@ -114,6 +114,11 @@ def patch_detail(request, patch_id):
> if is_authenticated(request.user):
> context['bundles'] = request.user.bundles.all()
>
> +comments = patch.comments.all()
> +comments = comments.select_related('submitter')
> +comments = comments.only('submitter','date','id','content','submission')
> +
> +context['comments'] = comments
> context['submission'] = patch
> context['patchform'] = form
> context['createbundleform'] = createbundleform
___
Patchwork mailing list
Patchwork@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/patchwork
Re: [PATCH 02/11] 4x performance improvement for viewing patch with many comments
Daniel Axtens writes:
> Stewart Smith writes:
>
>> Using the example of id:20180720035941.6844-1-khand...@linux.vnet.ibm.com
>> with my test dataset of a chunk of a variety of mailing lists, has
>> this cover letter have 67 comments from a variety of people. Thus,
>> it's on the larger side of things.
>>
>> Originally, displaying the /patch/550/ for this (redirected to /cover)
>> would take 81 SQL queries in ~60ms on my laptop.
>>
>> After this optimisation, it's down to 14 queries in 14ms.
>>
>> When the cache is cold, it's down to 32ms from 83ms.
>>
>> The effect of this patch is to execute a join in the database to
>> get the submitter information for each comment at the same time as
>> getting all the comments rather than doing a one-by-one lookup after
>> the fact.
>>
>> Signed-off-by: Stewart Smith
>> ---
>> patchwork/templates/patchwork/submission.html | 2 +-
>> patchwork/views/cover.py | 5 +
>> patchwork/views/patch.py | 5 +
>> 3 files changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/patchwork/templates/patchwork/submission.html
>> b/patchwork/templates/patchwork/submission.html
>> index e817713f7079..2f69735d6925 100644
>> --- a/patchwork/templates/patchwork/submission.html
>> +++ b/patchwork/templates/patchwork/submission.html
>> @@ -262,7 +262,7 @@ function toggle_div(link_id, headers_id)
>>
>>
>>
>> -{% for item in submission.comments.all %}
>> +{% for item in comments %}
>> {% if forloop.first %}
>> Comments
>> {% endif %}
>> diff --git a/patchwork/views/cover.py b/patchwork/views/cover.py
>> index 73f83cb99b99..edad90bc694d 100644
>> --- a/patchwork/views/cover.py
>> +++ b/patchwork/views/cover.py
>> @@ -45,6 +45,11 @@ def cover_detail(request, cover_id):
>> 'project': cover.project,
>> }
>>
>> +comments = cover.comments.all()
>> +comments = comments.select_related('submitter')
>> +comments = comments.only('submitter','date','id','content','submission')
> These items should be separated by ", " not just ",". I can fix this up
> when I apply. You also don't need 'id' as it's added automatically.
(thanks for fixup).
Hrm.. I wonder why I had id there then... I wouldn't put it past Django
to do something silly otherwise, but I could well be wrong.
--
Stewart Smith
OPAL Architect, IBM.
___
Patchwork mailing list
Patchwork@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/patchwork
Re: [PATCH 02/11] 4x performance improvement for viewing patch with many comments
Stewart Smith writes:
> Using the example of id:20180720035941.6844-1-khand...@linux.vnet.ibm.com
> with my test dataset of a chunk of a variety of mailing lists, has
> this cover letter have 67 comments from a variety of people. Thus,
> it's on the larger side of things.
>
> Originally, displaying the /patch/550/ for this (redirected to /cover)
> would take 81 SQL queries in ~60ms on my laptop.
>
> After this optimisation, it's down to 14 queries in 14ms.
>
> When the cache is cold, it's down to 32ms from 83ms.
>
> The effect of this patch is to execute a join in the database to
> get the submitter information for each comment at the same time as
> getting all the comments rather than doing a one-by-one lookup after
> the fact.
>
> Signed-off-by: Stewart Smith
> ---
> patchwork/templates/patchwork/submission.html | 2 +-
> patchwork/views/cover.py | 5 +
> patchwork/views/patch.py | 5 +
> 3 files changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/patchwork/templates/patchwork/submission.html
> b/patchwork/templates/patchwork/submission.html
> index e817713f7079..2f69735d6925 100644
> --- a/patchwork/templates/patchwork/submission.html
> +++ b/patchwork/templates/patchwork/submission.html
> @@ -262,7 +262,7 @@ function toggle_div(link_id, headers_id)
>
>
>
> -{% for item in submission.comments.all %}
> +{% for item in comments %}
> {% if forloop.first %}
> Comments
> {% endif %}
> diff --git a/patchwork/views/cover.py b/patchwork/views/cover.py
> index 73f83cb99b99..edad90bc694d 100644
> --- a/patchwork/views/cover.py
> +++ b/patchwork/views/cover.py
> @@ -45,6 +45,11 @@ def cover_detail(request, cover_id):
> 'project': cover.project,
> }
>
> +comments = cover.comments.all()
> +comments = comments.select_related('submitter')
> +comments = comments.only('submitter','date','id','content','submission')
These items should be separated by ", " not just ",". I can fix this up
when I apply. You also don't need 'id' as it's added automatically.
Apart from that, looks good to me.
Regards,
Daniel
> +context['comments'] = comments
> +
> return render_to_response('patchwork/submission.html', context)
>
>
> diff --git a/patchwork/views/patch.py b/patchwork/views/patch.py
> index cbd4ec395d99..f43fbecd9a4d 100644
> --- a/patchwork/views/patch.py
> +++ b/patchwork/views/patch.py
> @@ -114,6 +114,11 @@ def patch_detail(request, patch_id):
> if is_authenticated(request.user):
> context['bundles'] = request.user.bundles.all()
>
> +comments = patch.comments.all()
> +comments = comments.select_related('submitter')
> +comments = comments.only('submitter','date','id','content','submission')
> +
> +context['comments'] = comments
> context['submission'] = patch
> context['patchform'] = form
> context['createbundleform'] = createbundleform
> --
> 2.17.1
>
> ___
> Patchwork mailing list
> Patchwork@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/patchwork
___
Patchwork mailing list
Patchwork@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/patchwork
[PATCH 02/11] 4x performance improvement for viewing patch with many comments
Using the example of id:20180720035941.6844-1-khand...@linux.vnet.ibm.com
with my test dataset of a chunk of a variety of mailing lists, has
this cover letter have 67 comments from a variety of people. Thus,
it's on the larger side of things.
Originally, displaying the /patch/550/ for this (redirected to /cover)
would take 81 SQL queries in ~60ms on my laptop.
After this optimisation, it's down to 14 queries in 14ms.
When the cache is cold, it's down to 32ms from 83ms.
The effect of this patch is to execute a join in the database to
get the submitter information for each comment at the same time as
getting all the comments rather than doing a one-by-one lookup after
the fact.
Signed-off-by: Stewart Smith
---
patchwork/templates/patchwork/submission.html | 2 +-
patchwork/views/cover.py | 5 +
patchwork/views/patch.py | 5 +
3 files changed, 11 insertions(+), 1 deletion(-)
diff --git a/patchwork/templates/patchwork/submission.html
b/patchwork/templates/patchwork/submission.html
index e817713f7079..2f69735d6925 100644
--- a/patchwork/templates/patchwork/submission.html
+++ b/patchwork/templates/patchwork/submission.html
@@ -262,7 +262,7 @@ function toggle_div(link_id, headers_id)
-{% for item in submission.comments.all %}
+{% for item in comments %}
{% if forloop.first %}
Comments
{% endif %}
diff --git a/patchwork/views/cover.py b/patchwork/views/cover.py
index 73f83cb99b99..edad90bc694d 100644
--- a/patchwork/views/cover.py
+++ b/patchwork/views/cover.py
@@ -45,6 +45,11 @@ def cover_detail(request, cover_id):
'project': cover.project,
}
+comments = cover.comments.all()
+comments = comments.select_related('submitter')
+comments = comments.only('submitter','date','id','content','submission')
+context['comments'] = comments
+
return render_to_response('patchwork/submission.html', context)
diff --git a/patchwork/views/patch.py b/patchwork/views/patch.py
index cbd4ec395d99..f43fbecd9a4d 100644
--- a/patchwork/views/patch.py
+++ b/patchwork/views/patch.py
@@ -114,6 +114,11 @@ def patch_detail(request, patch_id):
if is_authenticated(request.user):
context['bundles'] = request.user.bundles.all()
+comments = patch.comments.all()
+comments = comments.select_related('submitter')
+comments = comments.only('submitter','date','id','content','submission')
+
+context['comments'] = comments
context['submission'] = patch
context['patchform'] = form
context['createbundleform'] = createbundleform
--
2.17.1
___
Patchwork mailing list
Patchwork@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/patchwork
