Patchwork [2,of,2] progress: split out elements of a pending commit to make diffs more readable

login
register
mail settings
Submitter Solomon Matthews
Date Jan. 17, 2015, 1:13 a.m.
Message ID <30518058824e89ee81ac.1421457204@dev562.prn1.facebook.com>
Download mbox | patch
Permalink /patch/7509/
State Deferred
Headers show

Comments

Solomon Matthews - Jan. 17, 2015, 1:13 a.m.
# HG changeset patch
# User Solomon Matthews <smat@fb.com>
# Date 1421455913 28800
#      Fri Jan 16 16:51:53 2015 -0800
# Node ID 30518058824e89ee81ac88870897b3a81877d738
# Parent  3e58f60b9c419e275eb14087c76e1ab04feeb3ed
progress: split out elements of a pending commit to make diffs more readable

No behavioral changes intended
Matt Mackall - Jan. 17, 2015, 9:24 p.m.
On Fri, 2015-01-16 at 17:13 -0800, Solomon Matthews wrote:
> # HG changeset patch
> # User Solomon Matthews <smat@fb.com>
> # Date 1421455913 28800
> #      Fri Jan 16 16:51:53 2015 -0800
> # Node ID 30518058824e89ee81ac88870897b3a81877d738
> # Parent  3e58f60b9c419e275eb14087c76e1ab04feeb3ed
> progress: split out elements of a pending commit to make diffs more readable
> 
> No behavioral changes intended

I've queued most of this, thanks. I went ahead and broke this second
patch apart to more clearly illustrate the granularity we're aiming for.
This will show up as three changes:

@  5502b smat tip  @
|  progress: add a lock to prepare for introducing a thread
o  63c7 smat
|  progress: move update check into helper method
o  e0ae smat
|  progress: move current topic to member variable
o  8b5b smat
|  progress: add try/finally to make the diffs for the next commit more
readable

I've left out the queue/event bits for now since we're still a ways from
having them used.
Augie Fackler - Jan. 26, 2015, 8 p.m.
On Fri, Jan 16, 2015 at 05:13:24PM -0800, Solomon Matthews wrote:
> # HG changeset patch
> # User Solomon Matthews <smat@fb.com>
> # Date 1421455913 28800
> #      Fri Jan 16 16:51:53 2015 -0800
> # Node ID 30518058824e89ee81ac88870897b3a81877d738
> # Parent  3e58f60b9c419e275eb14087c76e1ab04feeb3ed
> progress: split out elements of a pending commit to make diffs more readable
>
> No behavioral changes intended
>
> diff --git a/hgext/progress.py b/hgext/progress.py
> --- a/hgext/progress.py
> +++ b/hgext/progress.py
> @@ -37,6 +37,8 @@
>
>  import sys
>  import time
> +import threading
> +import Queue
>
>  from mercurial.i18n import _
>  testedwith = 'internal'
> @@ -90,6 +92,17 @@
>  class progbar(object):
>      def __init__(self, ui):
>          self.ui = ui
> +        self._refreshthread = None

I assume the goal for this unused variable is that progress printing
moves off the main thread so it can update things like ETA even when
we're in the middle of a complex operation?

