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

Emit measurement results immediately #203

Open
abrown opened this issue Oct 6, 2022 · 6 comments
Open

Emit measurement results immediately #203

abrown opened this issue Oct 6, 2022 · 6 comments

Comments

@abrown
Copy link
Collaborator

abrown commented Oct 6, 2022

@jameysharp mentioned in #202 (comment) that it would be nice if measurement results were emitted as soon as they were collected. Currently this is not the case: all the measurements for all of the benchmark runs are collected and emitted at the end. This issue could be fixable by refactoring how the measurements are serialized by serde.

@fitzgen
Copy link
Member

fitzgen commented Oct 12, 2022

Another thing we could do, just to provide feedback that something is happening and the process isn't just hanging, is to print a dot to stderr after each iteration (maybe also a newline after every 80 dots too).

@abrown
Copy link
Collaborator Author

abrown commented Oct 13, 2022

To take this one step further, I've been going back and forth about whether we should bring in a progress meter here. We know how many iterations need to be run so it's not hard to keep track of the percentage complete. I even took a look at crates.io at some options but they all seem snazzier (and bigger) than what is needed here. cargo has a built-in, single-file progress meter that could be used too. So, it's possible, but on the "con" side, this is just one more thing to maintain, which might break, etc., so I'm on the fence. @fitzgen (and others: @cfallin, @jameysharp), what do you think?

@cfallin
Copy link
Member

cfallin commented Oct 13, 2022

As someone who's spent a good amount of time staring at a sightglass invocation waiting for it to finish... some indication, even just "1% ... 2% ... 3% ... " textual output (adjust step size as desired), of how long I might be waiting would be very welcome! I've resorted to turning up RUST_LOG to see the individual invocations before to get some feedback.

@fitzgen
Copy link
Member

fitzgen commented Oct 13, 2022

I would personally want to avoid any fancy progress bar, since it seems like it might introduce unnecessary noise into the system and seems a bit overkill.

@abrown
Copy link
Collaborator Author

abrown commented Oct 13, 2022

Ok, no progress bars. One thing I just looked up is whether we could detect an ANSI terminal and just rewrite the same line; seems like it might work with \r and there may be some crates that can do this kind of thing.

@cfallin
Copy link
Member

cfallin commented Oct 13, 2022

Even simpler, just newlines? Perhaps with a --verbose flag we could emit a line after every run "Running compilation 1/N...", "Running instantiation 1/N...", "Running compilation 2/N...", etc?

abrown added a commit to abrown/sightglass that referenced this issue 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 bytecodealliance#203 and may close that issue.
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

No branches or pull requests

3 participants