Shared benchmark format for command line and CSV outputs #989

siv2r commented at 2:11 AM on October 15, 2021: contributor

Fixes #680 Change: Shared benchmark formats for command line and CSV outputs

Before: (only some part of the bench_internal output is shown here)

scalar_add: min 0.0319us / avg 0.0334us / max 0.0417us
scalar_negate: min 0.0162us / avg 0.0162us / max 0.0162us
scalar_mul: min 0.123us / avg 0.124us / max 0.124us
scalar_split: min 0.580us / avg 0.582us / max 0.584us
scalar_inverse: min 6.28us / avg 6.28us / max 6.29us
scalar_inverse_var: min 4.63us / avg 4.65us / max 4.69us

After:

Benchmark                      Min(μs)        Avg(μs)        Max(μs)       
----------------------------------------------------------------------
scalar_add                     0.0190         0.0194         0.0225         
scalar_negate                  0.00947        0.00967        0.0109         
scalar_mul                     0.0741         0.0744         0.0761         
scalar_split                   0.344          0.347          0.368          
scalar_inverse                 3.81           3.82           3.84           
scalar_inverse_var             2.67           2.68           2.73

Edit: This format is outdated. You can find the new format in this thread below.

These are all my benchmark outputs: (after formatting)

Edit: These benchmark output files are outdated. You can find the updated files in this thread below.

siv2r force-pushed on Oct 15, 2021

in src/bench.h:133 in 4f7a62185f outdated

 127 | @@ -130,4 +128,11 @@ int get_iters(int default_iters) {
 128 |      }
 129 |  }
 130 |  
 131 | +void print_output_table_header_row(void) {
 132 | +    int i;
 133 | +    printf("%-30s %-15s %-15s %-15s\n", "Benchmark", "Min(μs)", "Avg(μs)", "Max(μs)");

real-or-random commented at 10:09 AM on October 15, 2021:

    printf("%-30s %-15s %-15s %-15s\n", "Benchmark", "Min(us)", "Avg(us)", "Max(us)");

I think we should stick to ASCII chars here, otherwise the output depends on your local charset. (And the Unicode/wicechar support in C89 is pretty limited. We'd need wprintf but that's not in C89.)

siv2r commented at 4:13 PM on October 15, 2021:

Incorporated this change :)

real-or-random commented at 10:09 AM on October 15, 2021: contributor

Thanks, that looks nice! Concept ACK

siv2r force-pushed on Oct 15, 2021

real-or-random commented at 4:45 PM on October 15, 2021: contributor

I think now the offset is somewhat broken. At least on my machine, the number are not exactly below headers:

Benchmark                      Min(us)         Avg(us)         Max(us)        
----------------------------------------------------------------------
ecdsa_sign                     40.9           41.0           41.3

Also, can we make sure that the . is always in the same column? See for example the last line in the output of bench_internal. I think a simple solution is just to reserve 4 or 5 digits left of the ., which should be fine for most users. :)

siv2r commented at 5:12 PM on October 15, 2021: contributor

I think now the offset is somewhat broken. At least on my machine, the number are not exactly below headers:

I will look into this.

Also, can we make sure that the . is always in the same column?

I didn't notice this issue. Thanks! Currently, I simply left justified the output of print_number function. I will read more about the fixed point arithmetic used here and get this done.

real-or-random commented at 9:59 PM on October 15, 2021: contributor

Currently, I simply left justified the output of print_number function. I will read more about the fixed point arithmetic used here and get this done.

The arithmetic is a little bit strange admittedly. We really wanted to avoid floating point types in the entire library. But I think you simply need to have some parameter w for decimal places left of the decimal point and count that down in the do loop. If it's then greater than 0 after the loop, print w spaces.

siv2r force-pushed on Oct 18, 2021

siv2r commented at 9:15 AM on October 18, 2021: contributor

I aligned the decimal points in the same column (new bench_internal), but this breaks the format since we are padding the integer part.

we could pad with 0 instead of space to maintain the format but that might make the value hard to read. (shown below)

Benchmark                     Min(us)        Avg(us)        Max(us)        
----------------------------------------------------------------------
scalar_add                    0000.0189      0000.0190      0000.0193     
scalar_negate                 0000.00947     0000.00948     0000.00949    
scalar_mul                    0000.0741      0000.0742      0000.0744     
scalar_split                  0000.344       0000.345       0000.348      
scalar_inverse                0003.80        0003.82        0003.88

