diff mbox

xmlrpc: Don't load all Patches into memory

Message ID 1468403849-7325-1-git-send-email-stephen.finucane@intel.com
State Accepted
Headers show

Commit Message

Stephen Finucane July 13, 2016, 9:57 a.m. UTC
The way that reverse indexing of patches was implemented is broken.
At present, it will retrieve all patches in memory and return the
length from that data, then the slicing operation will then happen
without querying the DB and slice the results cached from the len()
evaluation. This is memory intensive, particularly for larger
instances.

Take advantage of Django's lazy loading to avoid this.

Signed-off-by: Stephen Finucane <stephen.finucane@intel.com>
Suggested-by: Damien Lespiau <damien.lespiau@intel.com>
---
 patchwork/views/xmlrpc.py |   14 ++++++++------
 1 files changed, 8 insertions(+), 6 deletions(-)

Comments

Andy Doan July 15, 2016, 4:31 p.m. UTC | #1
On 07/13/2016 04:57 AM, Stephen Finucane wrote:
> The way that reverse indexing of patches was implemented is broken.
> At present, it will retrieve all patches in memory and return the
> length from that data, then the slicing operation will then happen
> without querying the DB and slice the results cached from the len()
> evaluation. This is memory intensive, particularly for larger
> instances.
>
> Take advantage of Django's lazy loading to avoid this.
>
> Signed-off-by: Stephen Finucane <stephen.finucane@intel.com>
> Suggested-by: Damien Lespiau <damien.lespiau@intel.com>
> ---
Reviewed-by: Andy Doan <andy.doan@linaro.org>

>   patchwork/views/xmlrpc.py |   14 ++++++++------
>   1 files changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/patchwork/views/xmlrpc.py b/patchwork/views/xmlrpc.py
> index 638b3b1..8f759aa 100644
> --- a/patchwork/views/xmlrpc.py
> +++ b/patchwork/views/xmlrpc.py
> @@ -406,8 +406,8 @@ def project_list(search_str=None, max_count=0):
>           if max_count > 0:
>               return list(map(project_to_dict, projects[:max_count]))
>           elif max_count < 0:
> -            return list(map(project_to_dict,
> -                            projects[len(projects) + max_count:]))
> +            query = projects.reverse()[:-max_count]
> +            return list(map(project_to_dict, reversed(query)))
>           else:
>               return list(map(project_to_dict, projects))
>       except Project.DoesNotExist:
> @@ -461,7 +461,8 @@ def person_list(search_str=None, max_count=0):
>           if max_count > 0:
>               return list(map(person_to_dict, people[:max_count]))
>           elif max_count < 0:
> -            return list(map(person_to_dict, people[len(people) + max_count:]))
> +            query = people.reverse()[:-max_count]
> +            return list(map(person_to_dict, reversed(query)))
>           else:
>               return list(map(person_to_dict, people))
>       except Person.DoesNotExist:
> @@ -606,8 +607,8 @@ def patch_list(filt=None):
>           if max_count > 0:
>               return list(map(patch_to_dict, patches[:max_count]))
>           elif max_count < 0:
> -            return list(map(patch_to_dict,
> -                            patches[len(patches) + max_count:]))
> +            query = patches.reverse()[:-max_count]
> +            return list(map(patch_to_dict, reversed(query)))
>           else:
>               return list(map(patch_to_dict, patches))
>       except Patch.DoesNotExist:
> @@ -802,7 +803,8 @@ def state_list(search_str=None, max_count=0):
>           if max_count > 0:
>               return list(map(state_to_dict, states[:max_count]))
>           elif max_count < 0:
> -            return list(map(state_to_dict, states[len(states) + max_count:]))
> +            query = states.reverse()[:-max_count]
> +            return list(map(state_to_dict, reversed(query)))
>           else:
>               return list(map(state_to_dict, states))
>       except State.DoesNotExist:
>
Stephen Finucane Aug. 13, 2016, 10:51 p.m. UTC | #2
On 15 Jul 11:31, Andy Doan wrote:
> On 07/13/2016 04:57 AM, Stephen Finucane wrote:
> > The way that reverse indexing of patches was implemented is broken.
> > At present, it will retrieve all patches in memory and return the
> > length from that data, then the slicing operation will then happen
> > without querying the DB and slice the results cached from the len()
> > evaluation. This is memory intensive, particularly for larger
> > instances.
> > 
> > Take advantage of Django's lazy loading to avoid this.
> > 
> > Signed-off-by: Stephen Finucane <stephen.finucane@intel.com>
> > Suggested-by: Damien Lespiau <damien.lespiau@intel.com>
> > ---
> Reviewed-by: Andy Doan <andy.doan@linaro.org>

Merged.
diff mbox

Patch

diff --git a/patchwork/views/xmlrpc.py b/patchwork/views/xmlrpc.py
index 638b3b1..8f759aa 100644
--- a/patchwork/views/xmlrpc.py
+++ b/patchwork/views/xmlrpc.py
@@ -406,8 +406,8 @@  def project_list(search_str=None, max_count=0):
         if max_count > 0:
             return list(map(project_to_dict, projects[:max_count]))
         elif max_count < 0:
-            return list(map(project_to_dict,
-                            projects[len(projects) + max_count:]))
+            query = projects.reverse()[:-max_count]
+            return list(map(project_to_dict, reversed(query)))
         else:
             return list(map(project_to_dict, projects))
     except Project.DoesNotExist:
@@ -461,7 +461,8 @@  def person_list(search_str=None, max_count=0):
         if max_count > 0:
             return list(map(person_to_dict, people[:max_count]))
         elif max_count < 0:
-            return list(map(person_to_dict, people[len(people) + max_count:]))
+            query = people.reverse()[:-max_count]
+            return list(map(person_to_dict, reversed(query)))
         else:
             return list(map(person_to_dict, people))
     except Person.DoesNotExist:
@@ -606,8 +607,8 @@  def patch_list(filt=None):
         if max_count > 0:
             return list(map(patch_to_dict, patches[:max_count]))
         elif max_count < 0:
-            return list(map(patch_to_dict,
-                            patches[len(patches) + max_count:]))
+            query = patches.reverse()[:-max_count]
+            return list(map(patch_to_dict, reversed(query)))
         else:
             return list(map(patch_to_dict, patches))
     except Patch.DoesNotExist:
@@ -802,7 +803,8 @@  def state_list(search_str=None, max_count=0):
         if max_count > 0:
             return list(map(state_to_dict, states[:max_count]))
         elif max_count < 0:
-            return list(map(state_to_dict, states[len(states) + max_count:]))
+            query = states.reverse()[:-max_count]
+            return list(map(state_to_dict, reversed(query)))
         else:
             return list(map(state_to_dict, states))
     except State.DoesNotExist: