Patchwork [4,of,8] revlog: more efficient implementation for issnapshot

login
register
mail settings
Submitter Boris Feld
Date Dec. 17, 2018, noon
Message ID <9fe9cc49f235311269fd.1545048046@localhost.localdomain>
Download mbox | patch
Permalink /patch/37220/
State Accepted
Headers show

Comments

Boris Feld - Dec. 17, 2018, noon
# HG changeset patch
# User Boris Feld <boris.feld@octobus.net>
# Date 1545040296 -3600
#      Mon Dec 17 10:51:36 2018 +0100
# Node ID 9fe9cc49f235311269fd957c49898396ed7bdfc0
# Parent  4cafb262b243b02c217fb538165e354f77ce0fd8
# EXP-Topic sparse-revlog-corner-cases
# Available At https://bitbucket.org/octobus/mercurial-devel/
#              hg pull https://bitbucket.org/octobus/mercurial-devel/ -r 9fe9cc49f235
revlog: more efficient implementation for issnapshot

We avoid multiple method calls and tuple creation, this provides a significant
speedup in some case:

example affected manifest write
before: 0.815520s
after:  0.487767s (-40%)
Yuya Nishihara - Dec. 17, 2018, 12:45 p.m.
On Mon, 17 Dec 2018 12:00:46 +0000, Boris Feld wrote:
> # HG changeset patch
> # User Boris Feld <boris.feld@octobus.net>
> # Date 1545040296 -3600
> #      Mon Dec 17 10:51:36 2018 +0100
> # Node ID 9fe9cc49f235311269fd957c49898396ed7bdfc0
> # Parent  4cafb262b243b02c217fb538165e354f77ce0fd8
> # EXP-Topic sparse-revlog-corner-cases
> # Available At https://bitbucket.org/octobus/mercurial-devel/
> #              hg pull https://bitbucket.org/octobus/mercurial-devel/ -r 9fe9cc49f235
> revlog: more efficient implementation for issnapshot
> 
> We avoid multiple method calls and tuple creation, this provides a significant
> speedup in some case:
> 
> example affected manifest write
> before: 0.815520s
> after:  0.487767s (-40%)
> 
> diff --git a/mercurial/revlog.py b/mercurial/revlog.py
> --- a/mercurial/revlog.py
> +++ b/mercurial/revlog.py
> @@ -1535,11 +1535,17 @@ class revlog(object):
>          """
>          if rev == nullrev:
>              return True
> -        deltap = self.deltaparent(rev)
> +        entry = self.index[rev]
> +        deltap = entry[3]

Shouldn't it be called a "base" ?
Yuya Nishihara - Dec. 17, 2018, 2:14 p.m.
On Mon, 17 Dec 2018 12:00:46 +0000, Boris Feld wrote:
> # HG changeset patch
> # User Boris Feld <boris.feld@octobus.net>
> # Date 1545040296 -3600
> #      Mon Dec 17 10:51:36 2018 +0100
> # Node ID 9fe9cc49f235311269fd957c49898396ed7bdfc0
> # Parent  4cafb262b243b02c217fb538165e354f77ce0fd8
> # EXP-Topic sparse-revlog-corner-cases
> # Available At https://bitbucket.org/octobus/mercurial-devel/
> #              hg pull https://bitbucket.org/octobus/mercurial-devel/ -r 9fe9cc49f235
> revlog: more efficient implementation for issnapshot
> 
> We avoid multiple method calls and tuple creation, this provides a significant
> speedup in some case:
> 
> example affected manifest write
> before: 0.815520s
> after:  0.487767s (-40%)
> 
> diff --git a/mercurial/revlog.py b/mercurial/revlog.py
> --- a/mercurial/revlog.py
> +++ b/mercurial/revlog.py
> @@ -1535,11 +1535,17 @@ class revlog(object):
>          """
>          if rev == nullrev:
>              return True
> -        deltap = self.deltaparent(rev)
> +        entry = self.index[rev]
> +        deltap = entry[3]
> +        if deltap == rev:
> +            return True
> +        if not self._generaldelta:
> +            return False

I'm not sure if I understand it correctly, but before, deltap was rev - 1
if generaldelta off, and if it wasn't p1 nor p2, issnapshot() would be
called recursively.

