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

Display benchmark progress #209

Closed
wants to merge 2 commits into from

Conversation

abrown
Copy link
Collaborator

@abrown abrown commented Oct 24, 2022

This change emits a string to stderr that indicates how far Sightglass has come in running its benchmarks:

Iterations completed: 42/100

The idea is to provide more immediate and clear feedback than RUST_LOG that Sightglass is doing work and the user can wait for it to finish. This turns out to be rather tricky to do due to the single-process and multi-process paradigms for running benchmarks; the solution is to communicate here with an internal environment variable "protocol."

The other tricky bit is the addition of ANSI terminal escape codes. This allows Sightglass to overwrite the same line and avoid filling up a screen with repeated Iterations completed: ... messages. This part of the PR could be safely removed without change to the counting functionality but I hope that the implementation is simple enough--and pessimistic enough--that it should not cause trouble down the road. I say "pessimistic" because the heuristic for determining if a terminal can support these "overwriting" terminal codes should just default to printing extra lines if anything looks amiss.

This PR is related to #203 and may close that issue.

This change emits a string to `stderr` that indicates how far Sightglass
has come in running its benchmarks:

```
Iterations completed: 42/100
```

The idea is to provide more immediate and clear feedback than `RUST_LOG`
that Sightglass is doing work and the user can wait for it to finish.
This turns out to be rather tricky to do due to the single-process and
multi-process paradigms for running benchmarks; the solution is to
communicate here with an internal environment variable "protocol."

The other tricky bit is the addition of ANSI terminal escape codes. This
allows Sightglass to overwrite the same line and avoid filling up a
screen with repeated `Iterations completed: ...` messages. This part of
the PR could be safely removed without change to the counting
functionality but I hope that the implementation is simple enough--and
pessimistic enough--that it should not cause trouble down the road. I
say "pessimistic" because the heuristic for determining if a terminal
can support these "overwriting" terminal codes should just default to
printing extra lines if anything looks amiss.

This PR is related to bytecodealliance#203 and may close that issue.
@abrown abrown requested a review from fitzgen October 24, 2022 23:46
@abrown
Copy link
Collaborator Author

abrown commented Oct 24, 2022

cc: @cfallin, @jlb6740, @jameysharp.

Copy link
Member

@fitzgen fitzgen left a comment

Choose a reason for hiding this comment

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

This is... a lot of code for printing some bits to stderr to show that something is actually happening. Especially when we start interacting with RUST_LOG and all that. I've also found tty detection to be fairly flaky in the past (e.g. doesn't work properly with terminals inside other tools, like Emacs). The communication between parent and child processes, etc.

Also: do the terminal escape codes work on windows too?

I feel like we can get 95% of the benefit with 1% of the code if we just do

eprintln!(".");

in the execute_in_multiple_processes function's loop after spawning each child process. This would give us a . after every process completes, which is 10 iterations by default. That seems to satisfy the "I can't tell if it is doing anything" use case just fine by me. If we really want more information, we could print Process N/M complete. or something like that. But we really don't need all the bells and whistles just to get some feedback...

@fitzgen
Copy link
Member

fitzgen commented Oct 25, 2022

Opened #211 as an alternative approach

@abrown abrown closed this Mar 22, 2023
@abrown abrown deleted the track-iterations branch March 22, 2023 18:22
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