This is an archive of the discontinued Mercurial Phabricator instance.

progress: make ETA only consider progress made in the last minute
ClosedPublic

Authored by quark on Sep 26 2017, 3:56 PM.

Details

Summary

This patch limits the estimate time interval to roughly the last minute
(configurable by estimateinterval) to be more practical. See the test
change for why this is better.

.. feature:: Estimated time is more accurate with non-linear progress

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

quark created this revision.Sep 26 2017, 3:56 PM
quark edited the summary of this revision. (Show Details)Sep 26 2017, 5:24 PM
quark retitled this revision from progress: make ETA consider the last minute progress to progress: make ETA only consider progress made in the last minute.
mbthomas accepted this revision.Sep 27 2017, 9:56 AM
mbthomas added a subscriber: mbthomas.
mbthomas added inline comments.
mercurial/progress.py
261–262

I think this is saying: "if we suddenly start going really slowly (e.g. a stall), don't give wildly high estimates." Is that right?

Thanks for the patch - the current behavior has bothered me as well and we should improve the time estimation algorithm for reasons you've outlined.

Is there a reason you went with 60s? Have you experimented with shorter values? (Obviously this can be bikeshedded later because it is a config option and easy to change. I'm just curious where magic numbers come from.)

I'm also not sure why progress.estimate is experimental. I think we should remove that label.

mercurial/progress.py
108

Do we not have a configfloat()? This is being used in the context of non-integer math, so IMO it should be a float.

260

Nit: maybe just cast something to a float so we can avoid the useless multiply by 1.0?

quark added a comment.EditedSep 27 2017, 2:25 PM

Is there a reason you went with 60s? Have you experimented with shorter values? (Obviously this can be bikeshedded later because it is a config option and easy to change. I'm just curious where magic numbers come from.)

Intuitively, shorter values can make things less smooth (xkcd612). That said, 60s was not chosen very scientifically. If it has to come from something, maybe the first interval of uptime. I have that uptime value displayed in my i3-status.

I'm also not sure why progress.estimate is experimental. I think we should remove that label.

That's what I thought too. I'll send a follow-up.

mercurial/progress.py
108

Thanks. I wasn't aware configfloat is available. It should be used here.

260

Right. configfloat will solve that.

261–262

Yes.

quark updated this revision to Diff 2131.Sep 27 2017, 4:03 PM
quark edited the summary of this revision. (Show Details)Sep 27 2017, 6:15 PM
durin42 accepted this revision.Sep 27 2017, 7:40 PM
This revision is now accepted and ready to land.Sep 27 2017, 7:40 PM
This revision was automatically updated to reflect the committed changes.