Patchwork [1,of,3,v2] profiling: allow nested usage of maybeprofile

login
register
mail settings
Submitter Yuya Nishihara
Date Sept. 22, 2016, 3:51 a.m.
Message ID <20160922125137.aab2a5046a2952351526363b@tcha.org>
Download mbox | patch
Permalink /patch/16742/
State Not Applicable
Headers show

Comments

Yuya Nishihara - Sept. 22, 2016, 3:51 a.m.
On Wed, 21 Sep 2016 18:07:57 +0000, Arun Kulshreshtha wrote:
> On 9/21/16, 7:41 AM, "Yuya Nishihara" <yuya@tcha.org> wrote:
>     > maybeprofile() can be called in threads. If we need to prevent nesting, we'll
>     > have to save the flag in TLS.
>     
>     That said, I'm not sure if nested maybeprofile() can be noop. Last time, Greg
>     tried to make statprof stackable, which would imply maybeprofile() designed
>     to be nested.
>     
> Hm, right now it seems like if you stack profilers, you’ll get multiple profiling statistics printouts at the end of command execution, which is hard to read. But this this behavior is actually desirable, then we’ll need to deal with this situation differently.

Perhaps we can move both maybeprofile() calls to _dispatch() without thinking
about how nested profilers behave.
Arun Kulshreshtha - Sept. 22, 2016, 9:04 a.m.
On 9/21/16, 8:51 PM, "Yuya Nishihara" <youjah@gmail.com on behalf of yuya@tcha.org> wrote:

    On Wed, 21 Sep 2016 18:07:57 +0000, Arun Kulshreshtha wrote:
    > On 9/21/16, 7:41 AM, "Yuya Nishihara" <yuya@tcha.org> wrote:

    >     > maybeprofile() can be called in threads. If we need to prevent nesting, we'll

    >     > have to save the flag in TLS.

    >     

    >     That said, I'm not sure if nested maybeprofile() can be noop. Last time, Greg

    >     tried to make statprof stackable, which would imply maybeprofile() designed

    >     to be nested.

    >     

    > Hm, right now it seems like if you stack profilers, you’ll get multiple profiling statistics printouts at the end of command execution, which is hard to read. But this this behavior is actually desirable, then we’ll need to deal with this situation differently.

    
    Perhaps we can move both maybeprofile() calls to _dispatch() without thinking
    about how nested profilers behave.
    
    --- a/mercurial/dispatch.py
    +++ b/mercurial/dispatch.py
    @@ -774,7 +774,8 @@ def _dispatch(req):
         # Check abbreviation/ambiguity of shell alias.
         shellaliasfn = _checkshellalias(lui, ui, args)
         if shellaliasfn:
    -        return shellaliasfn()
    +        with profiling.maybeprofile(lui):
    +            return shellaliasfn()
     
         # check for fallback encoding
         fallback = lui.config('ui', 'fallbackencoding')
    @@ -844,6 +845,10 @@ def _dispatch(req):
         elif not cmd:
             return commands.help_(ui, 'shortlist')
     
    +    with profiling.maybeprofile(lui):
    +        return _dispatchcommand(...)
    +
    +def _dispatchcommand(...):
         repo = None
         cmdpats = args[:]
         if not _cmdattr(ui, cmd, func, 'norepo'):
    

