Patchwork [3,of,3] perfchangelog: add 'perfchangelog' command

login
register
mail settings
Submitter Pierre-Yves David
Date May 23, 2017, 2:04 p.m.
Message ID <73e20c262b133cf3117a.1495548263@nodosa.octopoid.net>
Download mbox | patch
Permalink /patch/20862/
State Accepted
Headers show

Comments

Pierre-Yves David - May 23, 2017, 2:04 p.m.
# HG changeset patch
# User Pierre-Yves David <pierre-yves.david@octobus.net>
# Date 1495499078 -7200
#      Tue May 23 02:24:38 2017 +0200
# Node ID 73e20c262b133cf3117a7148e6293b5721e8930d
# Parent  c79b8f2a95a35ce1b41325d003337c65bc5bffca
# EXP-Topic perf
# Available At https://www.mercurial-scm.org/repo/users/marmoute/mercurial/
#              hg pull https://www.mercurial-scm.org/repo/users/marmoute/mercurial/ -r 73e20c262b13
perfchangelog: add 'perfchangelog' command

That command monitor the time spend initializing a new changelog object.
If I'm not mistaken this includes reading the file on disk and building the
nodemap.
Gregory Szorc - May 26, 2017, 5:07 p.m.
On Tue, May 23, 2017 at 7:04 AM, Pierre-Yves David <
pierre-yves.david@ens-lyon.org> wrote:

> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@octobus.net>
> # Date 1495499078 -7200
> #      Tue May 23 02:24:38 2017 +0200
> # Node ID 73e20c262b133cf3117a7148e6293b5721e8930d
> # Parent  c79b8f2a95a35ce1b41325d003337c65bc5bffca
> # EXP-Topic perf
> # Available At https://www.mercurial-scm.org/
> repo/users/marmoute/mercurial/
> #              hg pull https://www.mercurial-scm.org/
> repo/users/marmoute/mercurial/ -r 73e20c262b13
> perfchangelog: add 'perfchangelog' command
>
> That command monitor the time spend initializing a new changelog object.
> If I'm not mistaken this includes reading the file on disk and building the
> nodemap.
>

I also don't like the low-level cache muckery in this patch. Furthermore,
the commit message doesn't convince me that this is monitoring what you
expect it to monitor. As written, the commit message implies this is about
testing just revlog opening. If so, this should be part of one of the
perfrevlog* commands. That being said, repo.changelog goes beyond mere
revlog opening (it also computes visibility set, etc). We should definitely
have a perf command for testing that. Whether it needs a dedicated command,
I'm not sure. I think I'd rather see a perfrepoopen command that operates
like perfrevlogchunks or perfrevlogrevision and measures all the various
phases of constructing a new repo object and doing a simple changelog index
operation to trigger changelog load.

That being said, the barrier for perf* commands is low. So I'm fine with a
perfchangelog command that just tests repo.changelog access as a first step
towards something grander.


