Re: [PATCH 1 of 3 v2] profiling: allow nested usage of maybeprofile
On 9/22/16, 4:21 AM, "Yuya Nishihara" wrote:
On Thu, 22 Sep 2016 09:04:40 +, Arun Kulshreshtha wrote:
> On 9/21/16, 8:51 PM, "Yuya Nishihara" 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.
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Re: [PATCH 1 of 3 v2] profiling: allow nested usage of maybeprofile
On Thu, 22 Sep 2016 09:04:40 +, Arun Kulshreshtha wrote:
> On 9/21/16, 8:51 PM, "Yuya Nishihara" y...@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().
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Re: [PATCH 1 of 3 v2] profiling: allow nested usage of maybeprofile
On 9/21/16, 8:51 PM, "Yuya Nishihara" wrote:
On Wed, 21 Sep 2016 18:07:57 +, Arun Kulshreshtha wrote:
> On 9/21/16, 7:41 AM, "Yuya Nishihara" 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.
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Re: [PATCH 1 of 3 v2] profiling: allow nested usage of maybeprofile
On Wed, 21 Sep 2016 18:07:57 +, Arun Kulshreshtha wrote:
> On 9/21/16, 7:41 AM, "Yuya Nishihara" 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'):
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Re: [PATCH 1 of 3 v2] profiling: allow nested usage of maybeprofile
On 9/21/16, 7:41 AM, "Yuya Nishihara" 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
> > # 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.
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Re: [PATCH 1 of 3 v2] profiling: allow nested usage of maybeprofile
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
> > # 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.
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Re: [PATCH 1 of 3 v2] profiling: allow nested usage of maybeprofile
On Mon, 19 Sep 2016 16:13:56 -0700, Arun Kulshreshtha wrote:
> # HG changeset patch
> # User Arun Kulshreshtha
> # 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().
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Re: [PATCH 1 of 3 v2] profiling: allow nested usage of maybeprofile
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"
wrote:
# HG changeset patch
# User Arun Kulshreshtha
# 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=
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
