Patchwork perf: add another command to test performance of writing to output streams

login
register
mail settings
Submitter Manuel Jacob
Date June 22, 2020, 12:02 a.m.
Message ID <1d9781c4f9bcd2df5745.1592784154@tmp>
Download mbox | patch
Permalink /patch/46541/
State New
Headers show

Comments

Manuel Jacob - June 22, 2020, 12:02 a.m.
# HG changeset patch
# User Manuel Jacob <me@manueljacob.de>
# Date 1591314853 -7200
#      Fri Jun 05 01:54:13 2020 +0200
# Node ID 1d9781c4f9bcd2df5745b7e9d880dbd5b46c559e
# Parent  61719b9658b156548634c216ffb2b9188266293b
# EXP-Topic perfwrite2
perf: add another command to test performance of writing to output streams

The command was used recently while finding a solution for a buffering bug
(eventually fixed in f9734b2d59cc (the changeset description uses a different
benchmark)).

Calling the new command "perfwrite2" to keep the existing command "perfwrite"
unchanged is not very clever, but I didn’t have a better idea.

In comparison to the other command, the new command is much more flexible. While
using it, the focus was on testing small writes. For this reason, by default it
calls ui.write() 100 times with a single byte plus one newline byte, for 100
lines.
Yuya Nishihara - June 22, 2020, 10:58 a.m.
On Mon, 22 Jun 2020 02:02:34 +0200, Manuel Jacob wrote:
> # HG changeset patch
> # User Manuel Jacob <me@manueljacob.de>
> # Date 1591314853 -7200
> #      Fri Jun 05 01:54:13 2020 +0200
> # Node ID 1d9781c4f9bcd2df5745b7e9d880dbd5b46c559e
> # Parent  61719b9658b156548634c216ffb2b9188266293b
> # EXP-Topic perfwrite2
> perf: add another command to test performance of writing to output streams
> 
> The command was used recently while finding a solution for a buffering bug
> (eventually fixed in f9734b2d59cc (the changeset description uses a different
> benchmark)).
> 
> Calling the new command "perfwrite2" to keep the existing command "perfwrite"
> unchanged is not very clever, but I didn’t have a better idea.

Maybe it's okay to remove the current perfwrite command so long as it can
be achieved by using the new implementation.

> In comparison to the other command, the new command is much more flexible. While
> using it, the focus was on testing small writes. For this reason, by default it
> calls ui.write() 100 times with a single byte plus one newline byte, for 100
> lines.
> 
> diff --git a/contrib/perf.py b/contrib/perf.py
> --- a/contrib/perf.py
> +++ b/contrib/perf.py
> @@ -3810,6 +3810,50 @@
>      fm.end()
>  
>  
> +@command(
> +    b'perfwrite2',
> +    formatteropts
> +    + [
> +        (b'', b'write-method', b'write', b'ui write method'),
> +        (b'', b'nlines', 100, b'number of lines'),
> +        (b'', b'nitems', 100, b'number of items (per line)'),
> +        (b'', b'item', b'x', b'item that is written'),
> +        (b'', b'batch-line', None, b'pass whole line to write method at once'),
> +        (b'', b'flush-line', None, b'flush after each line'),
> +    ],
> +)
> +def perfwrite2(ui, repo, testedtemplate=None, **opts):
                            ^^^^^^^^^^^^^^
Nit: unused argument?
Manuel Jacob - June 22, 2020, 11:13 a.m.
On 2020-06-22 12:58, Yuya Nishihara wrote:
> On Mon, 22 Jun 2020 02:02:34 +0200, Manuel Jacob wrote:
>> # HG changeset patch
>> # User Manuel Jacob <me@manueljacob.de>
>> # Date 1591314853 -7200
>> #      Fri Jun 05 01:54:13 2020 +0200
>> # Node ID 1d9781c4f9bcd2df5745b7e9d880dbd5b46c559e
>> # Parent  61719b9658b156548634c216ffb2b9188266293b
>> # EXP-Topic perfwrite2
>> perf: add another command to test performance of writing to output 
>> streams
>> 
>> The command was used recently while finding a solution for a buffering 
>> bug
>> (eventually fixed in f9734b2d59cc (the changeset description uses a 
>> different
>> benchmark)).
>> 
>> Calling the new command "perfwrite2" to keep the existing command 
>> "perfwrite"
>> unchanged is not very clever, but I didn’t have a better idea.
> 
> Maybe it's okay to remove the current perfwrite command so long as it 
> can
> be achieved by using the new implementation.

It would be `perfwrite2 --nlines=100000 --nitems=1 --item='Testing write 
performance' --batch-line`.

Do you think it would be a good idea to replace the old command (using 
the same name), but doing something different by default?

