Patchwork [3,of,3,v2] dispatch: make hg --profile wrap reposetup

login
register
mail settings
Submitter Arun Kulshreshtha
Date Sept. 19, 2016, 11:13 p.m.
Message ID <20af15cac045b249aece.1474326838@dev10559.prn2.facebook.com>
Download mbox | patch
Permalink /patch/16679/
State Superseded
Headers show

Comments

Arun Kulshreshtha - Sept. 19, 2016, 11:13 p.m.
# HG changeset patch
# User Arun Kulshreshtha <kulshrax@fb.com>
# Date 1474318006 25200
#      Mon Sep 19 13:46:46 2016 -0700
# Node ID 20af15cac045b249aece42cb71b671302b6c314c
# Parent  6f33cc84cdd6c9ab38d32784505b6fb53bf3eba9
dispatch: make hg --profile wrap reposetup

Add profiling to _dispatch so that reposetup is included in the profiler
output. All existing usage of the profiling context manager has been preserved,
so the existing behavior of profiling enabled after reposetup will not be
affected.
Yuya Nishihara - Sept. 20, 2016, 1:01 p.m.
On Mon, 19 Sep 2016 16:13:58 -0700, Arun Kulshreshtha wrote:
> # HG changeset patch
> # User Arun Kulshreshtha <kulshrax@fb.com>
> # Date 1474318006 25200
> #      Mon Sep 19 13:46:46 2016 -0700
> # Node ID 20af15cac045b249aece42cb71b671302b6c314c
> # Parent  6f33cc84cdd6c9ab38d32784505b6fb53bf3eba9
> dispatch: make hg --profile wrap reposetup
> 
> Add profiling to _dispatch so that reposetup is included in the profiler
> output. All existing usage of the profiling context manager has been preserved,
> so the existing behavior of profiling enabled after reposetup will not be
> affected.
> 
> diff --git a/mercurial/dispatch.py b/mercurial/dispatch.py
> --- a/mercurial/dispatch.py
> +++ b/mercurial/dispatch.py
> @@ -844,7 +844,7 @@
>      elif not cmd:
>          return commands.help_(ui, 'shortlist')
>  
> -    if True:
> +    with profiling.maybeprofile(ui):
>          repo = None
>          cmdpats = args[:]
>          if not _cmdattr(ui, cmd, func, 'norepo'):

Any reason to not remove maybeprofile() from _runcommand() ? Can it be enabled
after reposetup() ?
Arun Kulshreshtha - Sept. 20, 2016, 6:30 p.m.
On 9/20/16, 6:01 AM, "Yuya Nishihara" <youjah@gmail.com on behalf of yuya@tcha.org> wrote:

    On Mon, 19 Sep 2016 16:13:58 -0700, Arun Kulshreshtha wrote:
    > # HG changeset patch

    > # User Arun Kulshreshtha <kulshrax@fb.com>

    > # Date 1474318006 25200

    > #      Mon Sep 19 13:46:46 2016 -0700

    > # Node ID 20af15cac045b249aece42cb71b671302b6c314c

    > # Parent  6f33cc84cdd6c9ab38d32784505b6fb53bf3eba9

    > dispatch: make hg --profile wrap reposetup

    > 

    > Add profiling to _dispatch so that reposetup is included in the profiler

    > output. All existing usage of the profiling context manager has been preserved,

    > so the existing behavior of profiling enabled after reposetup will not be

    > affected.

    > 

    > diff --git a/mercurial/dispatch.py b/mercurial/dispatch.py

    > --- a/mercurial/dispatch.py

    > +++ b/mercurial/dispatch.py

    > @@ -844,7 +844,7 @@

    >      elif not cmd:

    >          return commands.help_(ui, 'shortlist')

    >  

    > -    if True:

    > +    with profiling.maybeprofile(ui):

    >          repo = None

    >          cmdpats = args[:]

    >          if not _cmdattr(ui, cmd, func, 'norepo'):

    
    Any reason to not remove maybeprofile() from _runcommand() ? Can it be enabled
    after reposetup() ?

Yes, if it is configured in the repo-specific settings (.hg/hgrc), for example, then it would be missed if
maybeprofile were removed from _runcommand(). Additionally, we’d need to wrap other callsites of
_runcommand(), such as _checkshellalias(), to maintain the existing behavior.