Yes, in fact this is originally what I had intended to do. However, this would mean that it
would not be possible to enable profiling from the repo-specific config file. Of course, we could
simply add a third call to maybeprofile after the call to hg.repository(), but prevent having duplicate
output with a boolean flag to check if profiling was already enabled before reposetup. This approach
seems a little inelegant, but it’s also simple enough.
Yuya Nishihara - Sept. 22, 2016, 11:21 a.m.
On Thu, 22 Sep 2016 09:04:40 +0000, Arun Kulshreshtha wrote:
> On 9/21/16, 8:51 PM, "Yuya Nishihara" <youjah@gmail.com on behalf of yuya@tcha.org> wrote:
>     Perhaps we can move both maybeprofile() calls to _dispatch() without thinking
>     about how nested profilers behave.
>     
>     --- a/mercurial/dispatch.py
>     +++ b/mercurial/dispatch.py
>     @@ -774,7 +774,8 @@ def _dispatch(req):
>          # Check abbreviation/ambiguity of shell alias.
>          shellaliasfn = _checkshellalias(lui, ui, args)
>          if shellaliasfn:
>     -        return shellaliasfn()
>     +        with profiling.maybeprofile(lui):
>     +            return shellaliasfn()
>      
>          # check for fallback encoding
>          fallback = lui.config('ui', 'fallbackencoding')
>     @@ -844,6 +845,10 @@ def _dispatch(req):
>          elif not cmd:
>              return commands.help_(ui, 'shortlist')
>      
>     +    with profiling.maybeprofile(lui):
>     +        return _dispatchcommand(...)
>     +
>     +def _dispatchcommand(...):
>          repo = None
>          cmdpats = args[:]
>          if not _cmdattr(ui, cmd, func, 'norepo'):
>     
> 
> Yes, in fact this is originally what I had intended to do. However, this would mean that it
> would not be possible to enable profiling from the repo-specific config file.

That's why 'lui' is passed to maybeprofile().
Arun Kulshreshtha - Sept. 22, 2016, 6:35 p.m.
On 9/22/16, 4:21 AM, "Yuya Nishihara" <youjah@gmail.com on behalf of yuya@tcha.org> wrote:

    On Thu, 22 Sep 2016 09:04:40 +0000, Arun Kulshreshtha wrote:
    > On 9/21/16, 8:51 PM, "Yuya Nishihara" <youjah@gmail.com on behalf of yuya@tcha.org> wrote:

    >     Perhaps we can move both maybeprofile() calls to _dispatch() without thinking

    >     about how nested profilers behave.

    >     

    >     --- a/mercurial/dispatch.py

    >     +++ b/mercurial/dispatch.py

    >     @@ -774,7 +774,8 @@ def _dispatch(req):

    >          # Check abbreviation/ambiguity of shell alias.

    >          shellaliasfn = _checkshellalias(lui, ui, args)

    >          if shellaliasfn:

    >     -        return shellaliasfn()

    >     +        with profiling.maybeprofile(lui):

    >     +            return shellaliasfn()

    >      

    >          # check for fallback encoding

    >          fallback = lui.config('ui', 'fallbackencoding')

    >     @@ -844,6 +845,10 @@ def _dispatch(req):

    >          elif not cmd:

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

    >      

    >     +    with profiling.maybeprofile(lui):

    >     +        return _dispatchcommand(...)

    >     +

    >     +def _dispatchcommand(...):

    >          repo = None

    >          cmdpats = args[:]

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

    >     

    > 

    > Yes, in fact this is originally what I had intended to do. However, this would mean that it

    > would not be possible to enable profiling from the repo-specific config file.

    
    That's why 'lui' is passed to maybeprofile().
    
Ah, I see. OK, I’ll change it then using lui and see if everything works.

Patch

--- a/mercurial/dispatch.py
+++ b/mercurial/dispatch.py
@@ -774,7 +774,8 @@  def _dispatch(req):
     # Check abbreviation/ambiguity of shell alias.
     shellaliasfn = _checkshellalias(lui, ui, args)
     if shellaliasfn:
-        return shellaliasfn()
+        with profiling.maybeprofile(lui):
+            return shellaliasfn()
 
     # check for fallback encoding
     fallback = lui.config('ui', 'fallbackencoding')
@@ -844,6 +845,10 @@  def _dispatch(req):
     elif not cmd:
         return commands.help_(ui, 'shortlist')
 
+    with profiling.maybeprofile(lui):
+        return _dispatchcommand(...)
+
+def _dispatchcommand(...):
     repo = None
     cmdpats = args[:]
     if not _cmdattr(ui, cmd, func, 'norepo'):