| Message ID | 20200129190122.21264-1-metepolat2000@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | REST: Fix duplicate project queries | expand |
On Wed, 2020-01-29 at 20:01 +0100, Mete Polat wrote: > Eliminates duplicate project queries caused by calling > get_absolute_url() in the embedded serializers. Following foreign keys > with 'series__project' will cache the project of the series as well as > the series itself. > > Signed-off-by: Mete Polat <metepolat2000@gmail.com> Hey Mete, > --- > There are still some duplicates in various /api/ views but it looks like > those are caused by the REST framework itself. > > patchwork/api/cover.py | 2 +- > patchwork/api/event.py | 4 ++-- > patchwork/api/patch.py | 2 +- > patchwork/api/series.py | 5 +++-- > 4 files changed, 7 insertions(+), 6 deletions(-) > > diff --git a/patchwork/api/cover.py b/patchwork/api/cover.py > index caf9a386efa5..9e86d47e00e5 100644 > --- a/patchwork/api/cover.py > +++ b/patchwork/api/cover.py > @@ -101,7 +101,7 @@ class CoverLetterList(ListAPIView): > > def get_queryset(self): > return CoverLetter.objects.all()\ > - .select_related('project', 'submitter', 'series')\ > + .select_related('project', 'submitter', 'series__project')\ > .defer('content', 'headers') > > > diff --git a/patchwork/api/event.py b/patchwork/api/event.py > index a066faaec63b..fdff6a4f2fa6 100644 > --- a/patchwork/api/event.py > +++ b/patchwork/api/event.py > @@ -86,7 +86,7 @@ class EventList(ListAPIView): > > def get_queryset(self): > return Event.objects.all()\ > - .prefetch_related('project', 'patch', 'series', 'cover', > - 'previous_state', 'current_state', > + .prefetch_related('project', 'patch__project', 'series__project', > + 'cover', 'previous_state', 'current_state', > 'previous_delegate', 'current_delegate', > 'created_check') The rest of these look good but I wasn't able to produce a test that proved this particular change was doing anything. Are you sure this particular change works and, if so, could you suggest one or more scenarios that I could use to validate this? Cheers, Stephen > diff --git a/patchwork/api/patch.py b/patchwork/api/patch.py > index a29a1ab0eb71..1a3ce9057490 100644 > --- a/patchwork/api/patch.py > +++ b/patchwork/api/patch.py > @@ -176,7 +176,7 @@ class PatchList(ListAPIView): > return Patch.objects.all()\ > .prefetch_related('check_set')\ > .select_related('project', 'state', 'submitter', 'delegate', > - 'series')\ > + 'series__project')\ > .defer('content', 'diff', 'headers') > > > diff --git a/patchwork/api/series.py b/patchwork/api/series.py > index f7bb8c06a6c9..df28f95dab1b 100644 > --- a/patchwork/api/series.py > +++ b/patchwork/api/series.py > @@ -55,8 +55,9 @@ class SeriesMixin(object): > serializer_class = SeriesSerializer > > def get_queryset(self): > - return Series.objects.all().prefetch_related('patches',)\ > - .select_related('submitter', 'cover_letter', 'project') > + return Series.objects.all()\ > + .prefetch_related('patches__project',)\ > + .select_related('submitter', 'cover_letter__project', 'project') > > > class SeriesList(SeriesMixin, ListAPIView):
Hi Stephen, On 01.02.20 15:05, Stephen Finucane wrote: > On Wed, 2020-01-29 at 20:01 +0100, Mete Polat wrote: >> Eliminates duplicate project queries caused by calling >> get_absolute_url() in the embedded serializers. Following foreign keys >> with 'series__project' will cache the project of the series as well as >> the series itself. >> >> Signed-off-by: Mete Polat <metepolat2000@gmail.com> > > Hey Mete, > >> --- >> There are still some duplicates in various /api/ views but it looks like >> those are caused by the REST framework itself. >> >> patchwork/api/cover.py | 2 +- >> patchwork/api/event.py | 4 ++-- >> patchwork/api/patch.py | 2 +- >> patchwork/api/series.py | 5 +++-- >> 4 files changed, 7 insertions(+), 6 deletions(-) >> >> diff --git a/patchwork/api/cover.py b/patchwork/api/cover.py >> index caf9a386efa5..9e86d47e00e5 100644 >> --- a/patchwork/api/cover.py >> +++ b/patchwork/api/cover.py >> @@ -101,7 +101,7 @@ class CoverLetterList(ListAPIView): >> >> def get_queryset(self): >> return CoverLetter.objects.all()\ >> - .select_related('project', 'submitter', 'series')\ >> + .select_related('project', 'submitter', 'series__project')\ >> .defer('content', 'headers') >> >> >> diff --git a/patchwork/api/event.py b/patchwork/api/event.py >> index a066faaec63b..fdff6a4f2fa6 100644 >> --- a/patchwork/api/event.py >> +++ b/patchwork/api/event.py >> @@ -86,7 +86,7 @@ class EventList(ListAPIView): >> >> def get_queryset(self): >> return Event.objects.all()\ >> - .prefetch_related('project', 'patch', 'series', 'cover', >> - 'previous_state', 'current_state', >> + .prefetch_related('project', 'patch__project', 'series__project', >> + 'cover', 'previous_state', 'current_state', >> 'previous_delegate', 'current_delegate', >> 'created_check') > > The rest of these look good but I wasn't able to produce a test that > proved this particular change was doing anything. Are you sure this > particular change works and, if so, could you suggest one or more > scenarios that I could use to validate this? > Just checked it again. Without patch__project, I get tons of these duplicate queries: SELECT `patchwork_project`.`id`, `patchwork_project`.`linkname`, `patchwork_project`.`name`, `patchwork_project`.`listid`, `patchwork_project`.`listemail`, `patchwork_project`.`subject_match`, `patchwork_project`.`web_url`, `patchwork_project`.`scm_url`, `patchwork_project`.`webscm_url`, `patchwork_project`.`list_archive_url`, `patchwork_project`.`list_archive_url_format`, `patchwork_project`.`commit_url_format`, `patchwork_project`.`send_notifications`, `patchwork_project`.`use_tags` FROM `patchwork_project` WHERE `patchwork_project`.`id` = 2 opt/pyenv/versions/3.8.0/lib/python3.8/site-packages/django/contrib/staticfiles/handlers.py in __call__(65) return self.application(environ, start_response) /opt/pyenv/versions/3.8.0/lib/python3.8/site-packages/django/views/decorators/csrf.py in wrapped_view(54) return view_func(*args, **kwargs) /opt/pyenv/versions/3.8.0/lib/python3.8/site-packages/django/views/generic/base.py in view(71) return self.dispatch(request, *args, **kwargs) /opt/pyenv/versions/3.8.0/lib/python3.8/site-packages/rest_framework/views.py in dispatch(502) response = handler(request, *args, **kwargs) /opt/pyenv/versions/3.8.0/lib/python3.8/site-packages/rest_framework/generics.py in get(199) return self.list(request, *args, **kwargs) /opt/pyenv/versions/3.8.0/lib/python3.8/site-packages/rest_framework/mixins.py in list(43) return self.get_paginated_response(serializer.data) /opt/pyenv/versions/3.8.0/lib/python3.8/site-packages/rest_framework/serializers.py in data(757) ret = super().data /opt/pyenv/versions/3.8.0/lib/python3.8/site-packages/rest_framework/serializers.py in data(261) self._data = self.to_representation(self.instance) /opt/pyenv/versions/3.8.0/lib/python3.8/site-packages/rest_framework/serializers.py in to_representation(674) return [ /opt/pyenv/versions/3.8.0/lib/python3.8/site-packages/rest_framework/serializers.py in <listcomp>(675) self.child.to_representation(item) for item in iterable /home/patchwork/patchwork/patchwork/api/event.py in to_representation(51) data = super(EventSerializer, self).to_representation(instance) /opt/pyenv/versions/3.8.0/lib/python3.8/site-packages/rest_framework/serializers.py in to_representation(526) ret[field.field_name] = field.to_representation(attribute) /home/patchwork/patchwork/patchwork/api/embedded.py in to_representation(57) return self._Serializer(context=self.context).to_representation(data) /home/patchwork/patchwork/patchwork/api/base.py in to_representation(90) data = super(BaseHyperlinkedModelSerializer, self).to_representation( /opt/pyenv/versions/3.8.0/lib/python3.8/site-packages/rest_framework/serializers.py in to_representation(526) ret[field.field_name] = field.to_representation(attribute) /opt/pyenv/versions/3.8.0/lib/python3.8/site-packages/rest_framework/fields.py in to_representation(1873) return method(value) /home/patchwork/patchwork/patchwork/api/embedded.py in get_web_url(81) return request.build_absolute_uri(instance.get_absolute_url()) /home/patchwork/patchwork/patchwork/models.py in get_absolute_url(600) kwargs={'project_id': self.project.linkname, Same goes for series__project. I didn't write a test case but I was able to manually produce this behavior by just viewing the /api/events on my local instance and by optionally filtering for patch/series_created events. I think you should be able to reproduce this when there are enough of these patch/series events. Best regards, Mete > Cheers, > Stephen > >> diff --git a/patchwork/api/patch.py b/patchwork/api/patch.py >> index a29a1ab0eb71..1a3ce9057490 100644 >> --- a/patchwork/api/patch.py >> +++ b/patchwork/api/patch.py >> @@ -176,7 +176,7 @@ class PatchList(ListAPIView): >> return Patch.objects.all()\ >> .prefetch_related('check_set')\ >> .select_related('project', 'state', 'submitter', 'delegate', >> - 'series')\ >> + 'series__project')\ >> .defer('content', 'diff', 'headers') >> >> >> diff --git a/patchwork/api/series.py b/patchwork/api/series.py >> index f7bb8c06a6c9..df28f95dab1b 100644 >> --- a/patchwork/api/series.py >> +++ b/patchwork/api/series.py >> @@ -55,8 +55,9 @@ class SeriesMixin(object): >> serializer_class = SeriesSerializer >> >> def get_queryset(self): >> - return Series.objects.all().prefetch_related('patches',)\ >> - .select_related('submitter', 'cover_letter', 'project') >> + return Series.objects.all()\ >> + .prefetch_related('patches__project',)\ >> + .select_related('submitter', 'cover_letter__project', 'project') >> >> >> class SeriesList(SeriesMixin, ListAPIView): >
On Sun, 2020-02-02 at 11:29 +0100, Mete Polat wrote: > Hi Stephen, > > On 01.02.20 15:05, Stephen Finucane wrote: > > On Wed, 2020-01-29 at 20:01 +0100, Mete Polat wrote: > > > Eliminates duplicate project queries caused by calling > > > get_absolute_url() in the embedded serializers. Following foreign keys > > > with 'series__project' will cache the project of the series as well as > > > the series itself. > > > > > > Signed-off-by: Mete Polat <metepolat2000@gmail.com> > > > > Hey Mete, > > > > > --- > > > There are still some duplicates in various /api/ views but it looks like > > > those are caused by the REST framework itself. > > > > > > patchwork/api/cover.py | 2 +- > > > patchwork/api/event.py | 4 ++-- > > > patchwork/api/patch.py | 2 +- > > > patchwork/api/series.py | 5 +++-- > > > 4 files changed, 7 insertions(+), 6 deletions(-) > > > > > > diff --git a/patchwork/api/cover.py b/patchwork/api/cover.py > > > index caf9a386efa5..9e86d47e00e5 100644 > > > --- a/patchwork/api/cover.py > > > +++ b/patchwork/api/cover.py > > > @@ -101,7 +101,7 @@ class CoverLetterList(ListAPIView): > > > > > > def get_queryset(self): > > > return CoverLetter.objects.all()\ > > > - .select_related('project', 'submitter', 'series')\ > > > + .select_related('project', 'submitter', 'series__project')\ > > > .defer('content', 'headers') > > > > > > > > > diff --git a/patchwork/api/event.py b/patchwork/api/event.py > > > index a066faaec63b..fdff6a4f2fa6 100644 > > > --- a/patchwork/api/event.py > > > +++ b/patchwork/api/event.py > > > @@ -86,7 +86,7 @@ class EventList(ListAPIView): > > > > > > def get_queryset(self): > > > return Event.objects.all()\ > > > - .prefetch_related('project', 'patch', 'series', 'cover', > > > - 'previous_state', 'current_state', > > > + .prefetch_related('project', 'patch__project', 'series__project', > > > + 'cover', 'previous_state', 'current_state', > > > 'previous_delegate', 'current_delegate', > > > 'created_check') > > > > The rest of these look good but I wasn't able to produce a test that > > proved this particular change was doing anything. Are you sure this > > particular change works and, if so, could you suggest one or more > > scenarios that I could use to validate this? > > > > Just checked it again. Without patch__project, I get tons of these > duplicate queries: > > SELECT `patchwork_project`.`id`, > `patchwork_project`.`linkname`, > `patchwork_project`.`name`, > `patchwork_project`.`listid`, > `patchwork_project`.`listemail`, > `patchwork_project`.`subject_match`, > `patchwork_project`.`web_url`, > `patchwork_project`.`scm_url`, > `patchwork_project`.`webscm_url`, > `patchwork_project`.`list_archive_url`, > `patchwork_project`.`list_archive_url_format`, > `patchwork_project`.`commit_url_format`, > `patchwork_project`.`send_notifications`, > `patchwork_project`.`use_tags` > FROM `patchwork_project` > WHERE `patchwork_project`.`id` = 2 > > opt/pyenv/versions/3.8.0/lib/python3.8/site-packages/django/contrib/staticfiles/handlers.py > in __call__(65) > return self.application(environ, start_response) > /opt/pyenv/versions/3.8.0/lib/python3.8/site-packages/django/views/decorators/csrf.py > in wrapped_view(54) > return view_func(*args, **kwargs) > /opt/pyenv/versions/3.8.0/lib/python3.8/site-packages/django/views/generic/base.py > in view(71) > return self.dispatch(request, *args, **kwargs) > /opt/pyenv/versions/3.8.0/lib/python3.8/site-packages/rest_framework/views.py > in dispatch(502) > response = handler(request, *args, **kwargs) > /opt/pyenv/versions/3.8.0/lib/python3.8/site-packages/rest_framework/generics.py > in get(199) > return self.list(request, *args, **kwargs) > /opt/pyenv/versions/3.8.0/lib/python3.8/site-packages/rest_framework/mixins.py > in list(43) > return self.get_paginated_response(serializer.data) > /opt/pyenv/versions/3.8.0/lib/python3.8/site-packages/rest_framework/serializers.py > in data(757) > ret = super().data > /opt/pyenv/versions/3.8.0/lib/python3.8/site-packages/rest_framework/serializers.py > in data(261) > self._data = self.to_representation(self.instance) > /opt/pyenv/versions/3.8.0/lib/python3.8/site-packages/rest_framework/serializers.py > in to_representation(674) > return [ > /opt/pyenv/versions/3.8.0/lib/python3.8/site-packages/rest_framework/serializers.py > in <listcomp>(675) > self.child.to_representation(item) for item in iterable > /home/patchwork/patchwork/patchwork/api/event.py in to_representation(51) > data = super(EventSerializer, self).to_representation(instance) > /opt/pyenv/versions/3.8.0/lib/python3.8/site-packages/rest_framework/serializers.py > in to_representation(526) > ret[field.field_name] = field.to_representation(attribute) > /home/patchwork/patchwork/patchwork/api/embedded.py in to_representation(57) > return self._Serializer(context=self.context).to_representation(data) > /home/patchwork/patchwork/patchwork/api/base.py in to_representation(90) > data = super(BaseHyperlinkedModelSerializer, self).to_representation( > /opt/pyenv/versions/3.8.0/lib/python3.8/site-packages/rest_framework/serializers.py > in to_representation(526) > ret[field.field_name] = field.to_representation(attribute) > /opt/pyenv/versions/3.8.0/lib/python3.8/site-packages/rest_framework/fields.py > in to_representation(1873) > return method(value) > /home/patchwork/patchwork/patchwork/api/embedded.py in get_web_url(81) > return request.build_absolute_uri(instance.get_absolute_url()) > /home/patchwork/patchwork/patchwork/models.py in get_absolute_url(600) > kwargs={'project_id': self.project.linkname, > > Same goes for series__project. > I didn't write a test case but I was able to manually produce this > behavior by just viewing the /api/events on my local instance and by > optionally filtering for patch/series_created events. I think you should > be able to reproduce this when there are enough of these patch/series > events. Ok, managed to get this to have an effect once I started creating a few more events. Have applied this along with the test patch I sent yesterday. Thanks! Stephen > Best regards, > > Mete > > > Cheers, > > Stephen > > > > > diff --git a/patchwork/api/patch.py b/patchwork/api/patch.py > > > index a29a1ab0eb71..1a3ce9057490 100644 > > > --- a/patchwork/api/patch.py > > > +++ b/patchwork/api/patch.py > > > @@ -176,7 +176,7 @@ class PatchList(ListAPIView): > > > return Patch.objects.all()\ > > > .prefetch_related('check_set')\ > > > .select_related('project', 'state', 'submitter', 'delegate', > > > - 'series')\ > > > + 'series__project')\ > > > .defer('content', 'diff', 'headers') > > > > > > > > > diff --git a/patchwork/api/series.py b/patchwork/api/series.py > > > index f7bb8c06a6c9..df28f95dab1b 100644 > > > --- a/patchwork/api/series.py > > > +++ b/patchwork/api/series.py > > > @@ -55,8 +55,9 @@ class SeriesMixin(object): > > > serializer_class = SeriesSerializer > > > > > > def get_queryset(self): > > > - return Series.objects.all().prefetch_related('patches',)\ > > > - .select_related('submitter', 'cover_letter', 'project') > > > + return Series.objects.all()\ > > > + .prefetch_related('patches__project',)\ > > > + .select_related('submitter', 'cover_letter__project', 'project') > > > > > > > > > class SeriesList(SeriesMixin, ListAPIView):
diff --git a/patchwork/api/cover.py b/patchwork/api/cover.py index caf9a386efa5..9e86d47e00e5 100644 --- a/patchwork/api/cover.py +++ b/patchwork/api/cover.py @@ -101,7 +101,7 @@ class CoverLetterList(ListAPIView): def get_queryset(self): return CoverLetter.objects.all()\ - .select_related('project', 'submitter', 'series')\ + .select_related('project', 'submitter', 'series__project')\ .defer('content', 'headers') diff --git a/patchwork/api/event.py b/patchwork/api/event.py index a066faaec63b..fdff6a4f2fa6 100644 --- a/patchwork/api/event.py +++ b/patchwork/api/event.py @@ -86,7 +86,7 @@ class EventList(ListAPIView): def get_queryset(self): return Event.objects.all()\ - .prefetch_related('project', 'patch', 'series', 'cover', - 'previous_state', 'current_state', + .prefetch_related('project', 'patch__project', 'series__project', + 'cover', 'previous_state', 'current_state', 'previous_delegate', 'current_delegate', 'created_check') diff --git a/patchwork/api/patch.py b/patchwork/api/patch.py index a29a1ab0eb71..1a3ce9057490 100644 --- a/patchwork/api/patch.py +++ b/patchwork/api/patch.py @@ -176,7 +176,7 @@ class PatchList(ListAPIView): return Patch.objects.all()\ .prefetch_related('check_set')\ .select_related('project', 'state', 'submitter', 'delegate', - 'series')\ + 'series__project')\ .defer('content', 'diff', 'headers') diff --git a/patchwork/api/series.py b/patchwork/api/series.py index f7bb8c06a6c9..df28f95dab1b 100644 --- a/patchwork/api/series.py +++ b/patchwork/api/series.py @@ -55,8 +55,9 @@ class SeriesMixin(object): serializer_class = SeriesSerializer def get_queryset(self): - return Series.objects.all().prefetch_related('patches',)\ - .select_related('submitter', 'cover_letter', 'project') + return Series.objects.all()\ + .prefetch_related('patches__project',)\ + .select_related('submitter', 'cover_letter__project', 'project') class SeriesList(SeriesMixin, ListAPIView):
Eliminates duplicate project queries caused by calling get_absolute_url() in the embedded serializers. Following foreign keys with 'series__project' will cache the project of the series as well as the series itself. Signed-off-by: Mete Polat <metepolat2000@gmail.com> --- There are still some duplicates in various /api/ views but it looks like those are caused by the REST framework itself. patchwork/api/cover.py | 2 +- patchwork/api/event.py | 4 ++-- patchwork/api/patch.py | 2 +- patchwork/api/series.py | 5 +++-- 4 files changed, 7 insertions(+), 6 deletions(-)