Patchwork [7,of,8,v4] extdiff: log time spent in external diff program

login
register
mail settings
Submitter Simon Farnsworth
Date Feb. 13, 2017, 5:29 p.m.
Message ID <0727b4d77849fbefbf1c.1487006947@devvm022.lla2.facebook.com>
Download mbox | patch
Permalink /patch/18451/
State Changes Requested
Headers show

Comments

Simon Farnsworth - Feb. 13, 2017, 5:29 p.m.
# HG changeset patch
# User Simon Farnsworth <simonfar@fb.com>
# Date 1486994849 28800
#      Mon Feb 13 06:07:29 2017 -0800
# Node ID 0727b4d77849fbefbf1ce1de6d9fe22ad2c5e1bd
# Parent  67a55b66a69520f84552cf3c1a7d93202c3f43da
extdiff: log time spent in external diff program

We can't fix the time external diff programs take to run. Log that duration
for us to remove from any stats we gather
Bryan O'Sullivan - Feb. 13, 2017, 6:04 p.m.
On Mon, Feb 13, 2017 at 9:29 AM, Simon Farnsworth <simonfar@fb.com> wrote:

> +        with ui.timeblockedsection('extdiff'):
> +            ui.system(cmdline, cwd=tmproot)
>

Why not simply instrument ui.system directly, and leave its callers
untouched?
Simon Farnsworth - Feb. 13, 2017, 6:25 p.m.
On 13/02/2017 18:04, Bryan O'Sullivan wrote:
>
> On Mon, Feb 13, 2017 at 9:29 AM, Simon Farnsworth <simonfar@fb.com
> <mailto:simonfar@fb.com>> wrote:
>
>     +        with ui.timeblockedsection('extdiff'):
>     +            ui.system(cmdline, cwd=tmproot)
>
>
> Why not simply instrument ui.system directly, and leave its callers
> untouched?

I want the caller to tell me what tag to apply to this blocking reason, 
so that I can account for the blocking in hook.py (where it's running a 
non-interactive process) and sshpeer.py (where it's network blocking as 
we talk to a server) differently to extdiff.py and filemerge.py (where 
it's an interactive process).

I could push the instrumentation into ui.system, conditional on being 
passed a tag (with a catch-all for "not tagged") if people think that 
would be more useful?
Augie Fackler - Feb. 13, 2017, 7:41 p.m.
On Mon, Feb 13, 2017 at 06:25:11PM +0000, Simon Farnsworth wrote:
> On 13/02/2017 18:04, Bryan O'Sullivan wrote:
> >
> > On Mon, Feb 13, 2017 at 9:29 AM, Simon Farnsworth <simonfar@fb.com
> > <mailto:simonfar@fb.com>> wrote:
> >
> >     +        with ui.timeblockedsection('extdiff'):
> >     +            ui.system(cmdline, cwd=tmproot)
> >
> >
> > Why not simply instrument ui.system directly, and leave its callers
> > untouched?
>
> I want the caller to tell me what tag to apply to this blocking reason, so
> that I can account for the blocking in hook.py (where it's running a
> non-interactive process) and sshpeer.py (where it's network blocking as we
> talk to a server) differently to extdiff.py and filemerge.py (where it's an
> interactive process).
>
> I could push the instrumentation into ui.system, conditional on being passed
> a tag (with a catch-all for "not tagged") if people think that would be more
> useful?

Seems useful to me ("unspecified subprocess" or similar label)

>
> --
> Simon Farnsworth
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Bryan O'Sullivan - Feb. 13, 2017, 10:26 p.m.
On Mon, Feb 13, 2017 at 10:25 AM, Simon Farnsworth <simonfar@fb.com> wrote:

> I want the caller to tell me what tag to apply to this blocking reason, so
> that I can account for the blocking in hook.py (where it's running a
> non-interactive process) and sshpeer.py (where it's network blocking as we
> talk to a server) differently to extdiff.py and filemerge.py (where it's an
> interactive process).
>
> I could push the instrumentation into ui.system, conditional on being
> passed a tag (with a catch-all for "not tagged") if people think that would
> be more useful?
>

I toyed with suggesting use of the first component of the string to be
executed by ui.system as the tag, but that's too trolly :-)

But yeah, passing in a tag and defaulting to something else (perhaps that
trolly suggestion?) sounds good.

Patch

diff --git a/hgext/extdiff.py b/hgext/extdiff.py
--- a/hgext/extdiff.py
+++ b/hgext/extdiff.py
@@ -273,7 +273,8 @@ 
         cmdline = re.sub(regex, quote, cmdline)
 
         ui.debug('running %r in %s\n' % (cmdline, tmproot))
-        ui.system(cmdline, cwd=tmproot)
+        with ui.timeblockedsection('extdiff'):
+            ui.system(cmdline, cwd=tmproot)
 
         for copy_fn, working_fn, mtime in fns_and_mtime:
             if os.lstat(copy_fn).st_mtime != mtime: