Patchwork [1,of,2] localrepo: cache self.changelog in local variable

login
register
mail settings
Submitter Stanislau Hlebik
Date Feb. 13, 2017, 10:32 a.m.
Message ID <8bc5ae6cf51408dbd1b7.1486981977@devvm1840.lla2.facebook.com>
Download mbox | patch
Permalink /patch/18436/
State Accepted
Headers show

Comments

Stanislau Hlebik - Feb. 13, 2017, 10:32 a.m.
# HG changeset patch
# User Stanislau Hlebik <stash@fb.com>
# Date 1486981578 28800
#      Mon Feb 13 02:26:18 2017 -0800
# Node ID 8bc5ae6cf51408dbd1b789555196f031bfef19d4
# Parent  a0e3d808690d57d1c9dff840e0b8ee099526397b
localrepo: cache self.changelog in local variable

Repeated self.changelog lookups can incur overhead. Let's cache it in the
separate variable.
Yuya Nishihara - Feb. 13, 2017, 2:08 p.m.
On Mon, 13 Feb 2017 02:32:57 -0800, Stanislau Hlebik wrote:
> # HG changeset patch
> # User Stanislau Hlebik <stash@fb.com>
> # Date 1486981578 28800
> #      Mon Feb 13 02:26:18 2017 -0800
> # Node ID 8bc5ae6cf51408dbd1b789555196f031bfef19d4
> # Parent  a0e3d808690d57d1c9dff840e0b8ee099526397b
> localrepo: cache self.changelog in local variable

Queued them, thanks!
Bryan O'Sullivan - Feb. 13, 2017, 6:20 p.m.
On Mon, Feb 13, 2017 at 2:32 AM, Stanislau Hlebik <stash@fb.com> wrote:

> localrepo: cache self.changelog in local variable
>
> Repeated self.changelog lookups can incur overhead. Let's cache it in the
> separate variable.
>

I'm not suggesting that you should change this since Yuya has already
queued it, but a few notes for the future...


> diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
> --- a/mercurial/localrepo.py
> +++ b/mercurial/localrepo.py
> @@ -1853,8 +1853,9 @@
>
>      def heads(self, start=None):
>          if start is None:
> -            headrevs = sorted(self.changelog.headrevs(), reverse=True)
> -            return [self.changelog.node(rev) for rev in headrevs]
> +            cl = self.changelog
> +            headrevs = sorted(cl.headrevs(), reverse=True)
> +            return [cl.node(rev) for rev in headrevs]
>
>          heads = self.changelog.heads(start)
>          # sort the output in rev descending order
>

For local nano-optimizations like this, it's worth being clear about
whether you saw this show up in a performance profile or not. (And if you
did, to explain why.) Most of the other changes of this form have happened
because of observed performance effects, but as these edits harm
readability a little, they deserve at least a little explicit justification.

Since the only place that the chained access could be hurting in your patch
is inside the list comprehension, you could further micro-optimize away
another chained access:

node = self.changelog.node
headrevs = sorted(self.changelog.headrevs(), reverse=True)
return [node(rev) for rev in headrevs]

Patch

diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
--- a/mercurial/localrepo.py
+++ b/mercurial/localrepo.py
@@ -1853,8 +1853,9 @@ 
 
     def heads(self, start=None):
         if start is None:
-            headrevs = sorted(self.changelog.headrevs(), reverse=True)
-            return [self.changelog.node(rev) for rev in headrevs]
+            cl = self.changelog
+            headrevs = sorted(cl.headrevs(), reverse=True)
+            return [cl.node(rev) for rev in headrevs]
 
         heads = self.changelog.heads(start)
         # sort the output in rev descending order