| 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
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.
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.
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.
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.
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):