Patchwork [2,of,2] log: add flush delay config to graphlog

login
register
mail settings
Submitter Durham Goode
Date Sept. 22, 2015, 1:18 a.m.
Message ID <7d7d6cde6e4991c96b3d.1442884721@dev2000.prn2.facebook.com>
Download mbox | patch
Permalink /patch/10562/
State Changes Requested
Headers show

Comments

Durham Goode - Sept. 22, 2015, 1:18 a.m.
# HG changeset patch
# User Durham Goode <durham@fb.com>
# Date 1442884584 25200
#      Mon Sep 21 18:16:24 2015 -0700
# Node ID 7d7d6cde6e4991c96b3db336a4eeeb85148452c9
# Parent  7f997a372e42f61638cb2609b3323c3fb7b45ed3
log: add flush delay config to graphlog

This was already added to the normal log code path. Let's add it to the graph
log path as well.
Pierre-Yves David - Sept. 22, 2015, 1:54 a.m.
On 09/21/2015 06:18 PM, Durham Goode wrote:
> # HG changeset patch
> # User Durham Goode <durham@fb.com>
> # Date 1442884584 25200
> #      Mon Sep 21 18:16:24 2015 -0700
> # Node ID 7d7d6cde6e4991c96b3db336a4eeeb85148452c9
> # Parent  7f997a372e42f61638cb2609b3323c3fb7b45ed3
> log: add flush delay config to graphlog
>
> This was already added to the normal log code path. Let's add it to the graph
> log path as well.

