[PATCH 1 of 5] platform: create a file closing utility
Augie Fackler
raf at durin42.com
Wed Sep 30 11:05:37 CDT 2015
> On Sep 30, 2015, at 12:01, Gregory Szorc <gregory.szorc at gmail.com> wrote:
>
> On Wed, Sep 30, 2015 at 8:46 AM, Augie Fackler <raf at durin42.com <mailto:raf at durin42.com>> wrote:
> > + """
> > + def __init__(self, ui):
> > + self._queue = []
>
> this should either be a threadsafe queue object, or you should use a
> collections.deque and a lock to preserve threadsafety (using a list as
> a queue can be a bummer performance-wise if it gets large).
>
> I thought the GIL removed considerations about thread safety here? My experience with the thread safe classes and locking in Python is that perf stinks :/ I can try it though.
Yes, I know threading.Queue stinks - but if you switch to a threading.Condition you can keep the list as long as you only mutate it while holding the lock (which you have to do to call notify() anyway).
The GIL does mean this won't break on cpython today, but people keep tilting at that windmill in cpython, and it's plausible that pypy or jyhton won't enforce that invariant forever - it's not in the language spec that list is threadsafe, so I only want to tiptoe into that area if it's /really worth it/.
>
>
> > + self._error = None
> > + self._alive = True
> > + self._ev = threading.Event()
> > + self._t = threading.Thread(target=self._processcloses,
> > + name='filecloser')
> > + self._t.start()
> > +
> > + # Windows defaults to max of 512 open file descriptors. A buffer of
> > + # 128 should be more than sufficient.
> > + # experimental config: worker.closefileslimit
> > + self._queuelimit = ui.configint('worker', 'closefilelimit', 384)
> > +
> > + def close(self, fh):
> > + self._queue.append(fh)
> > + self._ev.set()
> > +
> > + def verifystate(self):
> > + # Raise an error encountered on background thread.
> > + if self._error:
> > + raise self._error
> > +
> > + # Don't allow the queue to grow too long.
> > + while len(self._queue) >= self._queuelimit:
> > + # This will surrender the GIL. 100ms is a bit long. However,
> > + # if we have lots of queued operations, they likely won't finish
> > + # in 100ms since it takes ~1ms to close a file.
> > + time.sleep(0.100)
> > +
> > + def cleanup(self):
> > + self._alive = False
> > + # This arguably isn't necessary. But it should shave a few ms.
> > + self._ev.set()
> > + self._t.join()
> > +
> > + def _processcloses(self):
> > + """Main function of closing thread."""
> > + while self._alive or self._queue:
> > + if not self._queue:
> > + self._ev.wait(0.100)
>
> I think instead of busy-waiting, you could do better with a
> threading.Condition - that would also give you the lock you'd need for
> real thread-safety.
>
> OK. Something else to keep in mind is give the results from the stream clone patch, I may look into using multiple threads.
threading.Condition will work with multiple threads - I've used it that way many times.
>
>
> > + self._ev.clear()
> > +
> > + while self._queue:
> > + fh = self._queue.pop(0)
> > + try:
> > + fh.close()
> > + except Exception as e:
> > + self._error = e
> > + # Don't raise otherwise we'll leak handles.
> > _______________________________________________
> > Mercurial-devel mailing list
> > Mercurial-devel at selenic.com <mailto:Mercurial-devel at selenic.com>
> > https://selenic.com/mailman/listinfo/mercurial-devel <https://selenic.com/mailman/listinfo/mercurial-devel>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://selenic.com/pipermail/mercurial-devel/attachments/20150930/81b5b95e/attachment.html>
More information about the Mercurial-devel
mailing list