> +        self._refreshlock = threading.Lock()
> +        # Queue for passing signals to the refresh thread
> +        self._refreshqueue = Queue.Queue()
> +        # Event to indicate that the refresh thread has finished its task(s)
> +        self._refreshdone = threading.Event()
> +        # Event to indicate that the refresh thread has drawn the progress bar
> +        # after timing out (as opposed to having received a signal). Required
> +        # for unit testing.
> +        self.refreshbackgroundevent = threading.Event()
> +
>          self.resetstate()
>
>      def resetstate(self):
> @@ -100,6 +113,7 @@
>          self.printed = False
>          self.lastprint = time.time() + float(self.ui.config(
>              'progress', 'delay', default=3))
> +        self.curtopic = None
>          self.lasttopic = None
>          self.indetcount = 0
>          self.refresh = float(self.ui.config(
> @@ -227,8 +241,20 @@
>              return _('%d %s/sec') % (delta / elapsed, unit)
>          return ''
>
> +    def _oktoprint(self, now):
> +        '''Check if conditions are met to print - e.g. changedelay elapsed'''
> +        if (self.lasttopic is None # first time we printed
> +            # not a topic change
> +            or self.curtopic == self.lasttopic
> +            # it's been long enough we should print anyway
> +            or now - self.lastprint >= self.changedelay):
> +            return True
> +        else:
> +            return False
> +
>      def progress(self, topic, pos, item='', unit='', total=None):
>          now = time.time()
> +        self._refreshlock.acquire()
>          try:
>              if pos is None:
>                  self.starttimes.pop(topic, None)
> @@ -255,16 +281,13 @@
>                      self.startvals[topic] = pos
>                      self.topics.append(topic)
>                  self.topicstates[topic] = pos, item, unit, total
> +                self.curtopic = topic
>                  if now - self.lastprint >= self.refresh and self.topics:
> -                    if (self.lasttopic is None # first time we printed
> -                        # not a topic change
> -                        or topic == self.lasttopic
> -                        # it's been long enough we should print anyway
> -                        or now - self.lastprint >= self.changedelay):
> +                    if self._oktoprint(now):
>                          self.lastprint = now
>                          self.show(now, topic, *self.topicstates[topic])
>          finally:
> -            pass
> +            self._refreshlock.release()
>
>  _singleton = None
>
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
Solomon Matthews - Jan. 27, 2015, 4:26 a.m.
On 1/26/15, 12:00 PM, "Augie Fackler" <raf@durin42.com> wrote:


>On Fri, Jan 16, 2015 at 05:13:24PM -0800, Solomon Matthews wrote:
>> # HG changeset patch
>> # User Solomon Matthews <smat@fb.com>
>> # Date 1421455913 28800
>> #      Fri Jan 16 16:51:53 2015 -0800
>> # Node ID 30518058824e89ee81ac88870897b3a81877d738
>> # Parent  3e58f60b9c419e275eb14087c76e1ab04feeb3ed
>> progress: split out elements of a pending commit to make diffs more
>>readable
>>
>> No behavioral changes intended
>>
>> diff --git a/hgext/progress.py b/hgext/progress.py
>> --- a/hgext/progress.py
>> +++ b/hgext/progress.py
>> @@ -37,6 +37,8 @@
>>
>>  import sys
>>  import time
>> +import threading
>> +import Queue
>>
>>  from mercurial.i18n import _
>>  testedwith = 'internal'
>> @@ -90,6 +92,17 @@
>>  class progbar(object):
>>      def __init__(self, ui):
>>          self.ui = ui
>> +        self._refreshthread = None
>
>I assume the goal for this unused variable is that progress printing
>moves off the main thread so it can update things like ETA even when
>we're in the middle of a complex operation?

Exactly. Including the variables here was the wrong call, I'll find some
way to break the rest of the change up more intelligibly after the freeze.

>
>> +        self._refreshlock = threading.Lock()
>> +        # Queue for passing signals to the refresh thread
>> +        self._refreshqueue = Queue.Queue()
>> +        # Event to indicate that the refresh thread has finished its
>>task(s)
>> +        self._refreshdone = threading.Event()
>> +        # Event to indicate that the refresh thread has drawn the
>>progress bar
>> +        # after timing out (as opposed to having received a signal).
>>Required
>> +        # for unit testing.
>> +        self.refreshbackgroundevent = threading.Event()
>> +
>>          self.resetstate()
>>
>>      def resetstate(self):
>> @@ -100,6 +113,7 @@
>>          self.printed = False
>>          self.lastprint = time.time() + float(self.ui.config(
>>              'progress', 'delay', default=3))
>> +        self.curtopic = None
>>          self.lasttopic = None
>>          self.indetcount = 0
>>          self.refresh = float(self.ui.config(
>> @@ -227,8 +241,20 @@
>>              return _('%d %s/sec') % (delta / elapsed, unit)
>>          return ''
>>
>> +    def _oktoprint(self, now):
>> +        '''Check if conditions are met to print - e.g. changedelay
>>elapsed'''
>> +        if (self.lasttopic is None # first time we printed
>> +            # not a topic change
>> +            or self.curtopic == self.lasttopic
>> +            # it's been long enough we should print anyway
>> +            or now - self.lastprint >= self.changedelay):
>> +            return True
>> +        else:
>> +            return False
>> +
>>      def progress(self, topic, pos, item='', unit='', total=None):
>>          now = time.time()
>> +        self._refreshlock.acquire()
>>          try:
>>              if pos is None:
>>                  self.starttimes.pop(topic, None)
>> @@ -255,16 +281,13 @@
>>                      self.startvals[topic] = pos
>>                      self.topics.append(topic)
>>                  self.topicstates[topic] = pos, item, unit, total
>> +                self.curtopic = topic
>>                  if now - self.lastprint >= self.refresh and
>>self.topics:
>> -                    if (self.lasttopic is None # first time we printed
>> -                        # not a topic change
>> -                        or topic == self.lasttopic
>> -                        # it's been long enough we should print anyway
>> -                        or now - self.lastprint >= self.changedelay):
>> +                    if self._oktoprint(now):
>>                          self.lastprint = now
>>                          self.show(now, topic, *self.topicstates[topic])
>>          finally:
>> -            pass
>> +            self._refreshlock.release()
>>
>>  _singleton = None
>>
>> _______________________________________________
>> Mercurial-devel mailing list
>> Mercurial-devel@selenic.com
>> http://selenic.com/mailman/listinfo/mercurial-devel

Patch

diff --git a/hgext/progress.py b/hgext/progress.py
--- a/hgext/progress.py
+++ b/hgext/progress.py
@@ -37,6 +37,8 @@ 
 
 import sys
 import time
+import threading
+import Queue
 
 from mercurial.i18n import _
 testedwith = 'internal'
@@ -90,6 +92,17 @@ 
 class progbar(object):
     def __init__(self, ui):
         self.ui = ui
+        self._refreshthread = None
+        self._refreshlock = threading.Lock()
+        # Queue for passing signals to the refresh thread
+        self._refreshqueue = Queue.Queue()
+        # Event to indicate that the refresh thread has finished its task(s)
+        self._refreshdone = threading.Event()
+        # Event to indicate that the refresh thread has drawn the progress bar
+        # after timing out (as opposed to having received a signal). Required
+        # for unit testing.
+        self.refreshbackgroundevent = threading.Event()
+
         self.resetstate()
 
     def resetstate(self):
@@ -100,6 +113,7 @@ 
         self.printed = False
         self.lastprint = time.time() + float(self.ui.config(
             'progress', 'delay', default=3))
+        self.curtopic = None
         self.lasttopic = None
         self.indetcount = 0
         self.refresh = float(self.ui.config(
@@ -227,8 +241,20 @@ 
             return _('%d %s/sec') % (delta / elapsed, unit)
         return ''
 
+    def _oktoprint(self, now):
+        '''Check if conditions are met to print - e.g. changedelay elapsed'''
+        if (self.lasttopic is None # first time we printed
+            # not a topic change
+            or self.curtopic == self.lasttopic
+            # it's been long enough we should print anyway
+            or now - self.lastprint >= self.changedelay):
+            return True
+        else:
+            return False
+
     def progress(self, topic, pos, item='', unit='', total=None):
         now = time.time()
+        self._refreshlock.acquire()
         try:
             if pos is None:
                 self.starttimes.pop(topic, None)
@@ -255,16 +281,13 @@ 
                     self.startvals[topic] = pos
                     self.topics.append(topic)
                 self.topicstates[topic] = pos, item, unit, total
+                self.curtopic = topic
                 if now - self.lastprint >= self.refresh and self.topics:
-                    if (self.lasttopic is None # first time we printed
-                        # not a topic change
-                        or topic == self.lasttopic
-                        # it's been long enough we should print anyway
-                        or now - self.lastprint >= self.changedelay):
+                    if self._oktoprint(now):
                         self.lastprint = now
                         self.show(now, topic, *self.topicstates[topic])
         finally:
-            pass
+            self._refreshlock.release()
 
 _singleton = None