<div dir="ltr">On Sat, Jul 11, 2015 at 3:26 PM, Gregory Szorc <span dir="ltr"><<a href="mailto:gregory.szorc@gmail.com" target="_blank">gregory.szorc@gmail.com</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div><div>On Sat, Jul 11, 2015 at 1:17 PM, Matt Mackall <span dir="ltr"><<a href="mailto:mpm@selenic.com" target="_blank">mpm@selenic.com</a>></span> wrote:<br></div></div><div class="gmail_extra"><div class="gmail_quote"><div><div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span>On Sat, 2015-07-11 at 11:26 -0700, Gregory Szorc wrote:<br>
> On Fri, Jul 10, 2015 at 5:03 PM, Matt Mackall <<a href="mailto:mpm@selenic.com" target="_blank">mpm@selenic.com</a>> wrote:<br>
><br>
> > On Fri, 2015-07-10 at 16:46 -0700, Gregory Szorc wrote:<br>
> > > # HG changeset patch<br>
> > > # User Gregory Szorc <<a href="mailto:gregory.szorc@gmail.com" target="_blank">gregory.szorc@gmail.com</a>><br>
> > > # Date 1436571961 25200<br>
> > > #      Fri Jul 10 16:46:01 2015 -0700<br>
> > > # Node ID bca84aa6ea80b386a01c245e7f4e4ebdb720cd8c<br>
> > > # Parent  648323f41a89619d9eeeb7287213378c340866c8<br>
> > > revlog/changegroup: use callbacks for populating efiles<br>
> ><br>
> > Probably needs to use the flush() thingy that the nearby censor code is<br>
> > using.<br>
> ><br>
><br>
> Calling flush() makes the error go away!<br>
><br>
> While my SSD equipped MBP didn't appear to exhibit a slowdown unbundling<br>
> mozilla-central, flushing file descriptors after applying each changeset<br>
> feels a bit excessive and it makes me uncomfortable due to potential<br>
> performance implications. I just don't think an extra I/O related syscall<br>
> should be required for this feature.<br>
<br>
</span>The flush is only a flush from the userspace file buffers to kernel-side<br>
buffers, it won't actually force I/O. It'll still be lazily written by<br>
the kernel's caching layer at roughly the same rate as without the<br>
buffering because the userspace buffers are tiny (4k) relative a modern<br>
system's disk cache. But yes, it is a syscall and syscalls aren't free.<br>
<span><br>
> But you have a stronger grasp on these things than me. Is calling flush()<br>
> after every changeset add acceptable? Keep in mind this will be the only<br>
> behavior for addchangegroup(), regardless of whether progress bars are<br>
> displayed.<br>
<br>
</span>Well, it can be conditional on there being a callback.<br></blockquote><div><br></div></div></div><div>Right. But for changegroup.addchangegroup(), there will always be a callback so this will be the default behavior for many cases.<br></div><span><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
In the slightly bigger picture, we might want to make the revlog object<br>
aware of the file handles it might need to flush so that rather than<br>
explicitly flushing on the write side, the read side can flush only if<br>
needed. Something like:<br>
<br>
for f in self._flushonread:<br>
    f.flush()<br>
<br>
..with the appropriate try/finally around the writers.<br>
<br>
In the even bigger still picture, we might want to make it optionally<br>
possible for revlog to hold onto file handles for the object's lifetime<br>
to reduce syscall count and filesystem lookup overhead. Historically we<br>
don't do this because Windows.<br></blockquote><div><br></div></span><div>Looking at things a bit more, my original patch series also added additional flushes because buildtext() inside revlog._addrevision() does flushing and my "return text" patch made the call to buildtext() unconditional.<br><br></div><div>Tracing the system calls when applying a bunch of changesets, I'm seeing what I think is "odd" behavior. .hg/store/00changelog.i.a is opened, fstat()d twice, lseek()d twice, read() a few times, maybe write()n, and closed. This cycle repeats over and over. I'm still trying to grok what is happening. But it certainly doesn't feel very efficient. I suspect it has something to do with the flushing, which happens every couple of changesets inside buildtext(). I'm pretty sure this corresponds to starting/ending a delta chain.<br><br></div><div>This is the first time I've dug into the bowels of revlogs. It's a bit overwhelming. On the bright side, I think I'm starting to identify some potential performance improvements!<br></div></div></div></div></blockquote><br></div><div class="gmail_quote">I tracked down the source of all the extra opens. The stack looks like this:<br></div><div class="gmail_quote"><br>  /Users/gps/src/hg/mercurial/changegroup.py(757)addchangegroup()<br>-> srccontent = cl.addgroup(source, csmap, trp)<br>  /Users/gps/src/hg/mercurial/revlog.py(1461)addgroup()<br>-> ifh, dfh)<br>  /Users/gps/src/hg/mercurial/revlog.py(1346)_addrevision()<br>-> text = buildtext()<br>  /Users/gps/src/hg/mercurial/revlog.py(1265)buildtext()<br>-> basetext = self.revision(self.node(baserev))<br>  /Users/gps/src/hg/mercurial/revlog.py(1094)revision()<br>-> bins = self._chunks(chain)<br>  /Users/gps/src/hg/mercurial/revlog.py(1001)_chunks()<br>-> self._chunkraw(revs[0], revs[-1])<br>  /Users/gps/src/hg/mercurial/revlog.py(976)_chunkraw()<br>-> return self._getchunk(start, length)<br>  /Users/gps/src/hg/mercurial/revlog.py(967)_getchunk()<br>-> return self._loadchunk(offset, length)<br>  /Users/gps/src/hg/mercurial/revlog.py(936)_loadchunk()<br>-> df = self.opener(self.indexfile) <br></div><br></div><div class="gmail_extra">Essentially, every time we start/end a new delta chain, we need to flush the revlog (+ index), open a new file handle, and read in revisions necessary to construct the full revision text. In the case of the changelog during addgroup() (where delta chains are typically 2-4 in length), this results in tons of extra file I/O. On mozilla-central, we perform ~100,000 flush + opens over ~250,000 changesets.<br><br></div><div class="gmail_extra">I'll continue to poke around with creative solutions that don't regress performance.<br></div></div>