We just had a real life discussion with Durham about using a growing 
windows for flushing instead. He will follow up with details and V2.
Durham Goode - Sept. 22, 2015, 2:25 a.m.
On 9/21/15 6:54 PM, Pierre-Yves David wrote:
>
>
> On 09/21/2015 06:18 PM, Durham Goode wrote:
>> # HG changeset patch
>> # User Durham Goode <durham@fb.com>
>> # Date 1442884584 25200
>> #      Mon Sep 21 18:16:24 2015 -0700
>> # Node ID 7d7d6cde6e4991c96b3db336a4eeeb85148452c9
>> # Parent  7f997a372e42f61638cb2609b3323c3fb7b45ed3
>> log: add flush delay config to graphlog
>>
>> This was already added to the normal log code path. Let's add it to 
>> the graph
>> log path as well.
>
> We just had a real life discussion with Durham about using a growing 
> windows for flushing instead. He will follow up with details and V2.
>
I thought about this a bit more.  I think sticking with a time based 
approach might actually be the right choice here.  The problem we're 
trying to solve is the lack of responsiveness, and using a timer seems 
to ensure that things respond in a timely manner (regardless of whether 
it's for the first commit being output, or the 100th).

The one thing I will change though is I will make it always call flush 
for the first N results.  That way we always fill the users screen quickly.
Yuya Nishihara - Sept. 22, 2015, 7:42 a.m.
On Mon, 21 Sep 2015 19:25:10 -0700, Durham Goode wrote:
> On 9/21/15 6:54 PM, Pierre-Yves David wrote:
> > On 09/21/2015 06:18 PM, Durham Goode wrote:
> >> # HG changeset patch
> >> # User Durham Goode <durham@fb.com>
> >> # Date 1442884584 25200
> >> #      Mon Sep 21 18:16:24 2015 -0700
> >> # Node ID 7d7d6cde6e4991c96b3db336a4eeeb85148452c9
> >> # Parent  7f997a372e42f61638cb2609b3323c3fb7b45ed3
> >> log: add flush delay config to graphlog
> >>
> >> This was already added to the normal log code path. Let's add it to 
> >> the graph
> >> log path as well.
> >
> > We just had a real life discussion with Durham about using a growing 
> > windows for flushing instead. He will follow up with details and V2.
> >
> I thought about this a bit more.  I think sticking with a time based 
> approach might actually be the right choice here.  The problem we're 
> trying to solve is the lack of responsiveness, and using a timer seems 
> to ensure that things respond in a timely manner (regardless of whether 
> it's for the first commit being output, or the 100th).
> 
> The one thing I will change though is I will make it always call flush 
> for the first N results.  That way we always fill the users screen quickly.

Just an idea, but can we handle it in the ui layer?

It seems this is the issue of the pager, so it will make sense for the pager
to hook ui.write to flush stdout by timer.
Pierre-Yves David - Sept. 22, 2015, 8:40 p.m.
On 09/21/2015 07:25 PM, Durham Goode wrote:
>
>
> On 9/21/15 6:54 PM, Pierre-Yves David wrote:
>>
>>
>> On 09/21/2015 06:18 PM, Durham Goode wrote:
>>> # HG changeset patch
>>> # User Durham Goode <durham@fb.com>
>>> # Date 1442884584 25200
>>> #      Mon Sep 21 18:16:24 2015 -0700
>>> # Node ID 7d7d6cde6e4991c96b3db336a4eeeb85148452c9
>>> # Parent  7f997a372e42f61638cb2609b3323c3fb7b45ed3
>>> log: add flush delay config to graphlog
>>>
>>> This was already added to the normal log code path. Let's add it to
>>> the graph
>>> log path as well.
>>
>> We just had a real life discussion with Durham about using a growing
>> windows for flushing instead. He will follow up with details and V2.
>>
> I thought about this a bit more.  I think sticking with a time based
> approach might actually be the right choice here.  The problem we're
> trying to solve is the lack of responsiveness, and using a timer seems
> to ensure that things respond in a timely manner (regardless of whether
> it's for the first commit being output, or the 100th).

The timer (as it currently stand) includes a config option. I think the 
increasing windows options will provide good enough result without any 
change to our UX surface. So it seems like the way to go first.

There is extra appeal for the increasing windows as it is also used for 
reading changesets data during log.

If some problem still exist after that we could search for other options.

> The one thing I will change though is I will make it always call flush
> for the first N results.  That way we always fill the users screen quickly.
Matt Mackall - Sept. 22, 2015, 9:42 p.m.
On Mon, 2015-09-21 at 19:25 -0700, Durham Goode wrote:
> 
> On 9/21/15 6:54 PM, Pierre-Yves David wrote:
> >
> >
> > On 09/21/2015 06:18 PM, Durham Goode wrote:
> >> # HG changeset patch
> >> # User Durham Goode <durham@fb.com>
> >> # Date 1442884584 25200
> >> #      Mon Sep 21 18:16:24 2015 -0700
> >> # Node ID 7d7d6cde6e4991c96b3db336a4eeeb85148452c9
> >> # Parent  7f997a372e42f61638cb2609b3323c3fb7b45ed3
> >> log: add flush delay config to graphlog
> >>
> >> This was already added to the normal log code path. Let's add it to 
> >> the graph
> >> log path as well.
> >
> > We just had a real life discussion with Durham about using a growing 
> > windows for flushing instead. He will follow up with details and V2.
> >
> I thought about this a bit more.  I think sticking with a time based 
> approach might actually be the right choice here.  The problem we're 
> trying to solve is the lack of responsiveness, and using a timer seems 
> to ensure that things respond in a timely manner (regardless of whether 
> it's for the first commit being output, or the 100th).

Seems weird to have this option in the log code when there are lots of
places it could crop up.

> The one thing I will change though is I will make it always call flush 
> for the first N results.  That way we always fill the users screen quickly.

I do like this idea, though it's not clear how big N should be. 50
lines? 100?

Also, today it's possible to do:

PYTHONUNBUFFERED=1 hg ...

..and get unbuffered I/O.

Patch

diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
--- a/mercurial/cmdutil.py
+++ b/mercurial/cmdutil.py
@@ -7,7 +7,7 @@ 
 
 from node import hex, bin, nullid, nullrev, short
 from i18n import _
-import os, sys, errno, re, tempfile, cStringIO, shutil
+import os, sys, errno, re, tempfile, cStringIO, shutil, time
 import util, scmutil, templater, patch, error, templatekw, revlog, copies
 import match as matchmod
 import repair, graphmod, revset, phases, obsolete, pathutil
@@ -2150,6 +2150,9 @@  def getlogrevs(repo, pats, opts):
 def displaygraph(ui, dag, displayer, showparents, edgefn, getrenamed=None,
                  filematcher=None):
     seen, state = [], graphmod.asciistate()
+
+    lastflush = time.time()
+    flushlimit = float(ui.config('ui', 'maxlogflushdelay', 1))
     for rev, type, ctx, parents in dag:
         char = 'o'
         if ctx.node() in showparents:
@@ -2176,6 +2179,11 @@  def displaygraph(ui, dag, displayer, sho
         edges = edgefn(type, char, lines, seen, rev, parents)
         for type, char, lines, coldata in edges:
             graphmod.ascii(ui, state, type, char, lines, coldata)
+
+        now = time.time()
+        if now - lastflush > flushlimit:
+            ui.flush()
+            lastflush = now
     displayer.close()
 
 def graphlog(ui, repo, *pats, **opts):