Skip to content
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.

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

Store computed metrics in BenchmarkResult and move report/pretty-print functionality into BenchmarkResult #38

Merged
merged 4 commits into from
Feb 2, 2024

Conversation

Bryysen
Copy link
Collaborator

@Bryysen Bryysen commented Jan 13, 2024

Also added a total-time metric which represents how long the entire benchmark (all runs included) took to run. This is displayed when reporting/pretty printing. Also notably; we no longer print the result of a run after completion, the user must explicitly call prettyPrint on the BenchmarkResults instance. Updated examples to reflect this.

report/pretty-print functionality into BenchmarkResult

- Update examples

- Add total-time metric and display it when pretty-printing
@Bryysen
Copy link
Collaborator Author

Bryysen commented Jan 13, 2024

This can be considered (1/3) of the run overhaul. Note we deviate from https://pkg.go.dev/testing#hdr-Benchmarks by no longer pretty-printing/reporting right after a benchmark is complete. There are two reasons for why I think we should do that:

  1. If a user runs the benchmark in a multithreaded context and each thread starts writing to stdout we can get race-conditions (we are not locking before writing to stdout). With this change they can now let the threads complete and join, and print the results from the main thread.
  2. In the future more parsing options may be added, and the user might not want to print to stdout at all.

Copy link
Owner

@hendriknielaender hendriknielaender left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent progress. The printing logic is quite lengthy and involves multiple steps. It might be beneficial to break this down into smaller, reusable functions for improved readability and maintainability. What do you think?

zbench.zig Outdated Show resolved Hide resolved
zbench.zig Outdated Show resolved Hide resolved
@Bryysen
Copy link
Collaborator Author

Bryysen commented Jan 14, 2024

Excellent progress. The printing logic is quite lengthy and involves multiple steps. It might be beneficial to break this down into smaller, reusable functions for improved readability and maintainability. What do you think?

I agree with this. In the future we may add functionality for the user to choose which metrics to display, having split everything up into individual functions makes that easier. I'll address the feedback as soon as I have some time.

- Split BenchmarkResult.prettyPrint into individual functions
  with respect to each metric to be pretty-printed

- Propagate choice of writer for pretty-print functions within
  BenchmarkResult

- Add a buffered std-out writer as a field of BenchmarkResults. We
  use this for all formatted printing-operations within BenchmarkResults.

- Make use of BenchmarkResults.getColor to color-code the total-time metric
  in BenchmarkResults.prettyPrint

- Add various doc-comments
@Bryysen
Copy link
Collaborator Author

Bryysen commented Jan 17, 2024

Note: In 3313c40 I make use of BenchmarkResults.getColor the way I imagine it was initially intended to be used, however I haven't touched on the function internally nor have I checked that it does what we want it to when printing multiple results. In general we need more tests but I'm putting that off for now since there are a lot of changes in the works.

Edit: By the way if you don't agree with letting the caller select the writer let me know and I'll revert to using stdout directly in the pretty-print functions.

zbench.zig Outdated Show resolved Hide resolved
zbench.zig Outdated Show resolved Hide resolved
Copy link
Owner

@hendriknielaender hendriknielaender left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a couple of small things, but really cool stuff. 👍

- Rename BenchmarkResult.percs to BenchmarkResult.percentiles
@hendriknielaender hendriknielaender merged commit 1389ebf into hendriknielaender:main Feb 2, 2024
5 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants