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

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

Comments

Arun Kulshreshtha - Sept. 19, 2016, 11:13 p.m.
# HG changeset patch
# User Arun Kulshreshtha <kulshrax@fb.com>
# Date 1474324901 25200
#      Mon Sep 19 15:41:41 2016 -0700
# Node ID 679c90104cc1fc92099ede6bd359f6ab5b10640d
# Parent  285a8c3e53f2183438f0cdbc238e4ab851d0d110
profiling: allow nested usage of maybeprofile

Add a check to the maybeprofile context manager to ensure that profiling
is only enabled once in nested invocations of this context manager.

Updated in v2 of this patch to reset itself once the root invocation
has exited. While not currently used, this ensures that maybeprofile
can be used in multiple (non-nested) places in a single run.
Arun Kulshreshtha - Sept. 19, 2016, 11:19 p.m.
This wasn’t caught during review, but I’ve updated this so that the nested invocation check resets itself after the root context manager exits. This ensures that if maybeprofile is called in several (non-nested) places, it won’t be the case that only the first instance enables profiling. This situation does not exist in the code right now, but may in the future.

On 9/19/16, 4:13 PM, "Mercurial-devel on behalf of Arun Kulshreshtha" <mercurial-devel-bounces@mercurial-scm.org on behalf of kulshrax@fb.com> wrote:

    # HG changeset patch
    # User Arun Kulshreshtha <kulshrax@fb.com>
    # Date 1474324901 25200
    #      Mon Sep 19 15:41:41 2016 -0700
    # Node ID 679c90104cc1fc92099ede6bd359f6ab5b10640d
    # Parent  285a8c3e53f2183438f0cdbc238e4ab851d0d110
    profiling: allow nested usage of maybeprofile
    
    Add a check to the maybeprofile context manager to ensure that profiling
    is only enabled once in nested invocations of this context manager.
    
    Updated in v2 of this patch to reset itself once the root invocation
    has exited. While not currently used, this ensures that maybeprofile
    can be used in multiple (non-nested) places in a single run.
    
    diff --git a/mercurial/profiling.py b/mercurial/profiling.py
    --- a/mercurial/profiling.py
    +++ b/mercurial/profiling.py
    @@ -157,8 +157,15 @@
         just use a single code path for calling into code you may want to profile
         and this function determines whether to start profiling.
         """
    -    if ui.configbool('profiling', 'enabled'):
    +
    +    # Guard against nested invocations of this context manager.
    +    # Profiling should only be started in the outermost invocation.
    +    alreadyenabled = getattr(maybeprofile, 'enabled', False)
    +
    +    if ui.configbool('profiling', 'enabled') and not alreadyenabled:
    +        maybeprofile.enabled = True
             with profile(ui):
                 yield
    +        maybeprofile.enabled = False
         else:
             yield
    _______________________________________________
    Mercurial-devel mailing list
    Mercurial-devel@mercurial-scm.org
    https://urldefense.proofpoint.com/v2/url?u=https-3A__www.mercurial-2Dscm.org_mailman_listinfo_mercurial-2Ddevel&d=DQIGaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=NubUq6RFfuooh_aTR8enmw&m=3aENrW6F_q5S4SGV7mwHjUJsVONwl4BG3ung2RrP290&s=leJq90j_N6hfWm0fRl4FhQyV8kKH_SvH1kAJrOugI-w&e=
Yuya Nishihara - Sept. 20, 2016, 12:49 p.m.
On Mon, 19 Sep 2016 16:13:56 -0700, Arun Kulshreshtha wrote:
> # HG changeset patch
> # User Arun Kulshreshtha <kulshrax@fb.com>
> # Date 1474324901 25200
> #      Mon Sep 19 15:41:41 2016 -0700
> # Node ID 679c90104cc1fc92099ede6bd359f6ab5b10640d
> # Parent  285a8c3e53f2183438f0cdbc238e4ab851d0d110
> profiling: allow nested usage of maybeprofile
> 
> Add a check to the maybeprofile context manager to ensure that profiling
> is only enabled once in nested invocations of this context manager.
> 
> Updated in v2 of this patch to reset itself once the root invocation
> has exited. While not currently used, this ensures that maybeprofile
> can be used in multiple (non-nested) places in a single run.
> 
> diff --git a/mercurial/profiling.py b/mercurial/profiling.py
> --- a/mercurial/profiling.py
> +++ b/mercurial/profiling.py
> @@ -157,8 +157,15 @@
>      just use a single code path for calling into code you may want to profile
>      and this function determines whether to start profiling.
>      """
> -    if ui.configbool('profiling', 'enabled'):
> +
> +    # Guard against nested invocations of this context manager.
> +    # Profiling should only be started in the outermost invocation.
> +    alreadyenabled = getattr(maybeprofile, 'enabled', False)
> +
> +    if ui.configbool('profiling', 'enabled') and not alreadyenabled:
> +        maybeprofile.enabled = True
>          with profile(ui):
>              yield
> +        maybeprofile.enabled = False

