| Submitter | Bryan O'Sullivan |
|---|---|
| Date | Jan. 11, 2016, 11:34 p.m. |
| Message ID | <e909e11e2d42cf419f22.1452555272@bryano-mbp.local> |
| Download | mbox | patch |
| Permalink | /patch/12665/ |
| State | Not Applicable |
| Headers | show |
Comments
On Mon, Jan 11, 2016 at 3:34 PM, Bryan O'Sullivan <bos@serpentine.com> wrote: > + with util.ctxmanager(repo.lock, repo.translambda('phase')) as c: > I forgot the patch that introduces translambda; it simply defers repo.transaction with a lambda because that pattern is used so heavily. This patch series is several dozen long, all very simple transformations of this sort. I think the introductory patch should be enough to get conversation started :-)
On Mon, Jan 11, 2016 at 3:40 PM, Bryan O'Sullivan <bos@serpentine.com> wrote: > > On Mon, Jan 11, 2016 at 3:34 PM, Bryan O'Sullivan <bos@serpentine.com> > wrote: > >> + with util.ctxmanager(repo.lock, repo.translambda('phase')) as c: >> > > I forgot the patch that introduces translambda; it simply defers > repo.transaction with a lambda because that pattern is used so heavily. > > This patch series is several dozen long, all very simple transformations > of this sort. I think the introductory patch should be enough to get > conversation started :-) > +1 to moving locks and transactions to context managers. As much as we use the pattern of obtaining a lock *and* a transaction, I think it might be worthwhile to introduce a utility context manager. e.g. with repo.lockandtransaction('phase') as c: lock, tr = c
A couple of issues found in "use context manager" series, in clowncopter. http://hg.netv6.net/clowncopter/rev/5f2bd1d7b292 > --- a/mercurial/context.py Fri Jan 15 13:14:46 2016 -0800 > +++ b/mercurial/context.py Fri Jan 15 13:14:46 2016 -0800 > @@ -1548,17 +1548,15 @@ > # so we don't wait on the lock > # wlock can invalidate the dirstate, so cache normal _after_ > # taking the lock > - wlock = self._repo.wlock(False) > normal = self._repo.dirstate.normal > - try: > + with self._repo.wlock(False): wlock must be taken before caching normal. See https://selenic.com/repo/hg/rev/48e32c2c499b http://hg.netv6.net/clowncopter/rev/adf95f8332e6 > --- a/mercurial/context.py Fri Jan 15 13:14:46 2016 -0800 > +++ b/mercurial/context.py Fri Jan 15 13:14:46 2016 -0800 > @@ -1415,9 +1415,8 @@ > > def add(self, list, prefix=""): > join = lambda f: os.path.join(prefix, f) > - wlock = self._repo.wlock() > ui, ds = self._repo.ui, self._repo.dirstate > - try: > + with self._repo.wlock(): Perhaps dirstate should be cached after taking wlock. http://hg.netv6.net/clowncopter/rev/5eaee365f782 > @@ -258,14 +257,12 @@ > > class histeditstate(object): > def __init__(self, repo, parentctxnode=None, actions=None, keep=None, > - topmost=None, replacements=None, lock=None, wlock=None): > + topmost=None, replacements=None): > self.repo = repo > self.actions = actions > self.keep = keep > self.topmost = topmost > self.parentctxnode = parentctxnode > - self.lock = lock > - self.wlock = wlock These variables are planned to be used? https://selenic.com/repo/hg/rev/e0b5f5e3afe8 Regards,
On 01/16/2016 04:15 AM, Yuya Nishihara wrote: > A couple of issues found in "use context manager" series, in clowncopter. > > http://hg.netv6.net/clowncopter/rev/5f2bd1d7b292 > >> --- a/mercurial/context.py Fri Jan 15 13:14:46 2016 -0800 >> +++ b/mercurial/context.py Fri Jan 15 13:14:46 2016 -0800 >> @@ -1548,17 +1548,15 @@ >> # so we don't wait on the lock >> # wlock can invalidate the dirstate, so cache normal _after_ >> # taking the lock >> - wlock = self._repo.wlock(False) >> normal = self._repo.dirstate.normal >> - try: >> + with self._repo.wlock(False): > > wlock must be taken before caching normal. See > https://selenic.com/repo/hg/rev/48e32c2c499b Nice catch, I should have spotted that. I'm not sure what the status of that series in Matt hands. We sould send a fix once that status is clearer. > http://hg.netv6.net/clowncopter/rev/adf95f8332e6 > >> --- a/mercurial/context.py Fri Jan 15 13:14:46 2016 -0800 >> +++ b/mercurial/context.py Fri Jan 15 13:14:46 2016 -0800 >> @@ -1415,9 +1415,8 @@ >> >> def add(self, list, prefix=""): >> join = lambda f: os.path.join(prefix, f) >> - wlock = self._repo.wlock() >> ui, ds = self._repo.ui, self._repo.dirstate >> - try: >> + with self._repo.wlock(): > > Perhaps dirstate should be cached after taking wlock. ditto. > > > http://hg.netv6.net/clowncopter/rev/5eaee365f782 > >> @@ -258,14 +257,12 @@ >> >> class histeditstate(object): >> def __init__(self, repo, parentctxnode=None, actions=None, keep=None, >> - topmost=None, replacements=None, lock=None, wlock=None): >> + topmost=None, replacements=None): >> self.repo = repo >> self.actions = actions >> self.keep = keep >> self.topmost = topmost >> self.parentctxnode = parentctxnode >> - self.lock = lock >> - self.wlock = wlock > > These variables are planned to be used? > > https://selenic.com/repo/hg/rev/e0b5f5e3afe8 Without a comment explaining why we have these unused variable, there is few chance that they survive. We should reinstall them with a such comment. Cheers,
On Sat, 2016-01-16 at 20:25 -0800, Pierre-Yves David wrote: > > On 01/16/2016 04:15 AM, Yuya Nishihara wrote: > > A couple of issues found in "use context manager" series, in clowncopter. > > > > http://hg.netv6.net/clowncopter/rev/5f2bd1d7b292 > > > > > --- a/mercurial/context.py Fri Jan 15 13:14:46 2016 -0800 > > > +++ b/mercurial/context.py Fri Jan 15 13:14:46 2016 -0800 > > > @@ -1548,17 +1548,15 @@ > > > # so we don't wait on the lock > > > # wlock can invalidate the dirstate, so cache normal > > > _after_ > > > # taking the lock > > > - wlock = self._repo.wlock(False) > > > normal = self._repo.dirstate.normal > > > - try: > > > + with self._repo.wlock(False): > > > > wlock must be taken before caching normal. See > > https://selenic.com/repo/hg/rev/48e32c2c499b > > Nice catch, I should have spotted that. I'm not sure what the status of > that series in Matt hands. We sould send a fix once that status is clearer. I've got a bunch of other nits in my review notes for this series. There's also some stuff of the form: - finally: - tr.release() - repo.ui.flush() + repo.ui.flush() ..that reverts a bugfix. We don't flush ui buffers much, so doing it in a finally clause is a good sign that there's an issue being dealt with. I'm going to try to sort through it today. > > > http://hg.netv6.net/clowncopter/rev/adf95f8332e6 > > > > > --- a/mercurial/context.py Fri Jan 15 13:14:46 2016 -0800 > > > +++ b/mercurial/context.py Fri Jan 15 13:14:46 2016 -0800 > > > @@ -1415,9 +1415,8 @@ > > > > > > def add(self, list, prefix=""): > > > join = lambda f: os.path.join(prefix, f) > > > - wlock = self._repo.wlock() > > > ui, ds = self._repo.ui, self._repo.dirstate > > > - try: > > > + with self._repo.wlock(): > > > > Perhaps dirstate should be cached after taking wlock. > > ditto. > > > > > > > > http://hg.netv6.net/clowncopter/rev/5eaee365f782 > > > > > @@ -258,14 +257,12 @@ > > > > > > class histeditstate(object): > > > def __init__(self, repo, parentctxnode=None, actions=None, > > > keep=None, > > > - topmost=None, replacements=None, lock=None, wlock=None): > > > + topmost=None, replacements=None): > > > self.repo = repo > > > self.actions = actions > > > self.keep = keep > > > self.topmost = topmost > > > self.parentctxnode = parentctxnode > > > - self.lock = lock > > > - self.wlock = wlock > > > > These variables are planned to be used? > > > > https://selenic.com/repo/hg/rev/e0b5f5e3afe8 > > Without a comment explaining why we have these unused variable, there is > few chance that they survive. We should reinstall them with a such comment. > > Cheers, >
On Sun, Jan 17, 2016 at 5:39 AM, Matt Mackall <mpm@selenic.com> wrote: > I'm going to try to sort through it today. > Got more details?
On Sun, 2016-01-17 at 07:39 -0600, Matt Mackall wrote: > On Sat, 2016-01-16 at 20:25 -0800, Pierre-Yves David wrote: > > > > On 01/16/2016 04:15 AM, Yuya Nishihara wrote: > > > A couple of issues found in "use context manager" series, in clowncopter. > > > > > > http://hg.netv6.net/clowncopter/rev/5f2bd1d7b292 > > > > > > > --- a/mercurial/context.py Fri Jan 15 13:14:46 2016 -0800 > > > > +++ b/mercurial/context.py Fri Jan 15 13:14:46 2016 -0800 > > > > @@ -1548,17 +1548,15 @@ > > > > # so we don't wait on the lock > > > > # wlock can invalidate the dirstate, so cache normal > > > > _after_ > > > > # taking the lock > > > > - wlock = self._repo.wlock(False) > > > > normal = self._repo.dirstate.normal > > > > - try: > > > > + with self._repo.wlock(False): > > > > > > wlock must be taken before caching normal. See > > > https://selenic.com/repo/hg/rev/48e32c2c499b > > > > Nice catch, I should have spotted that. I'm not sure what the status of > > that series in Matt hands. We sould send a fix once that status is clearer. > > I've got a bunch of other nits in my review notes for this series. There's > also > some stuff of the form: > > - finally: > - tr.release() > - repo.ui.flush() > + repo.ui.flush() > > ..that reverts a bugfix. We don't flush ui buffers much, so doing it in a > finally clause is a good sign that there's an issue being dealt with. > > I'm going to try to sort through it today. For the record, I ended up taking about 80% of this series. I did the following: - reordered the whole series - cleanups first - context manager definitions - single locks - just transactions - lock+wlock - lock+tranaction - changed the summary keywords of everything to "with:" (the keywords mostly exist to sort our end-user release notes) - audited carefully for entry and exit semantic changes like the above I ended up not taking the multi-element changes because I have several concerns and we were up against the freeze. It seems to me that in general, trying to collapse multiple resource acquisitions into one "with:" is error-prone and doesn't always work because we often want a finally: or except: that's orthogonal to resource acquisition. We even have stuff we want to do between acquisition of resources and may not always want to acquire things early. So it seems that the right thing is to just consistently use one with: for each resource and accept the extra level(s) of indentation as the cost of clarity and consistency (and curse Python for having useless destructors). We're also going to in the not to distant future figure out how to break up our locks a bit more, so the patterns for lockall and lockedtxn are a bit doomed anyway. Lastly, it's rather unfortunate that Python's "context manager" name collides with our own very different and heavily used notion of context. We should try and minimize that collision, starting with not naming anything with "ctx" which is the abbreviation our context classes and variables all use (bearing in mind that naming consistency is one of our main forms of type safety lolfml). -- Mathematics is the supreme nostalgia of our time.
Patch
diff --git a/mercurial/commands.py b/mercurial/commands.py --- a/mercurial/commands.py +++ b/mercurial/commands.py @@ -5438,7 +5438,6 @@ def phase(ui, repo, *revs, **opts): revs = scmutil.revrange(repo, revs) - lock = None ret = 0 if targetphase is None: # display @@ -5446,10 +5445,8 @@ def phase(ui, repo, *revs, **opts): ctx = repo[r] ui.write('%i: %s\n' % (ctx.rev(), ctx.phasestr())) else: - tr = None - lock = repo.lock() - try: - tr = repo.transaction("phase") + with util.ctxmanager(repo.lock, repo.translambda('phase')) as c: + tr = c()[1] # set phase if not revs: raise error.Abort(_('empty revision set')) @@ -5463,10 +5460,6 @@ def phase(ui, repo, *revs, **opts): if opts['force']: phases.retractboundary(repo, tr, targetphase, nodes) tr.close() - finally: - if tr is not None: - tr.release() - lock.release() getphase = unfi._phasecache.phase newdata = [getphase(unfi, r) for r in unfi] changes = sum(newdata[r] != olddata[r] for r in unfi)
# HG changeset patch # User Bryan O'Sullivan <bos@serpentine.com> # Date 1452555256 28800 # Mon Jan 11 15:34:16 2016 -0800 # Node ID e909e11e2d42cf419f2205e632f23a206a66d2f1 # Parent 03e3c89b14b24b99e63ec985e71d9ddd0ef7fcf1 {RFC} commands: switch to ctxmanager in phase This is a small example of the kind of simplification that the ctxmanager class in the previous patch makes possible.