(Resending this because I didn’t CC the list; sorry for the duplicate message.)
Yuya Nishihara - Sept. 21, 2016, 2:34 p.m.
On Tue, 20 Sep 2016 18:30:49 +0000, Arun Kulshreshtha wrote:
> On 9/20/16, 6:01 AM, "Yuya Nishihara" <youjah@gmail.com on behalf of yuya@tcha.org> wrote:
> 
>     On Mon, 19 Sep 2016 16:13:58 -0700, Arun Kulshreshtha wrote:
>     > # HG changeset patch
>     > # User Arun Kulshreshtha <kulshrax@fb.com>
>     > # Date 1474318006 25200
>     > #      Mon Sep 19 13:46:46 2016 -0700
>     > # Node ID 20af15cac045b249aece42cb71b671302b6c314c
>     > # Parent  6f33cc84cdd6c9ab38d32784505b6fb53bf3eba9
>     > dispatch: make hg --profile wrap reposetup
>     > 
>     > Add profiling to _dispatch so that reposetup is included in the profiler
>     > output. All existing usage of the profiling context manager has been preserved,
>     > so the existing behavior of profiling enabled after reposetup will not be
>     > affected.
>     > 
>     > diff --git a/mercurial/dispatch.py b/mercurial/dispatch.py
>     > --- a/mercurial/dispatch.py
>     > +++ b/mercurial/dispatch.py
>     > @@ -844,7 +844,7 @@
>     >      elif not cmd:
>     >          return commands.help_(ui, 'shortlist')
>     >  
>     > -    if True:
>     > +    with profiling.maybeprofile(ui):
>     >          repo = None
>     >          cmdpats = args[:]
>     >          if not _cmdattr(ui, cmd, func, 'norepo'):
>     
>     Any reason to not remove maybeprofile() from _runcommand() ? Can it be enabled
>     after reposetup() ?
> 
> Yes, if it is configured in the repo-specific settings (.hg/hgrc), for example, then it would be missed if
> maybeprofile were removed from _runcommand().

.hg/hgrc should be loaded to 'lui'. Can you check if maybeprofile(lui) works?

> Additionally, we’d need to wrap other callsites of
> _runcommand(), such as _checkshellalias(), to maintain the existing behavior.

Good point. Given that we want to start profiling as early as possible, I think
it's better to test profiling.enabled before extensions.loadall(lui), and test
it again after parsing command options.
Arun Kulshreshtha - Sept. 21, 2016, 8:07 p.m.
On 9/21/16, 7:34 AM, "Yuya Nishihara" <yuya@tcha.org> wrote:

    On Tue, 20 Sep 2016 18:30:49 +0000, Arun Kulshreshtha wrote:
    > On 9/20/16, 6:01 AM, "Yuya Nishihara" <youjah@gmail.com on behalf of yuya@tcha.org> wrote:

    > 

    >     On Mon, 19 Sep 2016 16:13:58 -0700, Arun Kulshreshtha wrote:

    >     > # HG changeset patch

    >     > # User Arun Kulshreshtha <kulshrax@fb.com>

    >     > # Date 1474318006 25200

    >     > #      Mon Sep 19 13:46:46 2016 -0700

    >     > # Node ID 20af15cac045b249aece42cb71b671302b6c314c

    >     > # Parent  6f33cc84cdd6c9ab38d32784505b6fb53bf3eba9

    >     > dispatch: make hg --profile wrap reposetup

    >     > 

    >     > Add profiling to _dispatch so that reposetup is included in the profiler

    >     > output. All existing usage of the profiling context manager has been preserved,

    >     > so the existing behavior of profiling enabled after reposetup will not be

    >     > affected.

    >     > 

    >     > diff --git a/mercurial/dispatch.py b/mercurial/dispatch.py

    >     > --- a/mercurial/dispatch.py

    >     > +++ b/mercurial/dispatch.py

    >     > @@ -844,7 +844,7 @@

    >     >      elif not cmd:

    >     >          return commands.help_(ui, 'shortlist')

    >     >  

    >     > -    if True:

    >     > +    with profiling.maybeprofile(ui):

    >     >          repo = None

    >     >          cmdpats = args[:]

    >     >          if not _cmdattr(ui, cmd, func, 'norepo'):

    >     

    >     Any reason to not remove maybeprofile() from _runcommand() ? Can it be enabled

    >     after reposetup() ?

    > 

    > Yes, if it is configured in the repo-specific settings (.hg/hgrc), for example, then it would be missed if

    > maybeprofile were removed from _runcommand().

    
    .hg/hgrc should be loaded to 'lui'. Can you check if maybeprofile(lui) works?
    
Alright, I’ll see if I can get that to work.

    > Additionally, we’d need to wrap other callsites of

    > _runcommand(), such as _checkshellalias(), to maintain the existing behavior.

    
    Good point. Given that we want to start profiling as early as possible, I think
    it's better to test profiling.enabled before extensions.loadall(lui), and test
    it again after parsing command options.

