Bazaar

Merge lp:~spiv/bzr/debug-flag-relock into lp:bzr

  1. debug-flag-relock
  2. Merge into bzr.dev
Proposed by Andrew Bennetts
Status: Superseded
Proposed branch: lp:~spiv/bzr/debug-flag-relock
Merge into: lp:bzr
Diff against target: 230 lines
7 files modified
NEWS (+4/-0)
bzrlib/branch.py (+6/-1)
bzrlib/help_topics/en/debug-flags.txt (+4/-2)
bzrlib/lock.py (+21/-0)
bzrlib/remote.py (+6/-2)
bzrlib/repofmt/pack_repo.py (+7/-0)
bzrlib/repository.py (+4/-1)
To merge this branch: bzr merge lp:~spiv/bzr/debug-flag-relock
Reviewer Review Type Date Requested Status
Vincent Ladeuil Needs Fixing
Review via email: mp+12968@code.launchpad.net

This proposal has been superseded by a proposal from 2009-10-07.

To post a comment you must log in.
Revision history for this message
Andrew Bennetts (spiv) wrote :

This is an idea I had while looking at bug 389413. It adds a -Drelock debug flag to make noise about cases where we probably should take more care to just hold one lock rather than locking/unlocking/locking-again.

Here's some sample output:

$ ./bzr diff -c 4148 . -Drelock > /dev/null
BzrBranch7('file:///home/andrew/warthogs/bzr/debug-flag-relock/') was read locked again
CHKInventoryRepository('file:///home/andrew/warthogs/bzr/.bzr/repository/') was read locked again
CHKInventoryRepository('file:///home/andrew/warthogs/bzr/.bzr/repository/') was read locked again
CHKInventoryRepository('file:///home/andrew/warthogs/bzr/.bzr/repository/') was read locked again
BzrBranch7('file:///home/andrew/warthogs/bzr/debug-flag-relock/') was read locked again
CHKInventoryRepository('file:///home/andrew/warthogs/bzr/.bzr/repository/') was read locked again
BzrBranch7('file:///home/andrew/warthogs/bzr/debug-flag-relock/') was read locked again
CHKInventoryRepository('file:///home/andrew/warthogs/bzr/.bzr/repository/') was read locked again
CHKInventoryRepository('file:///home/andrew/warthogs/bzr/.bzr/repository/') was read locked again
CHKInventoryRepository('file:///home/andrew/warthogs/bzr/.bzr/repository/') was read locked again
BzrBranch7('file:///home/andrew/warthogs/bzr/debug-flag-relock/') was read locked again

Hopefully that gives some idea of how much we can improve :)

