Patchwork [2,of,3,v2] localrepo: don't reference transaction from hook closure (issue5043)

login
register
mail settings
Submitter Gregory Szorc
Date Jan. 17, 2016, 10:18 p.m.
Message ID <91969eee20a1dcd52470.1453069121@ubuntu-vm-main>
Download mbox | patch
Permalink /patch/12821/
State Accepted
Commit e219dbfd0342492eb5695f1a11d1156063aabdb5
Headers show

Comments

Gregory Szorc - Jan. 17, 2016, 10:18 p.m.
# HG changeset patch
# User Gregory Szorc <gregory.szorc@gmail.com>
# Date 1453068855 28800
#      Sun Jan 17 14:14:15 2016 -0800
# Node ID 91969eee20a1dcd52470258164bc44528861a4c6
# Parent  471f676f63e429745e5f691201faa91cfd897e83
localrepo: don't reference transaction from hook closure (issue5043)

Before, the hook() closure (which is called as part of locking hooks)
would maintain a reference to a transaction instance (which should be
finalized by the time lock hooks are called). Because we accumulate
hook() instances when there are multiple transactions per lock, this
would result in holding references to the transaction instances which
would lead to higher memory utilization.

Creating a reference to the hook arguments dict minimizes the number
of objects that are kept alive until the lock release hook runs,
minimizing memory "leaks."
Matt Mackall - Jan. 18, 2016, 3:14 a.m.
On Sun, 2016-01-17 at 14:18 -0800, Gregory Szorc wrote:
> # HG changeset patch
> # User Gregory Szorc <gregory.szorc@gmail.com>
> # Date 1453068855 28800
> #      Sun Jan 17 14:14:15 2016 -0800
> # Node ID 91969eee20a1dcd52470258164bc44528861a4c6
> # Parent  471f676f63e429745e5f691201faa91cfd897e83
> localrepo: don't reference transaction from hook closure (issue5043)

These first two are queued for default, thanks.

-- 
Mathematics is the supreme nostalgia of our time.

Patch

diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
--- a/mercurial/localrepo.py
+++ b/mercurial/localrepo.py
@@ -1080,19 +1080,25 @@  class localrepository(object):
         tr.hookargs['txnid'] = txnid
         # note: writing the fncache only during finalize mean that the file is
         # outdated when running hooks. As fncache is used for streaming clone,
         # this is not expected to break anything that happen during the hooks.
         tr.addfinalize('flush-fncache', self.store.write)
         def txnclosehook(tr2):
             """To be run if transaction is successful, will schedule a hook run
             """
+            # Don't reference tr2 in hook() so we don't hold a reference.
+            # This reduces memory consumption when there are multiple
+            # transactions per lock. This can likely go away if issue5045
+            # fixes the function accumulation.
+            hookargs = tr2.hookargs
+
             def hook():
                 reporef().hook('txnclose', throw=False, txnname=desc,
-                               **tr2.hookargs)
+                               **hookargs)
             reporef()._afterlock(hook)
         tr.addfinalize('txnclose-hook', txnclosehook)
         def txnaborthook(tr2):
             """To be run if transaction is aborted
             """
             reporef().hook('txnabort', throw=False, txnname=desc,
                            **tr2.hookargs)
         tr.addabort('txnabort-hook', txnaborthook)