loggerhead

Merge lp:~jameinel/loggerhead/merge_point_config into lp:loggerhead

  1. merge_point_config
  2. Merge into trunk-rich
Proposed by John A Meinel
Status: Rejected
Rejected by: John A Meinel
Proposed branch: lp:~jameinel/loggerhead/merge_point_config
Merge into: lp:loggerhead
Diff against target: 150 lines (+39/-11)
4 files modified
loggerhead/apps/branch.py (+8/-1)
loggerhead/apps/transport.py (+8/-3)
loggerhead/config.py (+10/-1)
loggerhead/history.py (+13/-6)
To merge this branch: bzr merge lp:~jameinel/loggerhead/merge_point_config
Reviewer Review Type Date Requested Status
Matt Nordhoff Needs Information
Review via email: mp+24145@code.launchpad.net

Description of the change

This adds a configuration option
  --show-merge-points
  --no-show-merge-points
and
  http_show_merge_points = True/False

This changes the 'revision' view, and the view when you expand a revision in the 'changes' view.

This is moderately expensive to compute, and potentially *very* expensive to compute if we get rid of/change the cache infrastructure.

So this is a step along the way to turn expensive stuff into options you can turn off. I'm not sure how to document it, more than just the 'command-line' documentation.

To post a comment you must log in.
Revision history for this message
Matt Nordhoff (mnordhoff) wrote :

Chopping out features to improve performance makes me sad, but if you think it's the right call, I won't object. Besides, it defaults to leaving the features on.

Although it might be prudent to wait to land this until we're sure the caching changes will cause problems.

58 + show_merge_points = ui.bool_from_string(show_merge_points)

bool_from_string() was added in bzr 1.18, while Loggerhead is currently supposed to support...um, 1.13? I forget. Anyway, older than 1.18. In fact, another part of Loggerhead implements this function itself.

I'm not sure it's worth changing the backwards compatibility just for this, although it's kind of moot since we're probably going to kill everything before 2.0 by landing your other major changes.

78 + # XXX: Shouldn't this be registering an atexit hook to delete the
79 + # directory? Otherwise we fill up /tmp with caches that we won't
80 + # ever use again...

Personally, I always shut down Loggerhead in ways that don't trigger atexit hooks. :( (Ctrl+C or Py_FatalError because StaticTuple doesn't like OOMs.)

Revision history for this message
Matt Nordhoff (mnordhoff) wrote :

Why have both --show-merge-points and --no-show-merge-points?

Oh, plus you should add the new args to the man page (serve-branches.1). (It's probably out-of-date in other ways, but oh well.)

Revision history for this message
Matt Nordhoff (mnordhoff) :
review: Needs Information
Revision history for this message
John A Meinel (jameinel) wrote :

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Matt Nordhoff wrote:
> Chopping out features to improve performance makes me sad, but if you think it's the right call, I won't object. Besides, it defaults to leaving the features on.
>
> Although it might be prudent to wait to land this until we're sure the caching changes will cause problems.
>
> 58 + show_merge_points = ui.bool_from_string(show_merge_points)
>
> bool_from_string() was added in bzr 1.18, while Loggerhead is currently supposed to support...um, 1.13? I forget. Anyway, older than 1.18. In fact, another part of Loggerhead implements this function itself.
>
> I'm not sure it's worth changing the backwards compatibility just for this, although it's kind of moot since we're probably going to kill everything before 2.0 by landing your other major changes.

I believe I remember Ian making a comment that:
  The loggerhead X branch will start supporting only bzr 2.x.

I don't remember whether that was trunk, or a different one.

>
> 78 + # XXX: Shouldn't this be registering an atexit hook to delete the
> 79 + # directory? Otherwise we fill up /tmp with caches that we won't
> 80 + # ever use again...
>
> Personally, I always shut down Loggerhead in ways that don't trigger atexit hooks. :( (Ctrl+C or Py_FatalError because StaticTuple doesn't like OOMs.)

