[02/11] 4x performance improvement for viewing patch with many comments

Message ID 20180810080106.10714-3-stewart@linux.ibm.com
State Accepted
Headers show
Series
  • Performance for ALL THE THINGS!
Related show

Commit Message

Stewart Smith Aug. 10, 2018, 8 a.m.
Using the example of id:20180720035941.6844-1-khandual@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 <stewart@linux.ibm.com>
---
 patchwork/templates/patchwork/submission.html | 2 +-
 patchwork/views/cover.py                      | 5 +++++
 patchwork/views/patch.py                      | 5 +++++
 3 files changed, 11 insertions(+), 1 deletion(-)

Comments

Daniel Axtens Aug. 14, 2018, 7 a.m. | #1
Stewart Smith <stewart@linux.ibm.com> writes:

> Using the example of id:20180720035941.6844-1-khandual@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 <stewart@linux.ibm.com>
> ---
>  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)
>  </pre>
>  </div>
>  
> -{% for item in submission.comments.all %}
> +{% for item in comments %}
>  {% if forloop.first %}
>  <h2>Comments</h2>
>  {% 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
Stewart Smith Aug. 14, 2018, 7:03 a.m. | #2
Daniel Axtens <dja@axtens.net> writes:
> Stewart Smith <stewart@linux.ibm.com> writes:
>
>> Using the example of id:20180720035941.6844-1-khandual@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 <stewart@linux.ibm.com>
>> ---
>>  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)
>>  </pre>
>>  </div>
>>  
>> -{% for item in submission.comments.all %}
>> +{% for item in comments %}
>>  {% if forloop.first %}
>>  <h2>Comments</h2>
>>  {% 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.
Stephen Finucane Aug. 31, 2018, 2:06 p.m. | #3
On Fri, 2018-08-10 at 18:00 +1000, Stewart Smith wrote:
> Using the example of id:20180720035941.6844-1-khandual@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 <stewart@linux.ibm.com>

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 <stephen@that.guru>

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)
>  </pre>
>  </div>
>  
> -{% for item in submission.comments.all %}
> +{% for item in comments %}
>  {% if forloop.first %}
>  <h2>Comments</h2>
>  {% 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
Stewart Smith Sept. 10, 2018, 2:25 a.m. | #4
Stephen Finucane <stephen@that.guru> writes:
> On Fri, 2018-08-10 at 18:00 +1000, Stewart Smith wrote:
>> Using the example of id:20180720035941.6844-1-khandual@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 <stewart@linux.ibm.com>
>
> 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 <stephen@that.guru>
>
> 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.

Patch

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)
 </pre>
 </div>
 
-{% for item in submission.comments.all %}
+{% for item in comments %}
 {% if forloop.first %}
 <h2>Comments</h2>
 {% 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