Patchwork [4,of,4,V2] annotate: simplify annotate parent function

login
register
mail settings
Submitter Durham Goode
Date June 4, 2013, 7:59 p.m.
Message ID <d0a0580f790466ff5566.1370375970@dev350.prn1.facebook.com>
Download mbox | patch
Permalink /patch/1699/
State Accepted
Commit e0aa6fff8f0241a0b655a15a3856b9ddf8bb0cb3
Headers show

Comments

Durham Goode - June 4, 2013, 7:59 p.m.
# HG changeset patch
# User Durham Goode <durham@fb.com>
# Date 1369967343 25200
#      Thu May 30 19:29:03 2013 -0700
# Node ID d0a0580f790466ff5566682bb7b29d8623acb1da
# Parent  a1a98fba2858dab9aa58431f4e4991f3d91649ab
annotate: simplify annotate parent function

The annotate algorithm used a custom parents() function to try to reuse
filectx and filelogs. I simplified it a bit to rely more heavily on the
self.parents() which makes it work well with alternative filectx
implementations. I tested performance on a file with 5000+ revisions
but no renames, and on a file with 500 revisions repeating a series of
4 edits+renames and saw zero performance hit.  In fact, it was reliably a
couple milliseconds faster now.

Added the perfannotate command to contrib/perf.py for future use.
Matt Mackall - June 5, 2013, 8:47 p.m.
On Tue, 2013-06-04 at 12:59 -0700, Durham Goode wrote:
> # HG changeset patch
> # User Durham Goode <durham@fb.com>
> # Date 1369967343 25200
> #      Thu May 30 19:29:03 2013 -0700
> # Node ID d0a0580f790466ff5566682bb7b29d8623acb1da
> # Parent  a1a98fba2858dab9aa58431f4e4991f3d91649ab
> annotate: simplify annotate parent function

These are queued for default, thanks.
Durham Goode - June 6, 2013, 5:58 p.m.
On 6/5/13 1:47 PM, "Matt Mackall" <mpm@selenic.com> wrote:

>On Tue, 2013-06-04 at 12:59 -0700, Durham Goode wrote:
>> # HG changeset patch
>> # User Durham Goode <durham@fb.com>
>> # Date 1369967343 25200
>> #      Thu May 30 19:29:03 2013 -0700
>> # Node ID d0a0580f790466ff5566682bb7b29d8623acb1da
>> # Parent  a1a98fba2858dab9aa58431f4e4991f3d91649ab
>> annotate: simplify annotate parent function
>
>These are queued for default, thanks.
>

After pulling, a few of my patches don't seem to have been applied. I
don't see these:

filelog: switch 'not len(filerevlog)' to 'not filerevlog'
filectx: remove dependencies on filerev
merge: use nullid instead of null rev

The final 'annotate: simplify annotate parent function' patch is applied,
which makes me think these three were missed.  Or perhaps they weren't
queued to begin with and I misunderstood.

Durham
Matt Mackall - June 6, 2013, 6:21 p.m.
On Thu, 2013-06-06 at 17:58 +0000, Durham Goode wrote:
> On 6/5/13 1:47 PM, "Matt Mackall" <mpm@selenic.com> wrote:
> 
> >On Tue, 2013-06-04 at 12:59 -0700, Durham Goode wrote:
> >> # HG changeset patch
> >> # User Durham Goode <durham@fb.com>
> >> # Date 1369967343 25200
> >> #      Thu May 30 19:29:03 2013 -0700
> >> # Node ID d0a0580f790466ff5566682bb7b29d8623acb1da
> >> # Parent  a1a98fba2858dab9aa58431f4e4991f3d91649ab
> >> annotate: simplify annotate parent function
> >
> >These are queued for default, thanks.
> >
> 
> After pulling, a few of my patches don't seem to have been applied. I
> don't see these:
> 
> filelog: switch 'not len(filerevlog)' to 'not filerevlog'
> filectx: remove dependencies on filerev
> merge: use nullid instead of null rev
> 
> The final 'annotate: simplify annotate parent function' patch is applied,
> which makes me think these three were missed.  Or perhaps they weren't
> queued to begin with and I misunderstood.

