Patchwork changelog: load pending file directly

login
register
mail settings
Submitter Gregory Szorc
Date May 13, 2017, 11:29 p.m.
Message ID <53a93d88e75d42e3e71c.1494718190@ubuntu-vm-main>
Download mbox | patch
Permalink /patch/20617/
State Accepted
Headers show

Comments

Gregory Szorc - May 13, 2017, 11:29 p.m.
# HG changeset patch
# User Gregory Szorc <gregory.szorc@gmail.com>
# Date 1494718003 25200
#      Sat May 13 16:26:43 2017 -0700
# Node ID 53a93d88e75d42e3e71c821154ccc6b5ed4fd349
# Parent  78496ac300255e9996b3e282086661afc08af37c
changelog: load pending file directly

When changelogs are written, a copy of the index (or inline revlog)
may be written to an 00changelog.i.a file to facilitate hooks and
other processes having access to the pending data before it is
finalized.

The way it works today, the localrepo class loads the changelog
like normal. Then, if it detects a pending transaction, it asks
the changelog class to load a pending changelog. The changelog
class looks for a 00changelog.i.a file. If it exists, it is
loaded and internal data structures on the new revlog class are
copied to the original instance.

The existing mechanism is inefficient because it loads 2 revlog
files. The index, node map, and chunk cache for 00changelog.i
are thrown away and replaced by those for 00changelog.i.a.

The existing mechanism is also brittle because it is a layering
violation to access the data structures being accessed. For example,
the code copies the "chunk cache" because for inline revlogs
this cache contains the raw revision chunks and allows the original
changelog/revlog instance to access revision data for these pending
revisions. This whole behavior of course relies on the revlog
constructor reading the entirety of an inline revlog into memory
and caching it. That's why it is brittle. (I discovered all this
as part of modifying behavior of the chunk cache.)

This patch streamlines the loading of a pending 00changelog.i.a
revlog by doing it directly in the changelog constructor if told
to do so. When this code path is active, we no longer load the
00changelog.i file at all.

The only negative outcome I see from this change is if loading
00changelog.i was somehow facilitating a role. But I can't imagine
what that would be because we throw away its data (the index data
structures are replaced and inline revision data is replaced via
the chunk cache) and since 00changelog.i.a is a copy of
00changelog.i, file content should be identical, so there should
be no meaninful file integrity checking at play. I think this was
all just sub-optimal code.
Jun Wu - May 14, 2017, 12:11 a.m.
Looks good to me. I like the clean up.

Excerpts from Gregory Szorc's message of 2017-05-13 16:29:50 -0700:
> [...]
>      @storecache('00changelog.i')

storecache needs to include '00changelog.i.a'. But that belongs to another
patch.
Gregory Szorc - May 14, 2017, 1:02 a.m.
On Sat, May 13, 2017 at 5:11 PM, Jun Wu <quark@fb.com> wrote:

> Looks good to me. I like the clean up.
>
> Excerpts from Gregory Szorc's message of 2017-05-13 16:29:50 -0700:
> > [...]
> >      @storecache('00changelog.i')
>
> storecache needs to include '00changelog.i.a'. But that belongs to another
> patch.
>

I thought about this when I wrote the patch. Semantically, yes @storecache
should include 00changelog.i.a. However, I don't think it actually needs it
because:

* 00changelog.i.a should be immutable for the lifetime of its existence
* when the transaction closes successfully, 00changelog.i is updated,
invalidating the cache
* when the transaction aborts, file cache is purged via
repo.invalidate(clearfilecache=True) and the transaction context is over,
so the next .changelog will load 00changelog.i

There's also the issue of a reader invalidating its .changelog by an
independent writer starting a transaction. That feels sub-optimal.

