Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

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

wants to merge 2 commits into from

Conversation

@dweiller
Copy link

@dweiller dweiller commented Jun 19, 2023 •
edited
Loading

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 ± σ                 min … max       outliers     delta
  wall_time            1.539ms ± 518.139us    894.54us … 2.19ms    0 ( 0%)      0%
  peak_rss                  2M ± 55K                2M … 2M        0 ( 0%)      0%
  cpu_cycles            394324 ± 34256          358799 … 472970    0 ( 0%)      0%
  instructions          399643 ± 33             399631 … 399765    1 ( 6%)      0%
  cache_references       28690 ± 1332            25886 … 31990     3 (19%)      0%
  cache_misses            9049 ± 328              8704 … 9867      2 (13%)      0%
  branch_misses           4873 ± 115              4720 … 5147      1 ( 6%)      0%
Benchmark 2 (12 runs): ls -R:
  measurement             mean ± σ             min … max        outliers           delta
  wall_time             4.14ms ± 1.33ms    2.356ms … 5.501ms    0 ( 0%)     💩 +169.0% ± 48.5%
  peak_rss                  2M ± 61K            2M … 2M         0 ( 0%)        +  2.0% ±  1.7%
  cpu_cycles           1594313 ± 124837    1478479 … 1901268    0 ( 0%)     💩 +304.3% ± 17.0%
  instructions         2503325 ± 50        2503294 … 2503435    0 ( 0%)     💩 +526.4% ±  0.0%
  cache_references      134853 ± 3338       130399 … 142218     0 ( 0%)     💩 +370.0% ±  6.6%
  cache_misses           11182 ± 635         10073 … 12195      0 ( 0%)     💩 + 23.6% ±  4.2%
  branch_misses          14396 ± 323         14016 … 14941      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.

@dweiller dweiller mentioned this pull request Jun 20, 2023
Open
@candrewlee14
Copy link
Contributor

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?

@candrewlee14 candrewlee14 mentioned this pull request Jun 20, 2023
Closed
@dweiller
Copy link
Author

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
Copy link
Author

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

@andrewrk
Copy link
Owner

let's try again tomorrow

@candrewlee14 candrewlee14 mentioned this pull request Jun 21, 2023
Merged
@andrewrk
Copy link
Owner

I believe this was solved by #30

@andrewrk andrewrk closed this Jun 22, 2023
@dweiller
Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

align stuff better

3 participants

@dweiller @candrewlee14 @andrewrk