-
Notifications
You must be signed in to change notification settings - Fork 778
fix: time remaining should use better estimates #4196
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR improves the ETA estimation for bootstrapping operations by replacing the simple time-based calculation with a more sophisticated rolling window approach that provides better accuracy and user experience.
- Introduces a new
EtaTracker
struct that uses a rolling window of samples and applies a slowdown factor for more accurate time estimates - Replaces direct
EstimateETA
calls with the new tracker in both bootstrap execution and fetching phases - Adds percentage completion reporting alongside ETA estimates
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
File | Description |
---|---|
utils/timer/eta.go | Implements new EtaTracker with rolling window sampling and slowdown factor for improved ETA accuracy |
utils/timer/eta_test.go | Adds comprehensive test coverage for the new ETA tracking functionality |
snow/engine/snowman/bootstrap/storage.go | Integrates EtaTracker for block execution progress with percentage reporting |
snow/engine/snowman/bootstrap/bootstrapper.go | Replaces simple ETA calculation with EtaTracker for block fetching progress |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
samples: make([]sample, maxSamples), | ||
samplePosition: 0, | ||
maxSamples: maxSamples, | ||
lowestSample: math.MaxUint64, // Initialize to maximum value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The field name 'lowestSample' is misleading since it's initialized to the maximum value and tracks the minimum completed value. Consider renaming to 'minCompleted' or 'baselineCompleted' for clarity.
lowestSample: math.MaxUint64, // Initialize to maximum value | |
minCompleted: math.MaxUint64, // Initialize to maximum value |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, I think it's the lowestSample. Perhaps it's also minCompleted but lowestSample better represents how it is used in the code.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Ron Kuris <swcafe@gmail.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Ron Kuris <swcafe@gmail.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Ron Kuris <swcafe@gmail.com>
now := time.Now() | ||
target := uint64(1000) | ||
|
||
thirdSampleEta := 80 * time.Second |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stupid go question: is there a better way to code this? I am only using this later to point to it.
Why this should be merged
This code does a much better job of estimating completion time, which improves my experience using avalanchego for bootstrapping.
How this works
Correctly computing an estimate involves:
How this was tested
Too many bootstraps. The 20% margin is a wild guess, but I wrote some code to show how much fuzz makes sense, and settled on 10 samples. It was not tested on a very slow machine.
Need to be documented in RELEASES.md?
Not a bad idea.