Speed up two-sided view for CL with large number of files

Speed up two-sided view for CL with large number of files.

This puts an arbitrary limit of 1000 files on a CL but Rietveld barely works
with 500 files in a CL.

[M-A, 2011-11-22 21:11:17]

I did this CL to figure out if is is faster to filter on ancestor than on a
member. In practice, it's hard to see if it's actually faster. I did a few
individual fetches and looked at appstat but the data is too noisy to tell
exactly, and data gathering is error prone.

---

My global thinking is that having the entity ancestor as a member is redundant.
Wouldn't it be cleaner to remove the double reference and just use ancestor when
needed?

This means removing Patch.patchset, Comment.patch, PatchSet.issue,
Message.issue. Instead, ancestor() could be used at each of these places. Any
opinion?

The main caveat is that the refactor needs to be done really thoroughly since
it's easy to miss a reference. :/

M-A

[gvrpython, 2011-11-22 21:26:58]

Hmm... will this still work in Andy's pure Django version of Rietveld?

On Tue, Nov 22, 2011 at 1:11 PM,  <maruel@chromium.org> wrote:
> Reviewers: Andi Albrecht,
>
> Message:
> I did this CL to figure out if is is faster to filter on ancestor than
> on a member. In practice, it's hard to see if it's actually faster. I
> did a few individual fetches and looked at appstat but the data is too
> noisy to tell exactly, and data gathering is error prone.
>
> ---
>
> My global thinking is that having the entity ancestor as a member is
> redundant. Wouldn't it be cleaner to remove the double reference and
> just use ancestor when needed?
>
> This means removing Patch.patchset, Comment.patch, PatchSet.issue,
> Message.issue. Instead, ancestor() could be used at each of these
> places. Any opinion?
>
> The main caveat is that the refactor needs to be done really thoroughly
> since it's easy to miss a reference. :/
>
> M-A
>
> Description:
> Speed up two-sided view for CL with large number of files.
>
> Note that it's the fetch(1000) calls that does the actual speed up. The
> index requirement change is to be discussed.
>
> Please review this at http://codereview.appspot.com/5430050/
>
> Affected files:
>  M codereview/views.py
>  M index.yaml
>
>
> Index: codereview/views.py
> ===================================================================
> --- a/codereview/views.py
> +++ b/codereview/views.py
> @@ -2631,8 +2631,8 @@
>  def _add_next_prev(patchset, patch):
>   """Helper to add .next and .prev attributes to a patch object."""
>   patch.prev = patch.next = None
> -  patches = list(models.Patch.gql("WHERE patchset = :1 ORDER BY filename",
> -                                  patchset))
> +  patches = models.Patch.all().ancestor(patchset.key()).order(
> +      'filename').fetch(1000)
>   patchset.patches = patches  # Required to render the jump to select.
>
>   comments_by_patch, drafts_by_patch = _get_comment_counts(
> Index: index.yaml
> ===================================================================
> --- a/index.yaml
> +++ b/index.yaml
> @@ -128,6 +128,11 @@
>   - name: patchset
>   - name: filename
>
> +- kind: Patch
> +  ancestor: yes
> +  properties:
> +  - name: filename
> +
>  - kind: PatchSet
>   properties:
>   - name: issue
>
>
> --
> You received this message because you are subscribed to the Google Groups
> "codereview-discuss" group.
> To post to this group, send email to codereview-discuss@googlegroups.com.
> To unsubscribe from this group, send email to
> codereview-discuss+unsubscribe@googlegroups.com.
> For more options, visit this group at
> http://groups.google.com/group/codereview-discuss?hl=en.
>
>



-- 
--Guido van Rossum (python.org/~guido)

[Andi Albrecht, 2011-11-23 07:26:31]

LGTM

On Tue, Nov 22, 2011 at 10:26 PM, Guido van Rossum <guido@python.org> wrote:
> Hmm... will this still work in Andy's pure Django version of Rietveld?

There was a bug where the ancestor() function didn't accept Key
instances and didn't returned a query instance. But this is fixed now.
Thanks for the hint :)

>
> On Tue, Nov 22, 2011 at 1:11 PM,  <maruel@chromium.org> wrote:
>> Reviewers: Andi Albrecht,
>>
>> Message:
>> I did this CL to figure out if is is faster to filter on ancestor than
>> on a member. In practice, it's hard to see if it's actually faster. I
>> did a few individual fetches and looked at appstat but the data is too
>> noisy to tell exactly, and data gathering is error prone.
>>
>> ---
>>
>> My global thinking is that having the entity ancestor as a member is
>> redundant. Wouldn't it be cleaner to remove the double reference and
>> just use ancestor when needed?
>>
>> This means removing Patch.patchset, Comment.patch, PatchSet.issue,
>> Message.issue. Instead, ancestor() could be used at each of these
>> places. Any opinion?
>>
>> The main caveat is that the refactor needs to be done really thoroughly
>> since it's easy to miss a reference. :/
>>
>> M-A
>>
>> Description:
>> Speed up two-sided view for CL with large number of files.
>>
>> Note that it's the fetch(1000) calls that does the actual speed up. The
>> index requirement change is to be discussed.
>>
>> Please review this at http://codereview.appspot.com/5430050/
>>
>> Affected files:
>>  M codereview/views.py
>>  M index.yaml
>>
>>
>> Index: codereview/views.py
>> ===================================================================
>> --- a/codereview/views.py
>> +++ b/codereview/views.py
>> @@ -2631,8 +2631,8 @@
>>  def _add_next_prev(patchset, patch):
>>   """Helper to add .next and .prev attributes to a patch object."""
>>   patch.prev = patch.next = None
>> -  patches = list(models.Patch.gql("WHERE patchset = :1 ORDER BY filename",
>> -                                  patchset))
>> +  patches = models.Patch.all().ancestor(patchset.key()).order(
>> +      'filename').fetch(1000)
>>   patchset.patches = patches  # Required to render the jump to select.
>>
>>   comments_by_patch, drafts_by_patch = _get_comment_counts(
>> Index: index.yaml
>> ===================================================================
>> --- a/index.yaml
>> +++ b/index.yaml
>> @@ -128,6 +128,11 @@
>>   - name: patchset
>>   - name: filename
>>
>> +- kind: Patch
>> +  ancestor: yes
>> +  properties:
>> +  - name: filename
>> +
>>  - kind: PatchSet
>>   properties:
>>   - name: issue
>>
>>
>> --
>> You received this message because you are subscribed to the Google Groups
>> "codereview-discuss" group.
>> To post to this group, send email to codereview-discuss@googlegroups.com.
>> To unsubscribe from this group, send email to
>> codereview-discuss+unsubscribe@googlegroups.com.
>> For more options, visit this group at
>> http://groups.google.com/group/codereview-discuss?hl=en.
>>
>>
>
>
>
> --
> --Guido van Rossum (python.org/~guido)
>

[M-A, 2011-11-23 16:16:45]

Removed all the ancestor stuff, switched from GQL to a Query but beside that,
left the filtering the same.

I guess .key() could be removed but it helps making sure it's not None.

[Andi Albrecht, 2011-11-23 18:46:09]

On Wed, Nov 23, 2011 at 5:16 PM,  <maruel@chromium.org> wrote:
> Removed all the ancestor stuff, switched from GQL to a Query but beside
> that, left the filtering the same.

Looks good.
The ancestor() query would be fine now for gae2django too.

>
> I guess .key() could be removed but it helps making sure it's not None.
>
> http://codereview.appspot.com/5430050/
>