>> In comparison to the other command, the new command is much more 
>> flexible. While
>> using it, the focus was on testing small writes. For this reason, by 
>> default it
>> calls ui.write() 100 times with a single byte plus one newline byte, 
>> for 100
>> lines.
>> 
>> diff --git a/contrib/perf.py b/contrib/perf.py
>> --- a/contrib/perf.py
>> +++ b/contrib/perf.py
>> @@ -3810,6 +3810,50 @@
>>      fm.end()
>> 
>> 
>> +@command(
>> +    b'perfwrite2',
>> +    formatteropts
>> +    + [
>> +        (b'', b'write-method', b'write', b'ui write method'),
>> +        (b'', b'nlines', 100, b'number of lines'),
>> +        (b'', b'nitems', 100, b'number of items (per line)'),
>> +        (b'', b'item', b'x', b'item that is written'),
>> +        (b'', b'batch-line', None, b'pass whole line to write method 
>> at once'),
>> +        (b'', b'flush-line', None, b'flush after each line'),
>> +    ],
>> +)
>> +def perfwrite2(ui, repo, testedtemplate=None, **opts):
>                             ^^^^^^^^^^^^^^
> Nit: unused argument?
Yuya Nishihara - June 22, 2020, 11:34 a.m.
On Mon, 22 Jun 2020 13:13:04 +0200, Manuel Jacob wrote:
> On 2020-06-22 12:58, Yuya Nishihara wrote:
> > On Mon, 22 Jun 2020 02:02:34 +0200, Manuel Jacob wrote:
> >> # HG changeset patch
> >> # User Manuel Jacob <me@manueljacob.de>
> >> # Date 1591314853 -7200
> >> #      Fri Jun 05 01:54:13 2020 +0200
> >> # Node ID 1d9781c4f9bcd2df5745b7e9d880dbd5b46c559e
> >> # Parent  61719b9658b156548634c216ffb2b9188266293b
> >> # EXP-Topic perfwrite2
> >> perf: add another command to test performance of writing to output 
> >> streams
> >> 
> >> The command was used recently while finding a solution for a buffering 
> >> bug
> >> (eventually fixed in f9734b2d59cc (the changeset description uses a 
> >> different
> >> benchmark)).
> >> 
> >> Calling the new command "perfwrite2" to keep the existing command 
> >> "perfwrite"
> >> unchanged is not very clever, but I didn’t have a better idea.
> > 
> > Maybe it's okay to remove the current perfwrite command so long as it 
> > can
> > be achieved by using the new implementation.
> 
> It would be `perfwrite2 --nlines=100000 --nitems=1 --item='Testing write 
> performance' --batch-line`.
> 
> Do you think it would be a good idea to replace the old command (using 
> the same name), but doing something different by default?

Yes. I don't think perfwrite is used as a source parameter of performance
testing, and even if it were, the command argument can be easily adjusted.

The change can be documented in relnotes.

Patch

diff --git a/contrib/perf.py b/contrib/perf.py
--- a/contrib/perf.py
+++ b/contrib/perf.py
@@ -3810,6 +3810,50 @@ 
     fm.end()
 
 
+@command(
+    b'perfwrite2',
+    formatteropts
+    + [
+        (b'', b'write-method', b'write', b'ui write method'),
+        (b'', b'nlines', 100, b'number of lines'),
+        (b'', b'nitems', 100, b'number of items (per line)'),
+        (b'', b'item', b'x', b'item that is written'),
+        (b'', b'batch-line', None, b'pass whole line to write method at once'),
+        (b'', b'flush-line', None, b'flush after each line'),
+    ],
+)
+def perfwrite2(ui, repo, testedtemplate=None, **opts):
+    """microbenchmark ui.write (and others) with small writes
+    """
+    opts = _byteskwargs(opts)
+
+    write = getattr(ui, _sysstr(opts[b'write_method']))
+    nlines = int(opts[b'nlines'])
+    nitems = int(opts[b'nitems'])
+    item = opts[b'item']
+    batch_line = opts.get(b'batch_line')
+    flush_line = opts.get(b'flush_line')
+
+    if batch_line:
+        line = item * nitems + b'\n'
+
+    def benchmark():
+        for i in pycompat.xrange(nlines):
+            if batch_line:
+                write(line)
+            else:
+                for i in pycompat.xrange(nitems):
+                    write(item)
+                write(b'\n')
+            if flush_line:
+                ui.flush()
+        ui.flush()
+
+    timer, fm = gettimer(ui, opts)
+    timer(benchmark)
+    fm.end()
+
+
 def uisetup(ui):
     if util.safehasattr(cmdutil, b'openrevlog') and not util.safehasattr(
         commands, b'debugrevlogopts'