^C Still triggers at-exit hooks. OOM may not, not sure.
John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkvi/jwACgkQJdeBCYSNAAPtqgCgxMYVG9I61LUY/SVe54ZE0wBl
NxoAmwZo3PWMK1CH3SeBVtCsp8tNqWTx
=8yLO
-----END PGP SIGNATURE-----

Revision history for this message
Michael Hudson-Doyle (mwhudson) wrote :

I have to be honest, I'm not in favour of having a config option for this. If the information is expensive and not particularly useful, let's drop it. If it's not, let's find a way to present it usefully.

My opinion is more that loggerhead's entire way of navigating the whole revision graph could do with a rethink...

Revision history for this message
Robert Collins (lifeless) wrote :

FWIW supporting older than 2.0 isn't very sane IMO. Just saying.

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

On 7 May 2010 05:42, Michael Hudson <email address hidden> wrote:
> I have to be honest, I'm not in favour of having a config option for this.  If the information is expensive and not particularly useful, let's drop it.  If it's not, let's find a way to present it usefully.

I think having an option for it may be useful:

* we can test performance for and against, both now and in the future
* sites may want to make a different performance/features tradeoff
depending on their situation
* if Launchpad is for some reason heavily loaded, they could drop
ballast by turning off some features

Contra that more options allows more bugs through their interaction.

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

Unmerged revisions

412. By John A Meinel

Turn show_merge_points into a config setting.