>          if deltap == nullrev:
>              return True
> -        p1, p2 = self.parentrevs(rev)
> -        if deltap in (p1, p2):
> +        p1 = entry[5]
> +        p2 = entry[6]
> +        if deltap == p1 or deltap == p2:
>              return False
>          return self.issnapshot(deltap)
>
Boris Feld - Dec. 21, 2018, 11:29 a.m.
On 17/12/2018 15:14, Yuya Nishihara wrote:
> On Mon, 17 Dec 2018 12:00:46 +0000, Boris Feld wrote:
>> # HG changeset patch
>> # User Boris Feld <boris.feld@octobus.net>
>> # Date 1545040296 -3600
>> #      Mon Dec 17 10:51:36 2018 +0100
>> # Node ID 9fe9cc49f235311269fd957c49898396ed7bdfc0
>> # Parent  4cafb262b243b02c217fb538165e354f77ce0fd8
>> # EXP-Topic sparse-revlog-corner-cases
>> # Available At https://bitbucket.org/octobus/mercurial-devel/
>> #              hg pull https://bitbucket.org/octobus/mercurial-devel/ -r 9fe9cc49f235
>> revlog: more efficient implementation for issnapshot
>>
>> We avoid multiple method calls and tuple creation, this provides a significant
>> speedup in some case:
>>
>> example affected manifest write
>> before: 0.815520s
>> after:  0.487767s (-40%)
>>
>> diff --git a/mercurial/revlog.py b/mercurial/revlog.py
>> --- a/mercurial/revlog.py
>> +++ b/mercurial/revlog.py
>> @@ -1535,11 +1535,17 @@ class revlog(object):
>>          """
>>          if rev == nullrev:
>>              return True
>> -        deltap = self.deltaparent(rev)
>> +        entry = self.index[rev]
>> +        deltap = entry[3]
>> +        if deltap == rev:
>> +            return True
>> +        if not self._generaldelta:
>> +            return False
> I'm not sure if I understand it correctly, but before, deltap was rev - 1
> if generaldelta off, and if it wasn't p1 nor p2, issnapshot() would be
> called recursively.

Hum, good point. That was "wrong" since before general delta, snapshots
are only the initial full snapshot.

This was not a big issue as the "isnapshot" code is not much triggered
when sparse-revlog is not used.

>
>>          if deltap == nullrev:
>>              return True
>> -        p1, p2 = self.parentrevs(rev)
>> -        if deltap in (p1, p2):
>> +        p1 = entry[5]
>> +        p2 = entry[6]
>> +        if deltap == p1 or deltap == p2:
>>              return False
>>          return self.issnapshot(deltap)
>>  
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Boris Feld - Dec. 21, 2018, 11:30 a.m.
On 17/12/2018 13:45, Yuya Nishihara wrote:
> On Mon, 17 Dec 2018 12:00:46 +0000, Boris Feld wrote:
>> # HG changeset patch
>> # User Boris Feld <boris.feld@octobus.net>
>> # Date 1545040296 -3600
>> #      Mon Dec 17 10:51:36 2018 +0100
>> # Node ID 9fe9cc49f235311269fd957c49898396ed7bdfc0
>> # Parent  4cafb262b243b02c217fb538165e354f77ce0fd8
>> # EXP-Topic sparse-revlog-corner-cases
>> # Available At https://bitbucket.org/octobus/mercurial-devel/
>> #              hg pull https://bitbucket.org/octobus/mercurial-devel/ -r 9fe9cc49f235
>> revlog: more efficient implementation for issnapshot
>>
>> We avoid multiple method calls and tuple creation, this provides a significant
>> speedup in some case:
>>
>> example affected manifest write
>> before: 0.815520s
>> after:  0.487767s (-40%)
>>
>> diff --git a/mercurial/revlog.py b/mercurial/revlog.py
>> --- a/mercurial/revlog.py
>> +++ b/mercurial/revlog.py
>> @@ -1535,11 +1535,17 @@ class revlog(object):
>>          """
>>          if rev == nullrev:
>>              return True
>> -        deltap = self.deltaparent(rev)
>> +        entry = self.index[rev]
>> +        deltap = entry[3]
> Shouldn't it be called a "base" ?

It could be called base. However, the previous code used "deltap". I
would rather not mix the two changes. I can do the modification in
another changeset.

> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Yuya Nishihara - Dec. 22, 2018, 3:38 a.m.
On Fri, 21 Dec 2018 12:30:14 +0100, Boris FELD wrote:
> On 17/12/2018 13:45, Yuya Nishihara wrote:
> > On Mon, 17 Dec 2018 12:00:46 +0000, Boris Feld wrote:
> >> # HG changeset patch
> >> # User Boris Feld <boris.feld@octobus.net>
> >> # Date 1545040296 -3600
> >> #      Mon Dec 17 10:51:36 2018 +0100
> >> # Node ID 9fe9cc49f235311269fd957c49898396ed7bdfc0
> >> # Parent  4cafb262b243b02c217fb538165e354f77ce0fd8
> >> # EXP-Topic sparse-revlog-corner-cases
> >> # Available At https://bitbucket.org/octobus/mercurial-devel/
> >> #              hg pull https://bitbucket.org/octobus/mercurial-devel/ -r 9fe9cc49f235
> >> revlog: more efficient implementation for issnapshot
> >>
> >> We avoid multiple method calls and tuple creation, this provides a significant
> >> speedup in some case:
> >>
> >> example affected manifest write
> >> before: 0.815520s
> >> after:  0.487767s (-40%)
> >>
> >> diff --git a/mercurial/revlog.py b/mercurial/revlog.py
> >> --- a/mercurial/revlog.py
> >> +++ b/mercurial/revlog.py
> >> @@ -1535,11 +1535,17 @@ class revlog(object):
> >>          """
> >>          if rev == nullrev:
> >>              return True
> >> -        deltap = self.deltaparent(rev)
> >> +        entry = self.index[rev]
> >> +        deltap = entry[3]
> > Shouldn't it be called a "base" ?
> 
> It could be called base. However, the previous code used "deltap". I
> would rather not mix the two changes.

Previously it was a delta parent, but entry[3] isn't always a delta parent
if I understand it correctly.

> I can do the modification in
> another changeset.

Yup. Please send a follow up.
Yuya Nishihara - Dec. 22, 2018, 3:40 a.m.
On Fri, 21 Dec 2018 12:29:12 +0100, Boris FELD wrote:
> 
> On 17/12/2018 15:14, Yuya Nishihara wrote:
> > On Mon, 17 Dec 2018 12:00:46 +0000, Boris Feld wrote:
> >> # HG changeset patch
> >> # User Boris Feld <boris.feld@octobus.net>
> >> # Date 1545040296 -3600
> >> #      Mon Dec 17 10:51:36 2018 +0100
> >> # Node ID 9fe9cc49f235311269fd957c49898396ed7bdfc0
> >> # Parent  4cafb262b243b02c217fb538165e354f77ce0fd8
> >> # EXP-Topic sparse-revlog-corner-cases
> >> # Available At https://bitbucket.org/octobus/mercurial-devel/
> >> #              hg pull https://bitbucket.org/octobus/mercurial-devel/ -r 9fe9cc49f235
> >> revlog: more efficient implementation for issnapshot
> >>
> >> We avoid multiple method calls and tuple creation, this provides a significant
> >> speedup in some case:
> >>
> >> example affected manifest write
> >> before: 0.815520s
> >> after:  0.487767s (-40%)
> >>
> >> diff --git a/mercurial/revlog.py b/mercurial/revlog.py
> >> --- a/mercurial/revlog.py
> >> +++ b/mercurial/revlog.py
> >> @@ -1535,11 +1535,17 @@ class revlog(object):
> >>          """
> >>          if rev == nullrev:
> >>              return True
> >> -        deltap = self.deltaparent(rev)
> >> +        entry = self.index[rev]
> >> +        deltap = entry[3]
> >> +        if deltap == rev:
> >> +            return True
> >> +        if not self._generaldelta:
> >> +            return False
> > I'm not sure if I understand it correctly, but before, deltap was rev - 1
> > if generaldelta off, and if it wasn't p1 nor p2, issnapshot() would be
> > called recursively.
> 
> Hum, good point. That was "wrong" since before general delta, snapshots
> are only the initial full snapshot.
> 
> This was not a big issue as the "isnapshot" code is not much triggered
> when sparse-revlog is not used.

Can you include it in the commit message?

Patch

diff --git a/mercurial/revlog.py b/mercurial/revlog.py
--- a/mercurial/revlog.py
+++ b/mercurial/revlog.py
@@ -1535,11 +1535,17 @@  class revlog(object):
         """
         if rev == nullrev:
             return True
-        deltap = self.deltaparent(rev)
+        entry = self.index[rev]
+        deltap = entry[3]
+        if deltap == rev:
+            return True
+        if not self._generaldelta:
+            return False
         if deltap == nullrev:
             return True
-        p1, p2 = self.parentrevs(rev)
-        if deltap in (p1, p2):
+        p1 = entry[5]
+        p2 = entry[6]
+        if deltap == p1 or deltap == p2:
             return False
         return self.issnapshot(deltap)