| Message ID | 20180810080106.10714-5-stewart@linux.ibm.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | Performance for ALL THE THINGS! | expand |
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
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
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
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(-)