Possibly we should have a more generic hook point for lock/unlock operations so that tests can assert "object X is only locked once", similar to the existing HPSS effort tests... but this seems cheap and useful. (The existing hook points for lockdir lock_acquired/lock_released would only catch lock_write, not lock_read, so they aren't sufficient for this case.)

Revision history for this message
Martin Pool (mbp) wrote :

That sounds good.

I think note() may not be the best interface to call because the ui
changes turn it into something more like "display a message to the
user" and you wouldn't want popup dialogs for each hit. But maybe
that's the best one for now and I can change it later.

So I think it's actually an idea for me to add a kind of 'loud mutter'
rather than a change for you.

The duplication is ugly though - I think you should put this into a
(possibly mixed in) base class.

--
Martin <http://launchpad.net/~mbp/>

Revision history for this message
Vincent Ladeuil (vila) wrote :

More a tweak really.

You lost your 'manner' for --strict-locks in reordering the help messages.

review: Needs Fixing

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'NEWS'
2--- NEWS 2009-10-06 20:45:48 +0000
3+++ NEWS 2009-10-07 08:19:11 +0000
4@@ -191,6 +191,10 @@
5 Internals
6 *********
7
8+* Added ``-Drelock`` debug flag. It will ``note`` a message every time a
9+ repository or branch object is unlocked then relocked the same way.
10+ (Andrew Bennetts)
11+
12 * ``BTreeLeafParser.extract_key`` has been tweaked slightly to reduce
13 mallocs while parsing the index (approx 3=>1 mallocs per key read).
14 This results in a 10% speedup while reading an index.
15
16=== modified file 'bzrlib/branch.py'
17--- bzrlib/branch.py 2009-08-19 18:04:49 +0000
18+++ bzrlib/branch.py 2009-10-07 08:19:11 +0000
19@@ -49,6 +49,7 @@
20 from bzrlib.decorators import needs_read_lock, needs_write_lock
21 from bzrlib.hooks import HookPoint, Hooks
22 from bzrlib.inter import InterObject
23+from bzrlib.lock import _RelockDebugMixin
24 from bzrlib import registry
25 from bzrlib.symbol_versioning import (
26 deprecated_in,
27@@ -2079,7 +2080,7 @@
28 _legacy_formats[0].network_name(), _legacy_formats[0].__class__)
29
30
31-class BzrBranch(Branch):
32+class BzrBranch(Branch, _RelockDebugMixin):
33 """A branch stored in the actual filesystem.
34
35 Note that it's "local" in the context of the filesystem; it doesn't
36@@ -2131,6 +2132,8 @@
37 return self.control_files.is_locked()
38
39 def lock_write(self, token=None):
40+ if not self.is_locked():
41+ self._note_lock('w')
42 # All-in-one needs to always unlock/lock.
43 repo_control = getattr(self.repository, 'control_files', None)
44 if self.control_files == repo_control or not self.is_locked():
45@@ -2146,6 +2149,8 @@
46 raise
47
48 def lock_read(self):
49+ if not self.is_locked():
50+ self._note_lock('r')
51 # All-in-one needs to always unlock/lock.
52 repo_control = getattr(self.repository, 'control_files', None)
53 if self.control_files == repo_control or not self.is_locked():
54
55=== modified file 'bzrlib/help_topics/en/debug-flags.txt'
56--- bzrlib/help_topics/en/debug-flags.txt 2009-08-20 05:02:45 +0000
57+++ bzrlib/help_topics/en/debug-flags.txt 2009-10-07 08:19:11 +0000
58@@ -22,16 +22,18 @@
59 -Dhttp Trace http connections, requests and responses.
60 -Dindex Trace major index operations.
61 -Dknit Trace knit operations.
62--Dstrict_locks Trace when OS locks are potentially used in a non-portable
63 manner.
64 -Dlock Trace when lockdir locks are taken or released.
65 -Dprogress Trace progress bar operations.
66 -Dmerge Emit information for debugging merges.
67 -Dno_apport Don't use apport to report crashes.
68--Dunlock Some errors during unlock are treated as warnings.
69 -Dpack Emit information about pack operations.
70+-Drelock Emit a message every time a branch or repository object is
71+ unlocked then relocked the same way.
72 -Dsftp Trace SFTP internals.
73 -Dstream Trace fetch streams.
74+-Dstrict_locks Trace when OS locks are potentially used in a non-portable
75+-Dunlock Some errors during unlock are treated as warnings.
76 -DIDS_never Never use InterDifferingSerializer when fetching.
77 -DIDS_always Always use InterDifferingSerializer to fetch if appropriate
78 for the format, even for non-local fetches.
79
80=== modified file 'bzrlib/lock.py'
81--- bzrlib/lock.py 2009-07-31 16:51:48 +0000
82+++ bzrlib/lock.py 2009-10-07 08:19:10 +0000
83@@ -518,3 +518,24 @@
84 # We default to using the first available lock class.
85 _lock_type, WriteLock, ReadLock = _lock_classes[0]
86
87+
88+class _RelockDebugMixin(object):
89+ """Mixin support for -Drelock flag.
90+
91+ Add this as a base class then call self._note_lock with 'r' or 'w' when
92+ acquiring a read- or write-lock. If this object was previously locked (and
93+ locked the same way), and -Drelock is set, then this will trace.note a
94+ message about it.
95+ """
96+
97+ _prev_lock = None
98+
99+ def _note_lock(self, lock_type):
100+ if 'relock' in debug.debug_flags and self._prev_lock == lock_type:
101+ if lock_type == 'r':
102+ type_name = 'read'
103+ else:
104+ type_name = 'write'
105+ trace.note('%r was %s locked again', self, type_name)
106+ self._prev_lock = lock_type
107+
108
109=== modified file 'bzrlib/remote.py'
110--- bzrlib/remote.py 2009-10-02 05:43:41 +0000
111+++ bzrlib/remote.py 2009-10-07 08:19:11 +0000
112@@ -619,7 +619,7 @@
113 return self._custom_format._serializer
114
115
116-class RemoteRepository(_RpcHelper):
117+class RemoteRepository(_RpcHelper, lock._RelockDebugMixin):
118 """Repository accessed over rpc.
119
120 For the moment most operations are performed using local transport-backed
121@@ -949,6 +949,7 @@
122 def lock_read(self):
123 # wrong eventually - want a local lock cache context
124 if not self._lock_mode:
125+ self._note_lock('r')
126 self._lock_mode = 'r'
127 self._lock_count = 1
128 self._unstacked_provider.enable_cache(cache_misses=True)
129@@ -974,6 +975,7 @@
130
131 def lock_write(self, token=None, _skip_rpc=False):
132 if not self._lock_mode:
133+ self._note_lock('w')
134 if _skip_rpc:
135 if self._lock_token is not None:
136 if token != self._lock_token:
137@@ -2081,7 +2083,7 @@
138 return self._custom_format.supports_set_append_revisions_only()
139
140
141-class RemoteBranch(branch.Branch, _RpcHelper):
142+class RemoteBranch(branch.Branch, _RpcHelper, lock._RelockDebugMixin):
143 """Branch stored on a server accessed by HPSS RPC.
144
145 At the moment most operations are mapped down to simple file operations.
146@@ -2318,6 +2320,7 @@
147 def lock_read(self):
148 self.repository.lock_read()
149 if not self._lock_mode:
150+ self._note_lock('r')
151 self._lock_mode = 'r'
152 self._lock_count = 1
153 if self._real_branch is not None:
154@@ -2343,6 +2346,7 @@
155
156 def lock_write(self, token=None):
157 if not self._lock_mode:
158+ self._note_lock('w')
159 # Lock the branch and repo in one remote call.
160 remote_tokens = self._remote_lock_write(token)
161 self._lock_token, self._repo_lock_token = remote_tokens
162
163=== modified file 'bzrlib/repofmt/pack_repo.py'
164--- bzrlib/repofmt/pack_repo.py 2009-09-08 05:51:36 +0000
165+++ bzrlib/repofmt/pack_repo.py 2009-10-07 08:19:10 +0000
166@@ -73,6 +73,7 @@
167 )
168 from bzrlib.trace import (
169 mutter,
170+ note,
171 warning,
172 )
173
174@@ -2300,6 +2301,9 @@
175 if self._write_lock_count == 1:
176 self._transaction = transactions.WriteTransaction()
177 if not locked:
178+ if 'relock' in debug.debug_flags and self._prev_lock == 'w':
179+ note('%r was write locked again', self)
180+ self._prev_lock = 'w'
181 for repo in self._fallback_repositories:
182 # Writes don't affect fallback repos
183 repo.lock_read()
184@@ -2312,6 +2316,9 @@
185 else:
186 self.control_files.lock_read()
187 if not locked:
188+ if 'relock' in debug.debug_flags and self._prev_lock == 'r':
189+ note('%r was read locked again', self)
190+ self._prev_lock = 'r'
191 for repo in self._fallback_repositories:
192 repo.lock_read()
193 self._refresh_data()
194
195=== modified file 'bzrlib/repository.py'
196--- bzrlib/repository.py 2009-09-24 04:54:19 +0000
197+++ bzrlib/repository.py 2009-10-07 08:19:10 +0000
198@@ -50,6 +50,7 @@
199 """)
200
201 from bzrlib.decorators import needs_read_lock, needs_write_lock
202+from bzrlib.lock import _RelockDebugMixin
203 from bzrlib.inter import InterObject
204 from bzrlib.inventory import (
205 Inventory,
206@@ -856,7 +857,7 @@
207 # Repositories
208
209
210-class Repository(object):
211+class Repository(_RelockDebugMixin):
212 """Repository holding history for one or more branches.
213
214 The repository holds and retrieves historical information including
215@@ -1381,6 +1382,7 @@
216 locked = self.is_locked()
217 result = self.control_files.lock_write(token=token)
218 if not locked:
219+ self._debug_lock('w')
220 for repo in self._fallback_repositories:
221 # Writes don't affect fallback repos
222 repo.lock_read()
223@@ -1391,6 +1393,7 @@
224 locked = self.is_locked()
225 self.control_files.lock_read()
226 if not locked:
227+ self._debug_lock('r')
228 for repo in self._fallback_repositories:
229 repo.lock_read()
230 self._refresh_data()