Skip to content

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

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

rkuris
Copy link
Contributor

@rkuris rkuris commented Aug 21, 2025

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:

  • Estimating high early on, but gradually removing the fuzz as the load progresses. This code uses a 20% margin for this.
  • Using a rolling window of samples to see what the rate is. This prevents a few large or slow blocks from making a big impact on the estimate right way. This code uses a window size of 10.
  • Don't report estimates until we have some reasonable number of samples. Estimates will not be reported for about a minute with this code (10 samples at a minimum of 5s apart)
  • Include a percentage complete. I found myself running this calculation all the time and it's nice to see how far along it is as a percentage.

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.

@rkuris rkuris self-assigned this Aug 21, 2025
@rkuris rkuris added enhancement New feature or request storage This involves storage primitives labels Aug 21, 2025
@rkuris rkuris moved this to In Progress 🏗️ in avalanchego Aug 21, 2025
@rkuris rkuris marked this pull request as ready for review August 21, 2025 22:08
@Copilot Copilot AI review requested due to automatic review settings August 21, 2025 22:08
Copy link
Contributor

@Copilot Copilot AI left a 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
Copy link
Preview

Copilot AI Aug 21, 2025

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.

Suggested change
lowestSample: math.MaxUint64, // Initialize to maximum value
minCompleted: math.MaxUint64, // Initialize to maximum value

Copilot uses AI. Check for mistakes.

Copy link
Contributor Author

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.

rkuris and others added 6 commits August 21, 2025 15:09
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
Copy link
Contributor Author

@rkuris rkuris Aug 21, 2025

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.

@rkuris rkuris requested review from joshua-kim, aaronbuchwald and a team August 22, 2025 17:48
@joshua-kim joshua-kim requested review from a team and removed request for a team, alarso16, joshua-kim and aaronbuchwald August 22, 2025 19:30
@joshua-kim joshua-kim requested review from a team and DracoLi and removed request for a team August 22, 2025 19:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request storage This involves storage primitives
Projects
Status: In Progress 🏗️
Development

Successfully merging this pull request may close these issues.

1 participant