maybeprofile() can be called in threads. If we need to prevent nesting, we'll
have to save the flag in TLS.

Also, I think that should be managed by profile() or (ls|flame|stat)profile().
Yuya Nishihara - Sept. 21, 2016, 2:41 p.m.
On Tue, 20 Sep 2016 21:49:26 +0900, Yuya Nishihara wrote:
> On Mon, 19 Sep 2016 16:13:56 -0700, Arun Kulshreshtha wrote:
> > # HG changeset patch
> > # User Arun Kulshreshtha <kulshrax@fb.com>
> > # Date 1474324901 25200
> > #      Mon Sep 19 15:41:41 2016 -0700
> > # Node ID 679c90104cc1fc92099ede6bd359f6ab5b10640d
> > # Parent  285a8c3e53f2183438f0cdbc238e4ab851d0d110
> > profiling: allow nested usage of maybeprofile
> > 
> > Add a check to the maybeprofile context manager to ensure that profiling
> > is only enabled once in nested invocations of this context manager.
> > 
> > Updated in v2 of this patch to reset itself once the root invocation
> > has exited. While not currently used, this ensures that maybeprofile
> > can be used in multiple (non-nested) places in a single run.
> > 
> > diff --git a/mercurial/profiling.py b/mercurial/profiling.py
> > --- a/mercurial/profiling.py
> > +++ b/mercurial/profiling.py
> > @@ -157,8 +157,15 @@
> >      just use a single code path for calling into code you may want to profile
> >      and this function determines whether to start profiling.
> >      """
> > -    if ui.configbool('profiling', 'enabled'):
> > +
> > +    # Guard against nested invocations of this context manager.
> > +    # Profiling should only be started in the outermost invocation.
> > +    alreadyenabled = getattr(maybeprofile, 'enabled', False)
> > +
> > +    if ui.configbool('profiling', 'enabled') and not alreadyenabled:
> > +        maybeprofile.enabled = True
> >          with profile(ui):
> >              yield
> > +        maybeprofile.enabled = False
> 
> 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.
Arun Kulshreshtha - Sept. 21, 2016, 6:07 p.m.
On 9/21/16, 7:41 AM, "Yuya Nishihara" <yuya@tcha.org> wrote:

    On Tue, 20 Sep 2016 21:49:26 +0900, Yuya Nishihara wrote:
    > On Mon, 19 Sep 2016 16:13:56 -0700, Arun Kulshreshtha wrote:

    > > # HG changeset patch

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

    > > # Date 1474324901 25200

    > > #      Mon Sep 19 15:41:41 2016 -0700

    > > # Node ID 679c90104cc1fc92099ede6bd359f6ab5b10640d

    > > # Parent  285a8c3e53f2183438f0cdbc238e4ab851d0d110

    > > profiling: allow nested usage of maybeprofile

    > > 

    > > Add a check to the maybeprofile context manager to ensure that profiling

    > > is only enabled once in nested invocations of this context manager.

    > > 

    > > Updated in v2 of this patch to reset itself once the root invocation

    > > has exited. While not currently used, this ensures that maybeprofile

    > > can be used in multiple (non-nested) places in a single run.

    > > 

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

    > > --- a/mercurial/profiling.py

    > > +++ b/mercurial/profiling.py

    > > @@ -157,8 +157,15 @@

    > >      just use a single code path for calling into code you may want to profile

    > >      and this function determines whether to start profiling.

    > >      """

    > > -    if ui.configbool('profiling', 'enabled'):

    > > +

    > > +    # Guard against nested invocations of this context manager.

    > > +    # Profiling should only be started in the outermost invocation.

    > > +    alreadyenabled = getattr(maybeprofile, 'enabled', False)

    > > +

    > > +    if ui.configbool('profiling', 'enabled') and not alreadyenabled:

    > > +        maybeprofile.enabled = True

    > >          with profile(ui):

    > >              yield

    > > +        maybeprofile.enabled = False

    > 

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

Patch

diff --git a/mercurial/profiling.py b/mercurial/profiling.py
--- a/mercurial/profiling.py
+++ b/mercurial/profiling.py
@@ -157,8 +157,15 @@ 
     just use a single code path for calling into code you may want to profile
     and this function determines whether to start profiling.
     """
-    if ui.configbool('profiling', 'enabled'):
+
+    # Guard against nested invocations of this context manager.
+    # Profiling should only be started in the outermost invocation.
+    alreadyenabled = getattr(maybeprofile, 'enabled', False)
+
+    if ui.configbool('profiling', 'enabled') and not alreadyenabled:
+        maybeprofile.enabled = True
         with profile(ui):
             yield
+        maybeprofile.enabled = False
     else:
         yield