Hmm, let's ask blackbox:


2013/06/05 15:37:37 mpm> qimport /home/mpm/patches/v2-filelog-switch-not-len-filerevlog-to-not.patch
2013/06/05 15:37:37 mpm> qimport /home/mpm/patches/v2-filelog-switch-not-len-filerevlog-to-not.patch exit
ed 0 after 0.04 seconds
2013/06/05 15:37:37 mpm> qpush
2013/06/05 15:37:37 mpm> qpush exited -1 after 0.11 seconds
2013/06/05 15:37:46 mpm> revert -a
2013/06/05 15:37:46 mpm> revert -a exited 0 after 0.14 seconds
2013/06/05 15:37:47 mpm> qa
2013/06/05 15:37:47 mpm> qa exited 0 after 0.03 seconds
2013/06/05 15:37:54 mpm> export
2013/06/05 15:37:55 mpm> export exited 0 after 0.11 seconds
2013/06/05 15:38:07 mpm> qpop
2013/06/05 15:38:08 mpm> qpop exited 0 after 0.17 seconds
2013/06/05 15:45:44 mpm> qimport /home/mpm/patches/v2-log-move-log-file-walk-to-its-own-function.patch
2013/06/05 15:45:44 mpm> qimport /home/mpm/patches/v2-log-move-log-file-walk-to-its-own-function.patch ex
ited 0 after 0.04 seconds
2013/06/05 15:45:44 mpm> qpush
2013/06/05 15:45:44 mpm> qpush exited 0 after 0.14 seconds
2013/06/05 15:46:11 mpm> qimport /home/mpm/patches/v2-changegroup-move-changegroup-file-adding-to-a.patch
2013/06/05 15:46:11 mpm> qimport /home/mpm/patches/v2-changegroup-move-changegroup-file-adding-to-a.patch
 exited 0 after 0.05 seconds
2013/06/05 15:46:11 mpm> qpush
2013/06/05 15:46:11 mpm> qpush exited 0 after 0.15 seconds
2013/06/05 15:46:45 mpm> qimport /home/mpm/patches/v2-annotate-simplify-annotate-parent-function.patch
2013/06/05 15:46:45 mpm> qimport /home/mpm/patches/v2-annotate-simplify-annotate-parent-function.patch ex
ited 0 after 0.05 seconds
2013/06/05 15:46:45 mpm> qpush

Looks like I ran into trouble queueing the first of four that I acked,
and failed to qpush it again. Will fix. I don't think I acked the other
two from your previous series.
Durham Goode - June 6, 2013, 6:32 p.m.
On 6/6/13 11:21 AM, "Matt Mackall" <mpm@selenic.com> wrote:

>On Thu, 2013-06-06 at 17:58 +0000, Durham Goode wrote:
>> On 6/5/13 1:47 PM, "Matt Mackall" <mpm@selenic.com> wrote:
>> 
>> >On Tue, 2013-06-04 at 12:59 -0700, Durham Goode wrote:
>> >> # HG changeset patch
>> >> # User Durham Goode <durham@fb.com>
>> >> # Date 1369967343 25200
>> >> #      Thu May 30 19:29:03 2013 -0700
>> >> # Node ID d0a0580f790466ff5566682bb7b29d8623acb1da
>> >> # Parent  a1a98fba2858dab9aa58431f4e4991f3d91649ab
>> >> annotate: simplify annotate parent function
>> >
>> >These are queued for default, thanks.
>> >
>> 
>>
>> 
>> filelog: switch 'not len(filerevlog)' to 'not filerevlog'
>> filectx: remove dependencies on filerev
>> merge: use nullid instead of null rev
>> 
>>
>
>Looks like I ran into trouble queueing the first of four that I acked,
>and failed to qpush it again. Will fix. I don't think I acked the other
>two from your previous series.

