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