What do you suggest? Should we revert back to the old format or pad with 0s?

real-or-random commented at 9:23 AM on October 18, 2021: contributor

I aligned the decimal points in the same column (new bench_internal), but this breaks the format since we are padding the integer part.

I don't understand, why does it break the format?

siv2r commented at 9:33 AM on October 18, 2021: contributor

Benchmark                 Min(us)        Avg(us)        Max(us)        
|-----30------------------|-----15-------|------15------|------15------|

This is how I divided the width for each column. Now let's consider the Min(us) column. For aligning the decimal points I gave 5 width for the integer part and 10 width for the fractional part of the total 15 width available for Min(us).

Min(us)
|--------15------|
0.678 (old format) -----> proper alignment
    0.678 (4 spaces before zero since the integer width is 5)  ---> not proper alignment
|--5-|----10-----|

real-or-random commented at 9:58 AM on October 18, 2021: contributor

Oh okay, I think that's not a big deal. It looks slightly off but you could move the column headers to the center of the column.

We could even go a step further and a commas as column separators, and remove/replace the --- line. Then you get file that at least LibreOffice can import correctly as CSV (it parses the numbers correctly if you don't select "Trim spaces"). Don't know about about Excel, that would be interesting to know.

Benchmark                     ,    Min(us)    ,    Avg(us)    ,    Max(us)        

scalar_add                    ,    0.0189     ,    0.0190     ,    0.0193     
scalar_negate                 ,    0.00947    ,    0.00948    ,    0.00949    
scalar_mul                    ,    0.0741     ,    0.0742     ,    0.0744     
scalar_split                  ,    0.344      ,    0.345      ,    0.348      
[...]

That would also solve #680. To be honest, that's we really need in practice. And I think this shared format is pragmatic and much simpler than having command-line options. Otherwise you run a benchmark for say 10 min and only then you figure you want it as CSV. And if people really want "better" CSV format, they can still strip spaces manually by piping the output to some almost trivial command.

siv2r commented at 10:15 AM on October 18, 2021: contributor

I was planning to work on the command line CSV option after this.

this shared format is pragmatic and much simpler than having command-line options.

Then, I will do this.

As you said, we could also mention the UNIX commands to get a better CSV format from this command line output (something like this) on the readme.

real-or-random commented at 11:58 AM on October 18, 2021: contributor

As you said, we could also mention the UNIX commands to get a better CSV format from this command line output (something like this) on the readme.

Indeed, ./bench_foo | tr -d ' ' will do. Even if we don't add commas as column separators, ./bench_foo | sed 's/ \{1,\}/,/g' will add the commas... So that's another possibility.

siv2r force-pushed on Oct 19, 2021

siv2r commented at 1:41 AM on October 19, 2021: contributor

Now the command line output looks like this:

Benchmark                     ,    Min(us)    ,    Avg(us)    ,    Max(us)    

scalar_add                    ,     0.0189    ,     0.0191    ,     0.0204    
scalar_negate                 ,     0.00947   ,     0.00948   ,     0.00949   
scalar_mul                    ,     0.0741    ,     0.0742    ,     0.0743    
scalar_split                  ,     0.344     ,     0.344     ,     0.346     
scalar_inverse                ,     3.80      ,     3.81      ,     3.87      
scalar_inverse_var            ,     2.67      ,     2.70      ,     2.94

The command line output for all available benchmarks can be found here.

This is the CSV file from the command line output for bench_internal.

csv file generation: ./bench_internal | sed 2d | tr -d ' ' > bench_internal.csv

siv2r renamed this:
~~Format benchmark command line output~~
Shared benchmark format for command line and CSV outputs
on Oct 19, 2021

in README.md:113 in a6deb40121 outdated

 108 | +
 109 | +    $ ./bench_name
 110 | +
 111 | +To create a CSV file for the benchmark result :
 112 | +
 113 | +    $ ./bench_name | sed 2d | tr -d ' ' > bench_name.csv

real-or-random commented at 9:12 AM on October 19, 2021:

    $ ./bench_name | sed '2d;s/ \{1,\}//g' > bench_name.csv

If we anyway rely on sed this does not need tr.

in README.md:105 in a6deb40121 outdated

  99 | @@ -100,6 +100,18 @@ To create a HTML report with coloured and annotated source code:
 100 |      $ mkdir -p coverage
 101 |      $ gcovr --exclude 'src/bench*' --html --html-details -o coverage/coverage.html
 102 |  
 103 | +Benchmark
 104 | +------------
 105 | +Binaries for benchmarking the libsecp256k1 functions will be present in the root directory after the build.

real-or-random commented at 9:16 AM on October 19, 2021:

If configured with `--enable-benchmark` (which is the default), binaries for benchmarking the libsecp256k1 functions will be present in the root directory after the build.

in src/bench.h:78 in a6deb40121 outdated

  76 |      if (x < 0) {
  77 |          buffer[--ptr] = '-';
  78 | +        g++;
  79 |      }
  80 | -    printf("%s", &buffer[ptr]);
  81 | +    printf(", "); /* for center alignment */

real-or-random commented at 9:18 AM on October 19, 2021:

    printf(", "); /* as a column delimiter */

I think that's easier to grasp if you haven't read the discussion in this PR.

in src/bench.h:81 in a6deb40121 outdated

  79 |      }
  80 | -    printf("%s", &buffer[ptr]);
  81 | +    printf(", "); /* for center alignment */
  82 | +    printf("%5.*s", g, &buffer[ptr]); /* Prints integer part */
  83 | +    printf("%-*s", FP_EXP, &buffer[ptr + g]); /* Prints fractional part */
  84 | +    printf("   ");

