Patchwork [7,of,7] revlog: avoid apply delta chain when cache hit in revlog.revision

login
register
mail settings
Submitter Jun Wu
Date March 28, 2017, 7:49 a.m.
Message ID <8c9f728ef3a3fff029d7.1490687348@localhost.localdomain>
Download mbox | patch
Permalink /patch/19777/
State Changes Requested
Headers show

Comments

Jun Wu - March 28, 2017, 7:49 a.m.
# HG changeset patch
# User Jun Wu <quark@fb.com>
# Date 1490683696 25200
#      Mon Mar 27 23:48:16 2017 -0700
# Node ID 8c9f728ef3a3fff029d7fe6c875ed783c66dc254
# Parent  d3d803ed16fe8e9d43f7a4daeca079e4022c297a
# Available At https://bitbucket.org/quark-zju/hg-draft
#              hg pull https://bitbucket.org/quark-zju/hg-draft -r 8c9f728ef3a3
revlog: avoid apply delta chain when cache hit in revlog.revision

Previously, revlog.revision(raw=False) may try to apply the delta chain
even on cache hit. That could happen if flags are non-empty so all fast
paths won't be executed. But if cache hit, the raw test is still useful.
So let's use it.
Ryan McElroy - March 29, 2017, 12:41 p.m.
On 3/28/17 8:49 AM, Jun Wu wrote:
> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1490683696 25200
> #      Mon Mar 27 23:48:16 2017 -0700
> # Node ID 8c9f728ef3a3fff029d7fe6c875ed783c66dc254
> # Parent  d3d803ed16fe8e9d43f7a4daeca079e4022c297a
> revlog: avoid apply delta chain when cache hit in revlog.revision
>
> Previously, revlog.revision(raw=False) may try to apply the delta chain
> even on cache hit. That could happen if flags are non-empty so all fast
> paths won't be executed. But if cache hit, the raw test is still useful.
> So let's use it.
>
> diff --git a/mercurial/revlog.py b/mercurial/revlog.py
> --- a/mercurial/revlog.py
> +++ b/mercurial/revlog.py
> @@ -1259,19 +1259,20 @@ class revlog(object):
>           cachedrev = None
>           flags = None
> +        text = None
>           if node == nullid:
>               return ""
>           if self._cache:
>               if self._cache[0] == node:
> +                text = self._cache[2]
>                   if raw:
> -                    return self._cache[2]
> +                    return text
>                   if rev is None:
>                       rev = self.rev(node)
>                   flags = self.flags(rev)
>                   if not flags:
> -                    return self._cache[2]
> +                    return text
>               cachedrev = self._cache[1]
>   
>           # look up what we need to read
> -        text = None
>           if rev is None:
>               rev = self.rev(node)
>

This one lgtm

This overall series is a bit hard to review mostly because the code 
you're modifying is pretty dense. The patches themselves made a lot of 
sense once I dove into the code, though.

I have enough review comments that I think it's obvious that I would 
like to see a second version (after you've responded to anything you 
don't think should change and we've had that discussion, if needed). 
Also feel free to "respond" by adding comments in a v2 where I asked 
about things and you think they should stay the same.

Overall, these changes clearly correct some pretty nasty bugs that would 
bite us badly when we start using flag processing. Thanks for diving 
into this tricky code area and fixing them, and adding a good test to 
prevent regressions.

I'll mark these are "changes requested" in patchview in anticipation of 
a V2.

Patch

diff --git a/mercurial/revlog.py b/mercurial/revlog.py
--- a/mercurial/revlog.py
+++ b/mercurial/revlog.py
@@ -1259,19 +1259,20 @@  class revlog(object):
         cachedrev = None
         flags = None
+        text = None
         if node == nullid:
             return ""
         if self._cache:
             if self._cache[0] == node:
+                text = self._cache[2]
                 if raw:
-                    return self._cache[2]
+                    return text
                 if rev is None:
                     rev = self.rev(node)
                 flags = self.flags(rev)
                 if not flags:
-                    return self._cache[2]
+                    return text
             cachedrev = self._cache[1]
 
         # look up what we need to read
-        text = None
         if rev is None:
             rev = self.rev(node)