[04/11] Fetch all series for patch/cover viewing

Message ID 20180810080106.10714-5-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.
e.g. a 10 comment patch goes from 26 queries in 17-20ms down to 20
queries in 12ms.

A 67 comment cover letter goes from 14 queries in 16ms down to 8 queries
in 8ms.

So, effectively, a near 2x perf improvement.

Previously, at several points we were asking for the latest series and
then asking for all the series. Since there just usually aren't *that*
many series, fetch them all and take the first one if we need to.

Signed-off-by: Stewart Smith <stewart@linux.ibm.com>
---
 patchwork/templates/patchwork/submission.html | 10 +++++-----
 patchwork/views/cover.py                      |  2 +-
 patchwork/views/patch.py                      |  1 +
 3 files changed, 7 insertions(+), 6 deletions(-)

Comments

Stephen Finucane Aug. 31, 2018, 2:09 p.m. | #1
On Fri, 2018-08-10 at 18:00 +1000, Stewart Smith wrote:
> e.g. a 10 comment patch goes from 26 queries in 17-20ms down to 20
> queries in 12ms.
> 
> A 67 comment cover letter goes from 14 queries in 16ms down to 8 queries
> in 8ms.
> 
> So, effectively, a near 2x perf improvement.
> 
> Previously, at several points we were asking for the latest series and
> then asking for all the series. Since there just usually aren't *that*
> many series, fetch them all and take the first one if we need to.
> 
> Signed-off-by: Stewart Smith <stewart@linux.ibm.com>

Another "don't do stuff like this in templates" example. This one also
makes sense to me and definitely improves performance.

Reviewed-by: Stephen Finucane <stephen@that.guru>

Stephen

> ---
>  patchwork/templates/patchwork/submission.html | 10 +++++-----
>  patchwork/views/cover.py                      |  2 +-
>  patchwork/views/patch.py                      |  1 +
>  3 files changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/patchwork/templates/patchwork/submission.html b/patchwork/templates/patchwork/submission.html
> index 2f69735d6925..3b6f9fbe909e 100644
> --- a/patchwork/templates/patchwork/submission.html
> +++ b/patchwork/templates/patchwork/submission.html
> @@ -64,15 +64,15 @@ function toggle_div(link_id, headers_id)
>     </div>
>    </td>
>   </tr>
> -{% if submission.latest_series %}
> +{% if submission.all_series %}
>   <tr>
>    <th>Series</th>
>    <td>
>     <div class="patchrelations">
>      <ul>
> -     {% for series in submission.series.all %}
> +     {% for series in all_series %}
>       <li>
> -      {% if series == submission.latest_series %}
> +      {% if forloop.first %}
>         {{ series }}
>        {% else %}
>         <a href="{% url 'patch-list' project_id=project.linkname %}?series={{ series.id }}">
> @@ -93,7 +93,7 @@ function toggle_div(link_id, headers_id)
>     >show</a>
>     <div id="patchrelations" class="patchrelations" style="display:none;">
>      <ul>
> -    {% with submission.latest_series.cover_letter as cover %}
> +    {% with all_series.cover_letter as cover %}
>       <li>
>       {% if cover %}
>        {% if cover == submission %}
> @@ -106,7 +106,7 @@ function toggle_div(link_id, headers_id)
>       {% endif %}
>       </li>
>      {% endwith %}
> -    {% for sibling in submission.latest_series.patches.all %}
> +    {% for sibling in all_series.patches.all %}
>       <li>
>        {% if sibling == submission %}
>         {{ sibling.name|default:"[no subject]"|truncatechars:100 }}
> diff --git a/patchwork/views/cover.py b/patchwork/views/cover.py
> index edad90bc694d..1ee2b3f988fa 100644
> --- a/patchwork/views/cover.py
> +++ b/patchwork/views/cover.py
> @@ -49,7 +49,7 @@ def cover_detail(request, cover_id):
>      comments = comments.select_related('submitter')
>      comments = comments.only('submitter','date','id','content','submission')
>      context['comments'] = comments
> -
> +    context['all_series'] = cover.series.all().order_by('-date')
>      return render_to_response('patchwork/submission.html', context)
>  
>  
> diff --git a/patchwork/views/patch.py b/patchwork/views/patch.py
> index f43fbecd9a4d..e1d0cdcfcf39 100644
> --- a/patchwork/views/patch.py
> +++ b/patchwork/views/patch.py
> @@ -118,6 +118,7 @@ def patch_detail(request, patch_id):
>      comments = comments.select_related('submitter')
>      comments = comments.only('submitter','date','id','content','submission')
>  
> +    context['all_series'] = patch.series.all().order_by('-date')
>      context['comments'] = comments
>      context['submission'] = patch
>      context['patchform'] = form
Stephen Finucane Sept. 5, 2018, 7:58 p.m. | #2
On Fri, 2018-08-31 at 15:09 +0100, Stephen Finucane wrote:
> On Fri, 2018-08-10 at 18:00 +1000, Stewart Smith wrote:
> > e.g. a 10 comment patch goes from 26 queries in 17-20ms down to 20
> > queries in 12ms.
> > 
> > A 67 comment cover letter goes from 14 queries in 16ms down to 8 queries
> > in 8ms.
> > 
> > So, effectively, a near 2x perf improvement.
> > 
> > Previously, at several points we were asking for the latest series and
> > then asking for all the series. Since there just usually aren't *that*
> > many series, fetch them all and take the first one if we need to.
> > 
> > Signed-off-by: Stewart Smith <stewart@linux.ibm.com>
> 
> Another "don't do stuff like this in templates" example. This one also
> makes sense to me and definitely improves performance.
> 
> Reviewed-by: Stephen Finucane <stephen@that.guru>