real-or-random commented at 9:22 AM on October 19, 2021:

I think it would be slightly nicer to move the printf(", ") and printf(" ") out of this function because conceptually they don't belong to print_number. This would also make it easier to avoid the three spaces at the end of the line (trailing whitespace)

real-or-random commented at 9:23 AM on October 19, 2021: contributor

Thanks. I have a few remaining nits (sorry to bother you again!) but this looks really nice now!

Shared benchmark format for command line and CSV outputs

1. add `print_output_table_header_row` func to print the table header for benchmark output
2. modify the following benchmarks to include the table header
    - bench_ecdh.c
    - bench_ecmult.c
    - bench_internal.c
    - bench_recover.c
    - bench_schnorrsig.c
    - bench_sign.c
    - bench_verify.c

26a255beb6

create csv file from the benchmark output b4b130678d

siv2r force-pushed on Oct 19, 2021

siv2r commented at 4:04 PM on October 19, 2021: contributor

I have a few remaining nits (sorry to bother you again!)

No worries, I am learning a lot in this process. I have incorporated the new suggestions.

real-or-random approved

real-or-random commented at 2:59 PM on October 20, 2021: contributor

ACK b4b130678db31a7cabc2cde091bc4acbca92b7a3

jonasnick commented at 12:27 PM on October 22, 2021: contributor

ACK b4b130678db31a7cabc2cde091bc4acbca92b7a3

Imports of the raw output (without sed) work fine with Google sheets. Thank you @siv2r.

jonasnick merged this on Oct 22, 2021

jonasnick closed this on Oct 22, 2021

siv2r deleted the branch on Oct 23, 2021

sipa referenced this in commit f727914d7e on Oct 28, 2021

sipa referenced this in commit 440f7ec80e on Oct 31, 2021

sipa referenced this in commit d057eae556 on Dec 2, 2021

fanquake referenced this in commit c4a1e09a8c on Dec 3, 2021

sipa referenced this in commit 86dbc4d075 on Dec 15, 2021

jonasnick cross-referenced this on Jan 2, 2022 from issue Sync Upstream by jonasnick

real-or-random referenced this in commit 21e2d65b79 on Jan 5, 2022

gwillen referenced this in commit 35d6112a72 on May 25, 2022

janus referenced this in commit 879a9a27b9 on Jul 10, 2022

patricklodder referenced this in commit 21badcf9d2 on Jul 25, 2022

patricklodder referenced this in commit 03002a9013 on Jul 28, 2022

backpacker69 referenced this in commit 77186f4a04 on Jan 18, 2023

str4d referenced this in commit 6de4698bf9 on Apr 21, 2023

vmta referenced this in commit e1120c94a1 on Jun 4, 2023

vmta referenced this in commit 8f03457eed on Jul 1, 2023