This is an archive of the discontinued Mercurial Phabricator instance.

Differential D820

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

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

Details

Reviewers
mbthomas
durin42
Group Reviewers
hg-reviewers
Commits
rHGa667f0ca1d5f: progress: make ETA only consider progress made in the last minute
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?

indygreg added a subscriber: indygreg.Sep 27 2017, 1:05 PM

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
Closed by commit rHGa667f0ca1d5f: progress: make ETA only consider progress made in the last minute (authored by quark). · Explain WhySep 27 2017, 7:48 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 2131

mercurial/configitems.py

default=False, default=False,
) )
coreconfigitem('progress', 'delay', coreconfigitem('progress', 'delay',
default=3, default=3,
) )
coreconfigitem('progress', 'disable', coreconfigitem('progress', 'disable',
default=False, default=False,
) )
coreconfigitem('progress', 'estimateinterval',
default=60.0,
)
coreconfigitem('progress', 'refresh', coreconfigitem('progress', 'refresh',
default=0.1, default=0.1,
) )
coreconfigitem('progress', 'width', coreconfigitem('progress', 'width',
default=dynamicdefault, default=dynamicdefault,
) )
coreconfigitem('push', 'pushvars.server', coreconfigitem('push', 'pushvars.server',
default=False, default=False,

mercurial/help/config.txt

``delay`` ``delay``
Number of seconds (float) before showing the progress bar. (default: 3) Number of seconds (float) before showing the progress bar. (default: 3)
``changedelay`` ``changedelay``
Minimum delay before showing a new topic. When set to less than 3 * refresh, Minimum delay before showing a new topic. When set to less than 3 * refresh,
that value will be used instead. (default: 1) that value will be used instead. (default: 1)
``estimateinterval``
Maximum sampling interval in seconds for speed and estimated time
calculation. (default: 60)
``refresh`` ``refresh``
Time in seconds between refreshes of the progress bar. (default: 0.1) Time in seconds between refreshes of the progress bar. (default: 0.1)
``format`` ``format``
Format of the progress bar. Format of the progress bar.
Valid entries for the format field are ``topic``, ``bar``, ``number``, Valid entries for the format field are ``topic``, ``bar``, ``number``,
``unit``, ``estimate``, ``speed``, and ``item``. ``item`` defaults to the ``unit``, ``estimate``, ``speed``, and ``item``. ``item`` defaults to the

mercurial/progress.py

self.refresh = float(self.ui.config( self.refresh = float(self.ui.config(
'progress', 'refresh')) 'progress', 'refresh'))
self.changedelay = max(3 * self.refresh, self.changedelay = max(3 * self.refresh,
float(self.ui.config( float(self.ui.config(
'progress', 'changedelay'))) 'progress', 'changedelay')))
self.order = self.ui.configlist( self.order = self.ui.configlist(
'progress', 'format', 'progress', 'format',
default=['topic', 'bar', 'number', 'estimate']) default=['topic', 'bar', 'number', 'estimate'])
self.estimateinterval = self.ui.configwith(
float, 'progress', 'estimateinterval')
indygregUnsubmitted
Not Done

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

indygreg: Do we not have a configfloat()? This is being used in the context of non-integer math, so IMO…
quarkAuthorUnsubmitted
Not Done

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

quark: Thanks. I wasn't aware `configfloat` is available. It should be used here.
def show(self, now, topic, pos, item, unit, total): def show(self, now, topic, pos, item, unit, total):
if not shouldprint(self.ui): if not shouldprint(self.ui):
return return
termwidth = self.width() termwidth = self.width()
self.printed = True self.printed = True
head = '' head = ''
needprogress = False needprogress = False
# not a topic change # not a topic change
or self.curtopic == self.lasttopic or self.curtopic == self.lasttopic
# it's been long enough we should print anyway # it's been long enough we should print anyway
or now - self.lastprint >= self.changedelay): or now - self.lastprint >= self.changedelay):
return True return True
else: else:
return False return False
def _calibrateestimate(self, topic, now, pos):
'''Adjust starttimes and startvals for topic so ETA works better
If progress is non-linear (ex. get much slower in the last minute),
it's more friendly to only use a recent time span for ETA and speed
calculation.
[======================================> ]
^^^^^^^
estimateinterval, only use this for estimation
'''
interval = self.estimateinterval
if interval <= 0:
return
elapsed = now - self.starttimes[topic]
if elapsed > interval:
delta = pos - self.startvals[topic]
newdelta = delta * interval / elapsed
indygregUnsubmitted
Not Done

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

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

Right. configfloat will solve that.

quark: Right. `configfloat` will solve that.
# If a stall happens temporarily, ETA could change dramatically
# frequently. This is to avoid such dramatical change and make ETA
mbthomasUnsubmitted
Not Done

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?

mbthomas: I think this is saying: "if we suddenly start going really slowly (e.g. a stall), don't give…
quarkAuthorUnsubmitted
Not Done

Yes.