CC’ing Durham on this. I considered doing this, but during our (offline) discussion about this, it seemed 
like it was undesirable to profile uisetup() for each extension, which is why I placed maybeprofile() after 
extensions.loadall(). However, if we do indeed want to start profiling as early as possible, then I’ll try enabling 
profiling as early as possible in _dispatch() and remove the other callsites.
Durham Goode - Sept. 21, 2016, 8:26 p.m.
On 9/21/16 1:07 PM, Arun Kulshreshtha wrote:
>
>
> On 9/21/16, 7:34 AM, "Yuya Nishihara" <yuya@tcha.org> wrote:
>
>      On Tue, 20 Sep 2016 18:30:49 +0000, Arun Kulshreshtha wrote:
>      > On 9/20/16, 6:01 AM, "Yuya Nishihara" <youjah@gmail.com on behalf of yuya@tcha.org> wrote:
>      >
>      >     On Mon, 19 Sep 2016 16:13:58 -0700, Arun Kulshreshtha wrote:
>      >     > # HG changeset patch
>      >     > # User Arun Kulshreshtha <kulshrax@fb.com>
>      >     > # Date 1474318006 25200
>      >     > #      Mon Sep 19 13:46:46 2016 -0700
>      >     > # Node ID 20af15cac045b249aece42cb71b671302b6c314c
>      >     > # Parent  6f33cc84cdd6c9ab38d32784505b6fb53bf3eba9
>      >     > dispatch: make hg --profile wrap reposetup
>      >     >
>      >     > Add profiling to _dispatch so that reposetup is included in the profiler
>      >     > output. All existing usage of the profiling context manager has been preserved,
>      >     > so the existing behavior of profiling enabled after reposetup will not be
>      >     > affected.
>      >     >
>      >     > diff --git a/mercurial/dispatch.py b/mercurial/dispatch.py
>      >     > --- a/mercurial/dispatch.py
>      >     > +++ b/mercurial/dispatch.py
>      >     > @@ -844,7 +844,7 @@
>      >     >      elif not cmd:
>      >     >          return commands.help_(ui, 'shortlist')
>      >     >
>      >     > -    if True:
>      >     > +    with profiling.maybeprofile(ui):
>      >     >          repo = None
>      >     >          cmdpats = args[:]
>      >     >          if not _cmdattr(ui, cmd, func, 'norepo'):
>      >
>      >     Any reason to not remove maybeprofile() from _runcommand() ? Can it be enabled
>      >     after reposetup() ?
>      >
>      > Yes, if it is configured in the repo-specific settings (.hg/hgrc), for example, then it would be missed if
>      > maybeprofile were removed from _runcommand().
>      
>      .hg/hgrc should be loaded to 'lui'. Can you check if maybeprofile(lui) works?
>      
> Alright, I’ll see if I can get that to work.
>
>      > Additionally, we’d need to wrap other callsites of
>      > _runcommand(), such as _checkshellalias(), to maintain the existing behavior.
>      
>      Good point. Given that we want to start profiling as early as possible, I think
>      it's better to test profiling.enabled before extensions.loadall(lui), and test
>      it again after parsing command options.
>
> CC’ing Durham on this. I considered doing this, but during our (offline) discussion about this, it seemed
> like it was undesirable to profile uisetup() for each extension, which is why I placed maybeprofile() after
> extensions.loadall(). However, if we do indeed want to start profiling as early as possible, then I’ll try enabling
> profiling as early as possible in _dispatch() and remove the other callsites.
The only reason I hesitated to wrap uisetup and extsetup is that 
extensions may modify the profile logic (like we have a statprofext 
extension that configures statprof), so wrapping early may make that 
impossible.
Yuya Nishihara - Sept. 22, 2016, 3:52 a.m.
On Wed, 21 Sep 2016 13:26:31 -0700, Durham Goode wrote:
d> On 9/21/16 1:07 PM, Arun Kulshreshtha wrote:
> > On 9/21/16, 7:34 AM, "Yuya Nishihara" <yuya@tcha.org> wrote:
> >      > Additionally, we’d need to wrap other callsites of
> >      > _runcommand(), such as _checkshellalias(), to maintain the existing behavior.
> >      
> >      Good point. Given that we want to start profiling as early as possible, I think
> >      it's better to test profiling.enabled before extensions.loadall(lui), and test
> >      it again after parsing command options.
> >
> > CC’ing Durham on this. I considered doing this, but during our (offline) discussion about this, it seemed
> > like it was undesirable to profile uisetup() for each extension, which is why I placed maybeprofile() after
> > extensions.loadall(). However, if we do indeed want to start profiling as early as possible, then I’ll try enabling
> > profiling as early as possible in _dispatch() and remove the other callsites.
> The only reason I hesitated to wrap uisetup and extsetup is that 
> extensions may modify the profile logic (like we have a statprofext 
> extension that configures statprof), so wrapping early may make that 
> impossible.

Yeah, that's a valid point. It makes some sense to start profiling after
ui/extseutp().

Maybe --debugger has the same chicken-and-egg issue.

Patch

diff --git a/mercurial/dispatch.py b/mercurial/dispatch.py
--- a/mercurial/dispatch.py
+++ b/mercurial/dispatch.py
@@ -844,7 +844,7 @@ 
     elif not cmd:
         return commands.help_(ui, 'shortlist')
 
-    if True:
+    with profiling.maybeprofile(ui):
         repo = None
         cmdpats = args[:]
         if not _cmdattr(ui, cmd, func, 'norepo'):