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
266–267

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.

265

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.

265

Right. configfloat will solve that.

266–267

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

PathPackages
Mmercurial/configitems.py (3 lines)
Mmercurial/progress.py (27 lines)
Mtests/test-progress.t (22 lines)

Diff 2091

mercurial/configitems.py

default=3, default=3,
) )
coreconfigitem('progress', 'disable', coreconfigitem('progress', 'disable',
default=False, default=False,
) )
coreconfigitem('progress', 'estimate', coreconfigitem('progress', 'estimate',
default=2, default=2,
) )
coreconfigitem('progress', 'estimateinterval',
default=60,
)
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/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'])
# experimental config: progress.estimateinterval, in seconds
self.estimateinterval = self.ui.configint(
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.
'progress', 'estimateinterval')
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 so ETA and speed works better
If progress is non-linear (ex. get much slower in the last minute),
it's more friendly to reflect the slow-down to the ETA or speed.
[======================================> ]
^^^^^^^
last minute (estimateinterval) progress
only use this for estimation
'''
interval = self.estimateinterval
if not interval:
return
elapsed = now - self.starttimes[topic]
if elapsed > interval:
delta = pos - self.startvals[topic]
newdelta = 1.0 * 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 newdelta < 0.1: # prevent too steep changes
return
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.
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)