Today it doesn't cost a whole lot, but with the caching changes, it will
start to cost a bit more.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'loggerhead/apps/branch.py'
2--- loggerhead/apps/branch.py 2010-04-14 17:54:32 +0000
3+++ loggerhead/apps/branch.py 2010-04-26 18:22:19 +0000
4@@ -49,6 +49,10 @@
5 def __init__(self, branch, friendly_name=None, config={},
6 graph_cache=None, branch_link=None, is_root=False,
7 served_url=_DEFAULT, use_cdn=False):
8+ # XXX: Why is config here a simple dictionary (which only ever has a
9+ # single item that I can find), while every other 'self._config'
10+ # is a LoggerheadConfig object. The latter seems a lot more
11+ # useful.
12 self.branch = branch
13 self._config = config
14 self.friendly_name = friendly_name
15@@ -64,6 +68,7 @@
16 def get_history(self):
17 file_cache = None
18 revinfo_disk_cache = None
19+ show_merge_points = self._config.get('show_merge_points', True)
20 cache_path = self._config.get('cachepath', None)
21 if cache_path is not None:
22 # Only import the cache if we're going to use it.
23@@ -79,7 +84,9 @@
24 revinfo_disk_cache = RevInfoDiskCache(cache_path)
25 return History(
26 self.branch, self.graph_cache, file_cache=file_cache,
27- revinfo_disk_cache=revinfo_disk_cache, cache_key=self.friendly_name)
28+ revinfo_disk_cache=revinfo_disk_cache,
29+ cache_key=self.friendly_name,
30+ show_merge_points=show_merge_points)
31
32 def url(self, *args, **kw):
33 if isinstance(args[0], list):
34
35=== modified file 'loggerhead/apps/transport.py'
36--- loggerhead/apps/transport.py 2010-03-25 16:19:24 +0000
37+++ loggerhead/apps/transport.py 2010-04-26 18:22:19 +0000
38@@ -1,4 +1,4 @@
39-# Copyright (C) 2008, 2009 Canonical Ltd.
40+# Copyright (C) 2008, 2009, 2010 Canonical Ltd.
41 #
42 # This program is free software; you can redistribute it and/or modify
43 # it under the terms of the GNU General Public License as published by
44@@ -18,7 +18,7 @@
45
46 import threading
47
48-from bzrlib import branch, errors, lru_cache, urlutils
49+from bzrlib import branch, errors, lru_cache, ui, urlutils
50 from bzrlib.config import LocationConfig
51 from bzrlib.smart import request
52 from bzrlib.transport import get_transport
53@@ -55,10 +55,15 @@
54 else:
55 name = self.name
56 is_root = False
57+ show_merge_points = self._config.get_option('show_merge_points')
58+ show_merge_points = ui.bool_from_string(show_merge_points)
59+ if show_merge_points is None:
60+ show_merge_points = True
61 branch_app = BranchWSGIApp(
62 branch,
63 name,
64- {'cachepath': self._config.SQL_DIR},
65+ {'cachepath': self._config.SQL_DIR,
66+ 'show_merge_points': show_merge_points},
67 self.root.graph_cache,
68 is_root=is_root,
69 use_cdn=self._config.get_option('use_cdn'),
70
71=== modified file 'loggerhead/config.py'
72--- loggerhead/config.py 2010-04-13 23:35:01 +0000
73+++ loggerhead/config.py 2010-04-26 18:22:19 +0000
74@@ -24,6 +24,9 @@
75 def _get_temporary_sql_dir():
76 global _temporary_sql_dir
77 if _temporary_sql_dir is None:
78+ # XXX: Shouldn't this be registering an atexit hook to delete the
79+ # directory? Otherwise we fill up /tmp with caches that we won't
80+ # ever use again...
81 _temporary_sql_dir = tempfile.mkdtemp(prefix='loggerhead-cache-')
82 return _temporary_sql_dir
83
84@@ -69,6 +72,12 @@
85 help="The directory to place the SQL cache in")
86 parser.add_option("--allow-writes", action="store_true",
87 help="Allow writing to the Bazaar server.")
88+ parser.add_option("--show-merge-points", action="store_true", default=True,
89+ help="When showing a revision, show where it"
90+ " was merged.")
91+ parser.add_option("--no-show-merge-points", action="store_false",
92+ dest='show_merge_points',
93+ help="Do not show where revisions are merged")
94 return parser
95
96
97@@ -92,7 +101,7 @@
98 All loggerhead-specific settings start with 'http_'
99 """
100 global_config = config.GlobalConfig().get_user_option('http_'+option)
101- cmd_config = getattr(self._options, option)
102+ cmd_config = getattr(self._options, option, None)
103 if global_config is not None and (
104 cmd_config is None or cmd_config is False):
105 return global_config
106
107=== modified file 'loggerhead/history.py'
108--- loggerhead/history.py 2010-04-14 17:54:32 +0000
109+++ loggerhead/history.py 2010-04-26 18:22:19 +0000
110@@ -274,7 +274,8 @@
111 self._revno_revid[revno_str] = revid
112
113 def __init__(self, branch, whole_history_data_cache, file_cache=None,
114- revinfo_disk_cache=None, cache_key=None):
115+ revinfo_disk_cache=None, cache_key=None,
116+ show_merge_points=True):
117 assert branch.is_locked(), (
118 "Can only construct a History object with a read-locked branch.")
119 if file_cache is not None:
120@@ -285,6 +286,7 @@
121 self._branch = branch
122 self._inventory_cache = {}
123 self._branch_nick = self._branch.get_config().get_nickname()
124+ self._show_merge_points = show_merge_points
125 self.log = logging.getLogger('loggerhead.%s' % (self._branch_nick,))
126
127 self.last_revid = branch.last_revision()
128@@ -614,12 +616,17 @@
129
130 # some data needs to be recalculated each time, because it may
131 # change as new revisions are added.
132- for change in changes:
133+ def merge_points_callback(a_change, attr):
134 merge_revids = self.simplify_merge_point_list(
135- self.get_merge_point_list(change.revid))
136- change.merge_points = [
137- util.Container(revid=r,
138- revno=self.get_revno(r)) for r in merge_revids]
139+ self.get_merge_point_list(a_change.revid))
140+ return [util.Container(revid=r, revno=self.get_revno(r))
141+ for r in merge_revids]
142+
143+ for change in changes:
144+ if self._show_merge_points:
145+ change._set_property('merge_points', merge_points_callback)
146+ else:
147+ change.merge_points = []
148 if len(change.parents) > 0:
149 change.parents = [util.Container(revid=r,
150 revno=self.get_revno(r)) for r in change.parents]

Subscribers

People subscribed via source and target branches