FWIW, I had a series a year or so ago where I added a context manager that
when active basically said "I am a read-only operation." It aggressively
cached some things, leading to significant perf wins for some operations.
If I had infinite time, I'd hack on that a bit more and refactor the
localrepo class so you needed to obtain a specific class/type representing
a mutable repository, probably via a context manager based API that also
handled locks and transactions. If done right, most consumers could assume
a read-only repo and aggressively cache by default (rather than today,
where we check for freshness on e.g. every .changelog attribute access.
Related to this idea, I want to add a context manager API to the localrepo
class to indicate intended access patterns. For example, the optimization
strategy for reading revlogs varies significantly depending on if you are
reading forward or reverse and whether you are accessing all revisions or a
subset. If we had a hint of the anticipated access pattern, we could
optimize I/O and caching patterns appropriately. Unlike a
"read-only/mutable repo context manager," a perf-centric "anticipated
access patterns context manager" is something I may do in 4.3.
Jun Wu - May 14, 2017, 3:36 a.m.
Excerpts from Gregory Szorc's message of 2017-05-13 18:02:05 -0700:
> On Sat, May 13, 2017 at 5:11 PM, Jun Wu <quark@fb.com> wrote:
> 
> > Looks good to me. I like the clean up.
> >
> > Excerpts from Gregory Szorc's message of 2017-05-13 16:29:50 -0700:
> > > [...]
> > >      @storecache('00changelog.i')
> >
> > storecache needs to include '00changelog.i.a'. But that belongs to another
> > patch.
> >
> 
> I thought about this when I wrote the patch. Semantically, yes @storecache
> should include 00changelog.i.a. However, I don't think it actually needs it
> because:
> 
> * 00changelog.i.a should be immutable for the lifetime of its existence
> * when the transaction closes successfully, 00changelog.i is updated,
> invalidating the cache
> * when the transaction aborts, file cache is purged via
> repo.invalidate(clearfilecache=True) and the transaction context is over,
> so the next .changelog will load 00changelog.i
> 
> There's also the issue of a reader invalidating its .changelog by an
> independent writer starting a transaction. That feels sub-optimal.

Right. When I think about it more, it's not needed.

I'm more interested in stateful chg case [1]. At the same time, it's
impossible to have 2 processes running 2 transactions, so it'd be fine.

[1]: https://bitbucket.org/quark-zju/hg-draft/commits/branch/chg-stateful

> FWIW, I had a series a year or so ago where I added a context manager that
> when active basically said "I am a read-only operation." It aggressively
> cached some things, leading to significant perf wins for some operations.
> If I had infinite time, I'd hack on that a bit more and refactor the
> localrepo class so you needed to obtain a specific class/type representing
> a mutable repository, probably via a context manager based API that also
> handled locks and transactions. If done right, most consumers could assume
> a read-only repo and aggressively cache by default (rather than today,
> where we check for freshness on e.g. every .changelog attribute access.

For changelog, the filecache layer should be a no-op (no stat syscalls)
because we will use "unfi.__dict__['changelog']". repoview.changelog seems
to be problematic, although it looks cheap, but I can see it becomes a
problem if "repo.changelog" is used in a loop.

Instead of marking part of the operations "read-only", I wonder if we can do
the opposite - repo is immutable by default, write operations need to be
marked explicitly. We already have repo.transaction, which provides the
information already.

So I think repoview could be something like:

  class repoview(object):
      def __init__(self):
          ....
          self.changelog = self._clcache = self._getchangelog()

      def _getchangelog(self):
          # the same as repoview.changelog today

      def transaction(self):
          self.changelog = property(self._getchangelog)
          def restorechangelog(self):
              self.changelog = self._clcache
          unfi.transaction(releasefn=restorechangelog)

That seems to be able to achieve the perf win and is easier.

> Related to this idea, I want to add a context manager API to the localrepo
> class to indicate intended access patterns. For example, the optimization
> strategy for reading revlogs varies significantly depending on if you are
> reading forward or reverse and whether you are accessing all revisions or a
> subset. If we had a hint of the anticipated access pattern, we could
> optimize I/O and caching patterns appropriately. Unlike a

That's true. Given the fact that revlog has so many callers I wonder if
there is an automatic way to do that so caller does not need to change.
A naive way is to record (access_direction, count), where "direction" is -1
or 1 depending on the rev being accessed compared to the last rev. Count
increases when direction does not change but another rev is accessed.

In this way, we could set a threshold for "count" and change caching
strategy dynamically, if direction is backwards, cache the delta base
revision, otherwise, cache whatever revision accessed like today.

> "read-only/mutable repo context manager," a perf-centric "anticipated
> access patterns context manager" is something I may do in 4.3.
Gregory Szorc - May 14, 2017, 6:20 a.m.
On Sat, May 13, 2017 at 8:36 PM, Jun Wu <quark@fb.com> wrote:

> Excerpts from Gregory Szorc's message of 2017-05-13 18:02:05 -0700:
> > On Sat, May 13, 2017 at 5:11 PM, Jun Wu <quark@fb.com> wrote:
> >
> > > Looks good to me. I like the clean up.
> > >
> > > Excerpts from Gregory Szorc's message of 2017-05-13 16:29:50 -0700:
> > > > [...]
> > > >      @storecache('00changelog.i')
> > >
> > > storecache needs to include '00changelog.i.a'. But that belongs to
> another
> > > patch.
> > >
> >
> > I thought about this when I wrote the patch. Semantically, yes
> @storecache
> > should include 00changelog.i.a. However, I don't think it actually needs
> it
> > because:
> >
> > * 00changelog.i.a should be immutable for the lifetime of its existence
> > * when the transaction closes successfully, 00changelog.i is updated,
> > invalidating the cache
> > * when the transaction aborts, file cache is purged via
> > repo.invalidate(clearfilecache=True) and the transaction context is
> over,
> > so the next .changelog will load 00changelog.i
> >
> > There's also the issue of a reader invalidating its .changelog by an
> > independent writer starting a transaction. That feels sub-optimal.
>
> Right. When I think about it more, it's not needed.
>
> I'm more interested in stateful chg case [1]. At the same time, it's
> impossible to have 2 processes running 2 transactions, so it'd be fine.
>
> [1]: https://bitbucket.org/quark-zju/hg-draft/commits/branch/chg-stateful
>
> > FWIW, I had a series a year or so ago where I added a context manager
> that
> > when active basically said "I am a read-only operation." It aggressively
> > cached some things, leading to significant perf wins for some operations.
> > If I had infinite time, I'd hack on that a bit more and refactor the
> > localrepo class so you needed to obtain a specific class/type
> representing
> > a mutable repository, probably via a context manager based API that also
> > handled locks and transactions. If done right, most consumers could
> assume
> > a read-only repo and aggressively cache by default (rather than today,
> > where we check for freshness on e.g. every .changelog attribute access.
>
> For changelog, the filecache layer should be a no-op (no stat syscalls)
> because we will use "unfi.__dict__['changelog']". repoview.changelog seems
> to be problematic, although it looks cheap, but I can see it becomes a
> problem if "repo.changelog" is used in a loop.
>

IIRC marmoute optimized it significantly. But it still isn't as cheap as it
could be. I think it should be an attribute lookup by default because it
isn't like the changelog is changing all the time.


>
> Instead of marking part of the operations "read-only", I wonder if we can
> do
> the opposite - repo is immutable by default, write operations need to be
> marked explicitly. We already have repo.transaction, which provides the
> information already.
>

I agree that the repo should be read-only by default and that some kind of
function call is necessary to transform it into mutable/slow mode.


>
> So I think repoview could be something like:
>
>   class repoview(object):
>       def __init__(self):
>           ....
>           self.changelog = self._clcache = self._getchangelog()
>
>       def _getchangelog(self):
>           # the same as repoview.changelog today
>
>       def transaction(self):
>           self.changelog = property(self._getchangelog)
>           def restorechangelog(self):
>               self.changelog = self._clcache
>           unfi.transaction(releasefn=restorechangelog)
>
> That seems to be able to achieve the perf win and is easier.
>
> > Related to this idea, I want to add a context manager API to the
> localrepo
> > class to indicate intended access patterns. For example, the optimization
> > strategy for reading revlogs varies significantly depending on if you are
> > reading forward or reverse and whether you are accessing all revisions
> or a
> > subset. If we had a hint of the anticipated access pattern, we could
> > optimize I/O and caching patterns appropriately. Unlike a
>
> That's true. Given the fact that revlog has so many callers I wonder if
> there is an automatic way to do that so caller does not need to change.
> A naive way is to record (access_direction, count), where "direction" is -1
> or 1 depending on the rev being accessed compared to the last rev. Count
> increases when direction does not change but another rev is accessed.
>
> In this way, we could set a threshold for "count" and change caching
> strategy dynamically, if direction is backwards, cache the delta base
> revision, otherwise, cache whatever revision accessed like today.
>

Yes, that's certainly possible (and is something I've considered
implementing). However, we do need hints to go to the next level. For
example, if you know you will be reading most/all revisions, you can do
things like fetch all the data and decompress all chunks parallel. That's
more efficient than going back and forth between I/O and processing.
python-zstandard has APIs for parallel decompression BTW. If the right
revlog APIs/hints are in place, revlog read performance can speed up
drastically. Of course, reading *everything* doesn't scale to infinity. But
you can set batch size to something ridiculously large like 200 MB to get
the benefits on 99% of revlogs.


>
> > "read-only/mutable repo context manager," a perf-centric "anticipated
> > access patterns context manager" is something I may do in 4.3.
>
Jun Wu - May 14, 2017, 4:22 p.m.
Excerpts from Gregory Szorc's message of 2017-05-13 23:20:21 -0700:
> [...]
> > Instead of marking part of the operations "read-only", I wonder if we can
> > do
> > the opposite - repo is immutable by default, write operations need to be
> > marked explicitly. We already have repo.transaction, which provides the
> > information already.
> >
> 
> I agree that the repo should be read-only by default and that some kind of
> function call is necessary to transform it into mutable/slow mode.

I tried replacing "def lock" in repoview to switch between fast and slow
path, sadly there are a few test failures, namely test-clone. I guess we
still have some code path that's writing without proper locking that needs
to be cleaned up first.

> [...]
> Yes, that's certainly possible (and is something I've considered
> implementing). However, we do need hints to go to the next level. For
> example, if you know you will be reading most/all revisions, you can do
> things like fetch all the data and decompress all chunks parallel. That's
> more efficient than going back and forth between I/O and processing.
> python-zstandard has APIs for parallel decompression BTW. If the right
> revlog APIs/hints are in place, revlog read performance can speed up
> drastically. Of course, reading *everything* doesn't scale to infinity. But
> you can set batch size to something ridiculously large like 200 MB to get
> the benefits on 99% of revlogs.

Agree. Some related notes:

  1. Currently accessing changelog index needs to take the GIL. Maybe
     re-invent changelog index features incrementally without CPython API
     (in clean C or rust) is the sane way to move forward.
  2. If revlog could spawn a thread pool implicitly, then chg needs a way to
     stop those threads. Otherwise at fork time threads will get lost.

> [...]
Augie Fackler - May 15, 2017, 6:27 p.m.
On Sat, May 13, 2017 at 04:29:50PM -0700, Gregory Szorc wrote:
> # HG changeset patch
> # User Gregory Szorc <gregory.szorc@gmail.com>
> # Date 1494718003 25200
> #      Sat May 13 16:26:43 2017 -0700
> # Node ID 53a93d88e75d42e3e71c821154ccc6b5ed4fd349
> # Parent  78496ac300255e9996b3e282086661afc08af37c
> changelog: load pending file directly

queued, thanks

Patch

diff --git a/mercurial/changelog.py b/mercurial/changelog.py
--- a/mercurial/changelog.py
+++ b/mercurial/changelog.py
@@ -258,9 +258,23 @@  class changelogrevision(object):
         return encoding.tolocal(self._text[self._offsets[3] + 2:])
 
 class changelog(revlog.revlog):
-    def __init__(self, opener):
-        revlog.revlog.__init__(self, opener, "00changelog.i",
-                               checkambig=True)
+    def __init__(self, opener, trypending=False):
+        """Load a changelog revlog using an opener.
+
+        If ``trypending`` is true, we attempt to load the index from a
+        ``00changelog.i.a`` file instead of the default ``00changelog.i``.
+        The ``00changelog.i.a`` file contains index (and possibly inline
+        revision) data for a transaction that hasn't been finalized yet.
+        It exists in a separate file to facilitate readers (such as
+        hooks processes) accessing data before a transaction is finalized.
+        """
+        if trypending and opener.exists('00changelog.i.a'):
+            indexfile = '00changelog.i.a'
+        else:
+            indexfile = '00changelog.i'
+
+        revlog.revlog.__init__(self, opener, indexfile, checkambig=True)
+
         if self._initempty:
             # changelogs don't benefit from generaldelta
             self.version &= ~revlog.REVLOGGENERALDELTA
@@ -401,27 +415,6 @@  class changelog(revlog.revlog):
         # split when we're done
         self.checkinlinesize(tr)
 
-    def readpending(self, file):
-        """read index data from a "pending" file
-
-        During a transaction, the actual changeset data is already stored in the
-        main file, but not yet finalized in the on-disk index. Instead, a
-        "pending" index is written by the transaction logic. If this function
-        is running, we are likely in a subprocess invoked in a hook. The
-        subprocess is informed that it is within a transaction and needs to
-        access its content.
-
-        This function will read all the index data out of the pending file and
-        overwrite the main index."""
-
-        if not self.opener.exists(file):
-            return # no pending data for changelog
-        r = revlog.revlog(self.opener, file)
-        self.index = r.index
-        self.nodemap = r.nodemap
-        self._nodecache = r._nodecache
-        self._chunkcache = r._chunkcache
-
     def _writepending(self, tr):
         "create a file containing the unfinalized state for pretxnchangegroup"
         if self._delaybuf:
diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
--- a/mercurial/localrepo.py
+++ b/mercurial/localrepo.py
@@ -528,10 +528,8 @@  class localrepository(object):
 
     @storecache('00changelog.i')
     def changelog(self):
-        c = changelog.changelog(self.svfs)
-        if txnutil.mayhavepending(self.root):
-            c.readpending('00changelog.i.a')
-        return c
+        return changelog.changelog(self.svfs,
+                                   trypending=txnutil.mayhavepending(self.root))
 
     def _constructmanifest(self):
         # This is a temporary function while we migrate from manifest to