There's a small issue with this one also. As with the other patch, I
can fix at merge time.

> Stephen
> 
> > ---
> >  patchwork/templates/patchwork/submission.html | 10 +++++-----
> >  patchwork/views/cover.py                      |  2 +-
> >  patchwork/views/patch.py                      |  1 +
> >  3 files changed, 7 insertions(+), 6 deletions(-)
> > 
> > diff --git a/patchwork/templates/patchwork/submission.html b/patchwork/templates/patchwork/submission.html
> > index 2f69735d6925..3b6f9fbe909e 100644
> > --- a/patchwork/templates/patchwork/submission.html
> > +++ b/patchwork/templates/patchwork/submission.html
> > @@ -64,15 +64,15 @@ function toggle_div(link_id, headers_id)
> >     </div>
> >    </td>
> >   </tr>
> > -{% if submission.latest_series %}
> > +{% if submission.all_series %}

This should presumably read 'all_series' - not 'submission.all_series'.

> >   <tr>
> >    <th>Series</th>
> >    <td>
> >     <div class="patchrelations">
> >      <ul>
> > -     {% for series in submission.series.all %}
> > +     {% for series in all_series %}
> >       <li>
> > -      {% if series == submission.latest_series %}
> > +      {% if forloop.first %}
> >         {{ series }}
> >        {% else %}
> >         <a href="{% url 'patch-list' project_id=project.linkname %}?series={{ series.id }}">
> > @@ -93,7 +93,7 @@ function toggle_div(link_id, headers_id)
> >     >show</a>
> >     <div id="patchrelations" class="patchrelations" style="display:none;">
> >      <ul>
> > -    {% with submission.latest_series.cover_letter as cover %}
> > +    {% with all_series.cover_letter as cover %}

'all_series' is a queryset so it doesn't have a 'cover_letter'
attribute. What we want is something like this.

   	<div id="patchrelations" class="patchrelations" style="display:none;">
  +	 {% for series in all_series %}
     	 <ul>
  +	 {% with series.cover_letter as cover %}

This will actually fix a small theoretical issue we have, whereby
dependencies for other series than the first weren't listed. I say
theoretical as we never actually assign more than one series to a patch
and I am actually looking at making the series-patch relationship a 1:N
relationship shortly.

Note that I didn't spot either of these initially as the templates
don't error out on missing attributes. I should check to see if that
behaviour is configurable.

Stephen