>
> diff --git a/contrib/perf.py b/contrib/perf.py
> --- a/contrib/perf.py
> +++ b/contrib/perf.py
> @@ -488,6 +488,22 @@ def perfchangegroupchangelog(ui, repo, v
>      timer(d)
>      fm.end()
>
> +def _clearchangelog(repo):
> +    if 'changelog' in vars(repo):
> +        del repo.changelog
> +        del repo._filecache['changelog']
> +
> +@command('perfchangelog', [], "")
> +def perfchangelog(ui, repo, **opts):
> +    """benchmark the changelog initialization"""
> +    timer, fm = gettimer(ui, opts)
> +    repo = repo.unfiltered()
> +    def d():
> +        _clearchangelog(repo)
> +        repo.changelog
> +    timer(d)
> +    fm.end()
> +
>  @command('perfdirs', formatteropts)
>  def perfdirs(ui, repo, **opts):
>      timer, fm = gettimer(ui, opts)
> diff --git a/tests/test-contrib-perf.t b/tests/test-contrib-perf.t
> --- a/tests/test-contrib-perf.t
> +++ b/tests/test-contrib-perf.t
> @@ -56,6 +56,8 @@ perfstatus
>     perfcca       (no help text available)
>     perfchangegroupchangelog
>                   Benchmark producing a changelog group for a changegroup.
> +   perfchangelog
> +                 benchmark the changelog initialization
>     perfchangeset
>                   (no help text available)
>     perfctxfiles  (no help text available)
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
Pierre-Yves David - May 26, 2017, 6:44 p.m.
On 05/26/2017 07:07 PM, Gregory Szorc wrote:
> On Tue, May 23, 2017 at 7:04 AM, Pierre-Yves David
> <pierre-yves.david@ens-lyon.org <mailto:pierre-yves.david@ens-lyon.org>>
> wrote:
>
>     # HG changeset patch
>     # User Pierre-Yves David <pierre-yves.david@octobus.net
>     <mailto:pierre-yves.david@octobus.net>>
>     # Date 1495499078 -7200
>     #      Tue May 23 02:24:38 2017 +0200
>     # Node ID 73e20c262b133cf3117a7148e6293b5721e8930d
>     # Parent  c79b8f2a95a35ce1b41325d003337c65bc5bffca
>     # EXP-Topic perf
>     # Available At
>     https://www.mercurial-scm.org/repo/users/marmoute/mercurial/
>     <https://www.mercurial-scm.org/repo/users/marmoute/mercurial/>
>     #              hg pull
>     https://www.mercurial-scm.org/repo/users/marmoute/mercurial/
>     <https://www.mercurial-scm.org/repo/users/marmoute/mercurial/> -r
>     73e20c262b13
>     perfchangelog: add 'perfchangelog' command
>
>     That command monitor the time spend initializing a new changelog object.
>     If I'm not mistaken this includes reading the file on disk and
>     building the
>     nodemap.
>
>
> I also don't like the low-level cache muckery in this patch.

(I'll address that in the other patch)

> Furthermore, the commit message doesn't convince me that this is
> monitoring what you expect it to monitor. As written, the commit message
> implies this is about testing just revlog opening. If so, this should be
> part of one of the perfrevlog* commands.

My goal is to check the amount of time it take to read the data from 
disk and parse nodemap index. This is currently the very bottom line of 
opening a repository and doing any node look up. I do not care about the 
repository filtering steps. (that is kinda covered by perfphases and 
perfvolatileset).

I ruled out perfrevlog because the docstring says "Benchmark reading a 
series of revisions from a revlog" and I do not care about the reading.
If you prefer I can update the command to allow that (eg: "--no-reading" 
flag)

> That being said, repo.changelog
> goes beyond mere revlog opening (it also computes visibility set, etc).
> We should definitely have a perf command for testing that. Whether it
> needs a dedicated command, I'm not sure. I think I'd rather see a
> perfrepoopen command that operates like perfrevlogchunks or
> perfrevlogrevision and measures all the various phases of constructing a
> new repo object and doing a simple changelog index operation to trigger
> changelog load.
>
> That being said, the barrier for perf* commands is low. So I'm fine with
> a perfchangelog command that just tests repo.changelog access as a first
> step towards something grander.

I'm fine with both (updating perfrevlog) or keeping the function. Just 
let me know.
Gregory Szorc - May 27, 2017, 3:04 a.m.
On Fri, May 26, 2017 at 11:44 AM, Pierre-Yves David <
pierre-yves.david@ens-lyon.org> wrote:

>
>
> On 05/26/2017 07:07 PM, Gregory Szorc wrote:
>
>> On Tue, May 23, 2017 at 7:04 AM, Pierre-Yves David
>> <pierre-yves.david@ens-lyon.org <mailto:pierre-yves.david@ens-lyon.org>>
>> wrote:
>>
>>     # HG changeset patch
>>     # User Pierre-Yves David <pierre-yves.david@octobus.net
>>     <mailto:pierre-yves.david@octobus.net>>
>>     # Date 1495499078 -7200
>>     #      Tue May 23 02:24:38 2017 +0200
>>     # Node ID 73e20c262b133cf3117a7148e6293b5721e8930d
>>     # Parent  c79b8f2a95a35ce1b41325d003337c65bc5bffca
>>     # EXP-Topic perf
>>     # Available At
>>     https://www.mercurial-scm.org/repo/users/marmoute/mercurial/
>>     <https://www.mercurial-scm.org/repo/users/marmoute/mercurial/>
>>     #              hg pull
>>     https://www.mercurial-scm.org/repo/users/marmoute/mercurial/
>>     <https://www.mercurial-scm.org/repo/users/marmoute/mercurial/> -r
>>     73e20c262b13
>>     perfchangelog: add 'perfchangelog' command
>>
>>     That command monitor the time spend initializing a new changelog
>> object.
>>     If I'm not mistaken this includes reading the file on disk and
>>     building the
>>     nodemap.
>>
>>
>> I also don't like the low-level cache muckery in this patch.
>>
>
> (I'll address that in the other patch)
>
> Furthermore, the commit message doesn't convince me that this is
>> monitoring what you expect it to monitor. As written, the commit message
>> implies this is about testing just revlog opening. If so, this should be
>> part of one of the perfrevlog* commands.
>>
>
> My goal is to check the amount of time it take to read the data from disk
> and parse nodemap index.


Turns out I started on something like this already. I may just send my
series...


> This is currently the very bottom line of opening a repository and doing
> any node look up. I do not care about the repository filtering steps. (that
> is kinda covered by perfphases and perfvolatileset).
>
> I ruled out perfrevlog because the docstring says "Benchmark reading a
> series of revisions from a revlog" and I do not care about the reading.
> If you prefer I can update the command to allow that (eg: "--no-reading"
> flag)
>
> That being said, repo.changelog
>> goes beyond mere revlog opening (it also computes visibility set, etc).
>> We should definitely have a perf command for testing that. Whether it
>> needs a dedicated command, I'm not sure. I think I'd rather see a
>> perfrepoopen command that operates like perfrevlogchunks or
>> perfrevlogrevision and measures all the various phases of constructing a
>> new repo object and doing a simple changelog index operation to trigger
>> changelog load.
>>
>> That being said, the barrier for perf* commands is low. So I'm fine with
>> a perfchangelog command that just tests repo.changelog access as a first
>> step towards something grander.
>>
>
> I'm fine with both (updating perfrevlog) or keeping the function. Just let
> me know.
>
> --
> Pierre-Yves David
>
Pierre-Yves David - May 27, 2017, 11:06 a.m.
On 05/27/2017 05:04 AM, Gregory Szorc wrote:
> On Fri, May 26, 2017 at 11:44 AM, Pierre-Yves David
> <pierre-yves.david@ens-lyon.org <mailto:pierre-yves.david@ens-lyon.org>>
> wrote:
>
>
>
>     On 05/26/2017 07:07 PM, Gregory Szorc wrote:
>
>         On Tue, May 23, 2017 at 7:04 AM, Pierre-Yves David
>         <pierre-yves.david@ens-lyon.org
>         <mailto:pierre-yves.david@ens-lyon.org>
>         <mailto:pierre-yves.david@ens-lyon.org
>         <mailto:pierre-yves.david@ens-lyon.org>>>
>         wrote:
>
>             # HG changeset patch
>             # User Pierre-Yves David <pierre-yves.david@octobus.net
>         <mailto:pierre-yves.david@octobus.net>
>             <mailto:pierre-yves.david@octobus.net
>         <mailto:pierre-yves.david@octobus.net>>>
>             # Date 1495499078 -7200
>             #      Tue May 23 02:24:38 2017 +0200
>             # Node ID 73e20c262b133cf3117a7148e6293b5721e8930d
>             # Parent  c79b8f2a95a35ce1b41325d003337c65bc5bffca
>             # EXP-Topic perf
>             # Available At
>             https://www.mercurial-scm.org/repo/users/marmoute/mercurial/
>         <https://www.mercurial-scm.org/repo/users/marmoute/mercurial/>
>
>         <https://www.mercurial-scm.org/repo/users/marmoute/mercurial/
>         <https://www.mercurial-scm.org/repo/users/marmoute/mercurial/>>
>             #              hg pull
>             https://www.mercurial-scm.org/repo/users/marmoute/mercurial/
>         <https://www.mercurial-scm.org/repo/users/marmoute/mercurial/>
>
>         <https://www.mercurial-scm.org/repo/users/marmoute/mercurial/
>         <https://www.mercurial-scm.org/repo/users/marmoute/mercurial/>> -r
>             73e20c262b13
>             perfchangelog: add 'perfchangelog' command
>
>             That command monitor the time spend initializing a new
>         changelog object.
>             If I'm not mistaken this includes reading the file on disk and
>             building the
>             nodemap.
>
>
>         I also don't like the low-level cache muckery in this patch.
>
>
>     (I'll address that in the other patch)
>
>         Furthermore, the commit message doesn't convince me that this is
>         monitoring what you expect it to monitor. As written, the commit
>         message
>         implies this is about testing just revlog opening. If so, this
>         should be
>         part of one of the perfrevlog* commands.
>
>
>     My goal is to check the amount of time it take to read the data from
>     disk and parse nodemap index.
>
>
> Turns out I started on something like this already. I may just send my
> series...

You are talking about that series, right:

   https://hg.mozilla.org/users/gszorc_mozilla.com/hg/rev/afc99aaff8a3

That looks very promising, Any pointer on what need to be done before it 
can go out?

Cheers,
Pierre-Yves David - May 27, 2017, 11:35 a.m.
On 05/27/2017 01:06 PM, Pierre-Yves David wrote:
>
>
> On 05/27/2017 05:04 AM, Gregory Szorc wrote:
>> On Fri, May 26, 2017 at 11:44 AM, Pierre-Yves David
>> <pierre-yves.david@ens-lyon.org <mailto:pierre-yves.david@ens-lyon.org>>
>> wrote:
>>
>>
>>
>>     On 05/26/2017 07:07 PM, Gregory Szorc wrote:
>>
>>         On Tue, May 23, 2017 at 7:04 AM, Pierre-Yves David
>>         <pierre-yves.david@ens-lyon.org
>>         <mailto:pierre-yves.david@ens-lyon.org>
>>         <mailto:pierre-yves.david@ens-lyon.org
>>         <mailto:pierre-yves.david@ens-lyon.org>>>
>>         wrote:
>>
>>             # HG changeset patch
>>             # User Pierre-Yves David <pierre-yves.david@octobus.net
>>         <mailto:pierre-yves.david@octobus.net>
>>             <mailto:pierre-yves.david@octobus.net
>>         <mailto:pierre-yves.david@octobus.net>>>
>>             # Date 1495499078 -7200
>>             #      Tue May 23 02:24:38 2017 +0200
>>             # Node ID 73e20c262b133cf3117a7148e6293b5721e8930d
>>             # Parent  c79b8f2a95a35ce1b41325d003337c65bc5bffca
>>             # EXP-Topic perf
>>             # Available At
>>             https://www.mercurial-scm.org/repo/users/marmoute/mercurial/
>>         <https://www.mercurial-scm.org/repo/users/marmoute/mercurial/>
>>
>>         <https://www.mercurial-scm.org/repo/users/marmoute/mercurial/
>>         <https://www.mercurial-scm.org/repo/users/marmoute/mercurial/>>
>>             #              hg pull
>>             https://www.mercurial-scm.org/repo/users/marmoute/mercurial/
>>         <https://www.mercurial-scm.org/repo/users/marmoute/mercurial/>
>>
>>         <https://www.mercurial-scm.org/repo/users/marmoute/mercurial/
>>
>> <https://www.mercurial-scm.org/repo/users/marmoute/mercurial/>> -r
>>             73e20c262b13
>>             perfchangelog: add 'perfchangelog' command
>>
>>             That command monitor the time spend initializing a new
>>         changelog object.
>>             If I'm not mistaken this includes reading the file on disk
>> and
>>             building the
>>             nodemap.
>>
>>
>>         I also don't like the low-level cache muckery in this patch.
>>
>>
>>     (I'll address that in the other patch)
>>
>>         Furthermore, the commit message doesn't convince me that this is
>>         monitoring what you expect it to monitor. As written, the commit
>>         message
>>         implies this is about testing just revlog opening. If so, this
>>         should be
>>         part of one of the perfrevlog* commands.
>>
>>
>>     My goal is to check the amount of time it take to read the data from
>>     disk and parse nodemap index.
>>
>>
>> Turns out I started on something like this already. I may just send my
>> series...
>
> You are talking about that series, right:
>
>   https://hg.mozilla.org/users/gszorc_mozilla.com/hg/rev/afc99aaff8a3
>
> That looks very promising, Any pointer on what need to be done before it
> can go out?

Never mind, I just spotted it on the list.

Cheers,

Patch

diff --git a/contrib/perf.py b/contrib/perf.py
--- a/contrib/perf.py
+++ b/contrib/perf.py
@@ -488,6 +488,22 @@  def perfchangegroupchangelog(ui, repo, v
     timer(d)
     fm.end()
 
+def _clearchangelog(repo):
+    if 'changelog' in vars(repo):
+        del repo.changelog
+        del repo._filecache['changelog']
+
+@command('perfchangelog', [], "")
+def perfchangelog(ui, repo, **opts):
+    """benchmark the changelog initialization"""
+    timer, fm = gettimer(ui, opts)
+    repo = repo.unfiltered()
+    def d():
+        _clearchangelog(repo)
+        repo.changelog
+    timer(d)
+    fm.end()
+
 @command('perfdirs', formatteropts)
 def perfdirs(ui, repo, **opts):
     timer, fm = gettimer(ui, opts)
diff --git a/tests/test-contrib-perf.t b/tests/test-contrib-perf.t
--- a/tests/test-contrib-perf.t
+++ b/tests/test-contrib-perf.t
@@ -56,6 +56,8 @@  perfstatus
    perfcca       (no help text available)
    perfchangegroupchangelog
                  Benchmark producing a changelog group for a changegroup.
+   perfchangelog
+                 benchmark the changelog initialization
    perfchangeset
                  (no help text available)
    perfctxfiles  (no help text available)