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

Add keys per second display #8

Merged
merged 5 commits into from
Feb 11, 2023
Merged

Conversation

ar1a
Copy link
Contributor

@ar1a ar1a commented Jan 17, 2023

This PR adds a display of the live keys/second.

In the second commit, I pull in a library to add commas into the global_counter display, if you don't want to add a dependency just for this i'm happy to get this merged without that commit!

@delan
Copy link

delan commented Jan 18, 2023

Playing with this in a long-running process (700 million so far at 134 k/s), the displayed key rate seems to respond very slowly to drastic changes in available cpu time, gradually stepping up or down by 0.01 k/s. I think this is because we’re calculating the average key rate over the whole run.

What do you think about counting how many keys were processed since the last progress output, then dividing that by the time elapsed since that output (should be 0.25 s, but check monotonic time for accuracy)?

@ar1a
Copy link
Contributor Author

ar1a commented Jan 18, 2023

That was my initial solution, but I had a look at a similar project and they did it this way, so i mimicked their behaviour.

// instead of starting at 0 and taking many seconds to tend towards the
// actual key rate
if oldCounter == 0 {
avgKeyRate = float64(global_counter)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

every few runs, this hits a race condition and stays at 0. i figure this is fine, considering after a few seconds it tends towards the actual number anyways

@ar1a
Copy link
Contributor Author

ar1a commented Jan 18, 2023

alright, i've switched it to an exponential moving average

@danielewood
Copy link
Owner

I'm happy with the changes, I did a pretty major overhaul with optional json output, but I'll make sure to preserve your changes to the stdout that runs by default.

@danielewood danielewood merged commit 12dc19a into danielewood:master Feb 11, 2023
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.

3 participants