andrewrk / poop

Performance Optimizer Observation Platform
MIT License
788 stars 50 forks source link

align result columns on '±' and '…' #24

Closed dweiller closed 1 year ago

dweiller commented 1 year ago

NOTE: This PR depends on ziglang/zig#16093 or an equivalent being merged.

This change aligns the output columns nicely (closes #4, maybe #10):

Benchmark 1 (16 runs): ls:
  measurement             mean ± σ                 minmax       outliers     delta
  wall_time            1.539ms ± 518.139us    894.54us2.19ms    0 ( 0%)      0%
  peak_rss                  2M ± 55K                2M2M        0 ( 0%)      0%
  cpu_cycles            394324 ± 34256          358799472970    0 ( 0%)      0%
  instructions          399643 ± 33             399631399765    1 ( 6%)      0%
  cache_references       28690 ± 1332            2588631990     3 (19%)      0%
  cache_misses            9049 ± 328              87049867      2 (13%)      0%
  branch_misses           4873 ± 115              47205147      1 ( 6%)      0%
Benchmark 2 (12 runs): ls -R:
  measurement             mean ± σ             minmax        outliers           delta
  wall_time             4.14ms ± 1.33ms    2.356ms5.501ms    0 ( 0%)     💩 +169.0% ± 48.5%
  peak_rss                  2M ± 61K            2M2M         0 ( 0%)        +  2.0% ±  1.7%
  cpu_cycles           1594313 ± 124837    14784791901268    0 ( 0%)     💩 +304.3% ± 17.0%
  instructions         2503325 ± 50        25032942503435    0 ( 0%)     💩 +526.4% ±  0.0%
  cache_references      134853 ± 3338       130399142218     0 ( 0%)     💩 +370.0% ±  6.6%
  cache_misses           11182 ± 635         1007312195      0 ( 0%)     💩 + 23.6% ±  4.2%
  branch_misses          14396 ± 323         1401614941      0 ( 0%)     💩 +195.4% ±  3.7%

Note that the columns of different benchmarks are no longer aligned - doing this would require either only printing results after all benchmarks have run, or making the columns wide enough that we can hope/pray the numbers fit inside them (and hence be much wider than needed when number are smaller).

Possible future improvements could be properly centering the 'outliers' and 'delta' headers.

candrewlee14 commented 1 year ago

I think alignment of the columns across benchmarks is actually really helpful for viewing...

Could we limit the number of sigfigs shown for numbers so we know the max width of each column at comptime?

dweiller commented 1 year ago

I think alignment of the columns across benchmarks is actually really helpful for viewing...

Could we limit the number of sigfigs shown for numbers so we know the max width of each column at comptime?

While I agree it would look nicer, I personally don't find it that helpful, so I would rather leave this to a follow up PR. I think it may not be so easy to pick a static column width, and using one will likely require a strategy for handling number too large to fit as well. For reference the current widths of the mean and outliers columns on master are both small enough that I have gotten underflows on some usages.

It would be straightforward to make the columns have the same with by delaying all printing until the end, but I think it's more important to see the results as they are ready.

dweiller commented 1 year ago

Looks like the CI ran with a version of zig that is 5 commits too old to include the fmtDuration changes.

andrewrk commented 1 year ago

let's try again tomorrow

andrewrk commented 1 year ago

I believe this was solved by #30

dweiller commented 1 year ago

I believe this was solved by #30

For the most part yes. There might be a few things to take/rework from here e.g. if we want the range columns to be aligned to the central character or preventing underflows and making sure columns are aligned when the values get larger than is covered by #30. But given #30 is in I'll wait until I experience these issues before looking at fixing them and open a new PR if/when that happens.