quark: Yes.
# smoother.
if newdelta < 0.1:
return
self.startvals[topic] = pos - newdelta
self.starttimes[topic] = now - interval
def progress(self, topic, pos, item='', unit='', total=None): def progress(self, topic, pos, item='', unit='', total=None):
now = time.time() now = time.time()
self._refreshlock.acquire() self._refreshlock.acquire()
try: try:
if pos is None: if pos is None:
self.starttimes.pop(topic, None) self.starttimes.pop(topic, None)
self.startvals.pop(topic, None) self.startvals.pop(topic, None)
self.topicstates.pop(topic, None) self.topicstates.pop(topic, None)
self.lasttopic = None self.lasttopic = None
else: else:
if topic not in self.topics: if topic not in self.topics:
self.starttimes[topic] = now self.starttimes[topic] = now
self.startvals[topic] = pos self.startvals[topic] = pos
self.topics.append(topic) self.topics.append(topic)
self.topicstates[topic] = pos, item, unit, total self.topicstates[topic] = pos, item, unit, total
self.curtopic = topic self.curtopic = topic
self._calibrateestimate(topic, now, pos)
if now - self.lastprint >= self.refresh and self.topics: if now - self.lastprint >= self.refresh and self.topics:
if self._oktoprint(now): if self._oktoprint(now):
self.lastprint = now self.lastprint = now
self.show(now, topic, *self.topicstates[topic]) self.show(now, topic, *self.topicstates[topic])
finally: finally:
self._refreshlock.release() self._refreshlock.release()

tests/test-progress.t

loop [=> ] 1/20 6m21s\r (no-eol) (esc) loop [=> ] 1/20 6m21s\r (no-eol) (esc)
loop [===> ] 2/20 6m01s\r (no-eol) (esc) loop [===> ] 2/20 6m01s\r (no-eol) (esc)
loop [=====> ] 3/20 5m41s\r (no-eol) (esc) loop [=====> ] 3/20 5m41s\r (no-eol) (esc)
loop [=======> ] 4/20 5m21s\r (no-eol) (esc) loop [=======> ] 4/20 5m21s\r (no-eol) (esc)
loop [=========> ] 5/20 5m01s\r (no-eol) (esc) loop [=========> ] 5/20 5m01s\r (no-eol) (esc)
loop [===========> ] 6/20 4m41s\r (no-eol) (esc) loop [===========> ] 6/20 4m41s\r (no-eol) (esc)
loop [=============> ] 7/20 4m21s\r (no-eol) (esc) loop [=============> ] 7/20 4m21s\r (no-eol) (esc)
loop [===============> ] 8/20 4m01s\r (no-eol) (esc) loop [===============> ] 8/20 4m01s\r (no-eol) (esc)
loop [================> ] 9/20 13m27s\r (no-eol) (esc) loop [================> ] 9/20 25m40s\r (no-eol) (esc)
loop [==================> ] 10/20 19m21s\r (no-eol) (esc) loop [===================> ] 10/20 1h06m\r (no-eol) (esc)
loop [====================> ] 11/20 22m39s\r (no-eol) (esc) loop [=====================> ] 11/20 1h13m\r (no-eol) (esc)
loop [======================> ] 12/20 24m01s\r (no-eol) (esc) loop [=======================> ] 12/20 1h07m\r (no-eol) (esc)
loop [========================> ] 13/20 23m53s\r (no-eol) (esc) loop [========================> ] 13/20 58m19s\r (no-eol) (esc)
loop [==========================> ] 14/20 19m09s\r (no-eol) (esc) loop [===========================> ] 14/20 7m09s\r (no-eol) (esc)
loop [============================> ] 15/20 15m01s\r (no-eol) (esc) loop [=============================> ] 15/20 3m38s\r (no-eol) (esc)
loop [==============================> ] 16/20 11m21s\r (no-eol) (esc) loop [===============================> ] 16/20 2m15s\r (no-eol) (esc)
loop [=================================> ] 17/20 8m04s\r (no-eol) (esc) loop [=================================> ] 17/20 1m27s\r (no-eol) (esc)
loop [===================================> ] 18/20 5m07s\r (no-eol) (esc) loop [====================================> ] 18/20 52s\r (no-eol) (esc)
loop [=====================================> ] 19/20 2m27s\r (no-eol) (esc) loop [======================================> ] 19/20 25s\r (no-eol) (esc)
\r (no-eol) (esc) \r (no-eol) (esc)
Time estimates should not fail when there's no end point: Time estimates should not fail when there's no end point:
$ hg -y loop -- -4 $ hg -y loop -- -4
\r (no-eol) (esc) \r (no-eol) (esc)
loop [ <=> ] 2\r (no-eol) (esc) loop [ <=> ] 2\r (no-eol) (esc)
loop [ <=> ] 3\r (no-eol) (esc) loop [ <=> ] 3\r (no-eol) (esc)
\r (no-eol) (esc) \r (no-eol) (esc)