> >       <li>
> >       {% if cover %}
> >        {% if cover == submission %}
> > @@ -106,7 +106,7 @@ function toggle_div(link_id, headers_id)
> >       {% endif %}
> >       </li>
> >      {% endwith %}
> > -    {% for sibling in submission.latest_series.patches.all %}
> > +    {% for sibling in all_series.patches.all %}
> >       <li>
> >        {% if sibling == submission %}
> >         {{ sibling.name|default:"[no subject]"|truncatechars:100 }}
> > diff --git a/patchwork/views/cover.py b/patchwork/views/cover.py
> > index edad90bc694d..1ee2b3f988fa 100644
> > --- a/patchwork/views/cover.py
> > +++ b/patchwork/views/cover.py
> > @@ -49,7 +49,7 @@ def cover_detail(request, cover_id):
> >      comments = comments.select_related('submitter')
> >      comments = comments.only('submitter','date','id','content','submission')
> >      context['comments'] = comments
> > -
> > +    context['all_series'] = cover.series.all().order_by('-date')
> >      return render_to_response('patchwork/submission.html', context)
> >  
> >  
> > diff --git a/patchwork/views/patch.py b/patchwork/views/patch.py
> > index f43fbecd9a4d..e1d0cdcfcf39 100644
> > --- a/patchwork/views/patch.py
> > +++ b/patchwork/views/patch.py
> > @@ -118,6 +118,7 @@ def patch_detail(request, patch_id):
> >      comments = comments.select_related('submitter')
> >      comments = comments.only('submitter','date','id','content','submission')
> >  
> > +    context['all_series'] = patch.series.all().order_by('-date')
> >      context['comments'] = comments
> >      context['submission'] = patch
> >      context['patchform'] = form
> 
> 
> _______________________________________________
> Patchwork mailing list
> Patchwork@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/patchwork

Patch

diff --git a/patchwork/templates/patchwork/submission.html b/patchwork/templates/patchwork/submission.html
index 2f69735d6925..3b6f9fbe909e 100644
--- a/patchwork/templates/patchwork/submission.html
+++ b/patchwork/templates/patchwork/submission.html
@@ -64,15 +64,15 @@  function toggle_div(link_id, headers_id)
    </div>
   </td>
  </tr>
-{% if submission.latest_series %}
+{% if submission.all_series %}
  <tr>
   <th>Series</th>
   <td>
    <div class="patchrelations">
     <ul>
-     {% for series in submission.series.all %}
+     {% for series in all_series %}
      <li>
-      {% if series == submission.latest_series %}
+      {% if forloop.first %}
        {{ series }}
       {% else %}
        <a href="{% url 'patch-list' project_id=project.linkname %}?series={{ series.id }}">
@@ -93,7 +93,7 @@  function toggle_div(link_id, headers_id)
    >show</a>
    <div id="patchrelations" class="patchrelations" style="display:none;">
     <ul>
-    {% with submission.latest_series.cover_letter as cover %}
+    {% with all_series.cover_letter as cover %}
      <li>
      {% if cover %}
       {% if cover == submission %}
@@ -106,7 +106,7 @@  function toggle_div(link_id, headers_id)
      {% endif %}
      </li>
     {% endwith %}
-    {% for sibling in submission.latest_series.patches.all %}
+    {% for sibling in all_series.patches.all %}
      <li>
       {% if sibling == submission %}
        {{ sibling.name|default:"[no subject]"|truncatechars:100 }}
diff --git a/patchwork/views/cover.py b/patchwork/views/cover.py
index edad90bc694d..1ee2b3f988fa 100644
--- a/patchwork/views/cover.py
+++ b/patchwork/views/cover.py
@@ -49,7 +49,7 @@  def cover_detail(request, cover_id):
     comments = comments.select_related('submitter')
     comments = comments.only('submitter','date','id','content','submission')
     context['comments'] = comments
-
+    context['all_series'] = cover.series.all().order_by('-date')
     return render_to_response('patchwork/submission.html', context)
 
 
diff --git a/patchwork/views/patch.py b/patchwork/views/patch.py
index f43fbecd9a4d..e1d0cdcfcf39 100644
--- a/patchwork/views/patch.py
+++ b/patchwork/views/patch.py
@@ -118,6 +118,7 @@  def patch_detail(request, patch_id):
     comments = comments.select_related('submitter')
     comments = comments.only('submitter','date','id','content','submission')
 
+    context['all_series'] = patch.series.all().order_by('-date')
     context['comments'] = comments
     context['submission'] = patch
     context['patchform'] = form