For the two from the previous series, I assumed when you acked the third
commit in the series it included the first two. Are there changes needed
for those two?
Matt Mackall - June 6, 2013, 8:09 p.m.
On Thu, 2013-06-06 at 18:32 +0000, Durham Goode wrote:
> On 6/6/13 11:21 AM, "Matt Mackall" <mpm@selenic.com> wrote:
> 
> >On Thu, 2013-06-06 at 17:58 +0000, Durham Goode wrote:
> >> On 6/5/13 1:47 PM, "Matt Mackall" <mpm@selenic.com> wrote:
> >> 
> >> >On Tue, 2013-06-04 at 12:59 -0700, Durham Goode wrote:
> >> >> # HG changeset patch
> >> >> # User Durham Goode <durham@fb.com>
> >> >> # Date 1369967343 25200
> >> >> #      Thu May 30 19:29:03 2013 -0700
> >> >> # Node ID d0a0580f790466ff5566682bb7b29d8623acb1da
> >> >> # Parent  a1a98fba2858dab9aa58431f4e4991f3d91649ab
> >> >> annotate: simplify annotate parent function
> >> >
> >> >These are queued for default, thanks.
> >> >
> >> 
> >>
> >> 
> >> filelog: switch 'not len(filerevlog)' to 'not filerevlog'
> >> filectx: remove dependencies on filerev
> >> merge: use nullid instead of null rev
> >> 
> >>
> >
> >Looks like I ran into trouble queueing the first of four that I acked,
> >and failed to qpush it again. Will fix. I don't think I acked the other
> >two from your previous series.
> 
> For the two from the previous series, I assumed when you acked the third
> commit in the series it included the first two. Are there changes needed
> for those two?

They need a bit more mulling of this end, as they're poking at fast-path
micro-optimizations.

Patch

diff --git a/contrib/perf.py b/contrib/perf.py
--- a/contrib/perf.py
+++ b/contrib/perf.py
@@ -45,6 +45,11 @@ 
         except Exception:
             timer(lambda: len(list(cmdutil.walk(repo, pats, {}))))
 
+@command('perfannotate')
+def perfannotate(ui, repo, f):
+    fc = repo['.'][f]
+    timer(lambda: len(fc.annotate(True)))
+
 @command('perfstatus',
          [('u', 'unknown', False,
            'ask status to look for unknown files')])
diff --git a/mercurial/context.py b/mercurial/context.py
--- a/mercurial/context.py
+++ b/mercurial/context.py
@@ -649,25 +649,21 @@ 
             return child
 
         getlog = util.lrucachefunc(lambda x: self._repo.file(x))
-        def getctx(path, fileid):
-            log = path == self._path and self._filelog or getlog(path)
-            return filectx(self._repo, path, fileid=fileid, filelog=log)
-        getctx = util.lrucachefunc(getctx)
 
         def parents(f):
-            # we want to reuse filectx objects as much as possible
-            p = f._path
-            if f._filerev is None: # working dir
-                pl = [(n.path(), n.filerev()) for n in f.parents()]
-            else:
-                pl = [(p, n) for n in f._filelog.parentrevs(f._filerev)]
+            pl = f.parents()
 
-            if follow:
-                r = f.renamed()
-                if r:
-                    pl[0] = (r[0], getlog(r[0]).rev(r[1]))
+            # Don't return renamed parents if we aren't following.
+            if not follow:
+                pl = [p for p in pl if p.path() == f.path()]
 
-            return [getctx(p, n) for p, n in pl if n != nullrev]
+            # renamed filectx won't have a filelog yet, so set it
+            # from the cache to save time
+            for p in pl:
+                if not '_filelog' in p.__dict__:
+                    p._filelog = getlog(p.path())
+
+            return pl
 
         # use linkrev to find the first changeset where self appeared
         if self.rev() != self.linkrev():