loggerhead

Merge lp:~danilo/loggerhead/bug-718566 into lp:loggerhead

  1. bug-718566
  2. Merge into trunk-rich
Proposed by Данило Шеган
Status: Merged
Merged at revision: 455
Proposed branch: lp:~danilo/loggerhead/bug-718566
Merge into: lp:loggerhead
Diff against target: 109 lines (+4/-38)
3 files modified
loggerhead/apps/branch.py (+2/-5)
loggerhead/changecache.py (+0/-18)
loggerhead/history.py (+2/-15)
To merge this branch: bzr merge lp:~danilo/loggerhead/bug-718566
Reviewer Review Type Date Requested Status
John A Meinel Approve
Review via email: mp+71849@code.launchpad.net

Description of the change

= Bug 718566 =

Remove FileChangeCache because it's not needed anymore (as suggested by John in bug 718566, comment #2).

All tests pass, and ./serve-branches browsing to a revision keeps working.

I'd welcome any ideas on how to best QA this.

To post a comment you must log in.
Revision history for this message
John A Meinel (jameinel) wrote :

The only reason we needed the file changes cache was because formats pre-2.0 were much slower about finding what files have changed (as mentioned in the comment).

The places this shows up is stuff like when you expand a revision on "Changes" and it shows you what files were changed in that revision.

There shouldn't be an effect on correctness. The big question is one of performance. So the best bet is to run loggerhead on a large project (launchpad could be large enough). Then expand some revisions, to see it churn on showing the file list. Then restart loggerhead, and time the same queries.

With the cache, it should certainly be faster. But without a cache if it is *fast enough* then we don't need the cache.

Overall, I approve of this change, given that I suggested it. But it would be good to get some hard performance data before we roll this out on Launchpad.

review: Approve
Revision history for this message
Данило Шеган (danilo) wrote :

Benchmarking summary
--------------------

Running "ab -n 100 -c 5" on http://127.0.0.1:8080/changes and http://127.0.0.1:8080/revision/13402

                Time per request
 ..................................................
                 /changes /revision/13402
 trunk: 338.897ms 361.218ms
 cache removed: 343.560ms 433.962ms

Revision 13402 is 4402 lines long and modified 57 files. Variation is much bigger on /revision page, and I can't tell if this is an acceptable performance loss (also, repeated benchmarking attempts made results much more stable and closer to trunk results, probably due to disk caches).

Full results on http://paste.ubuntu.com/668190/

Revision history for this message
John A Meinel (jameinel) wrote :

I'd like to see the time difference for +<email address hidden>

(The revlog page corresponding to that revision page.)

However, I think this falls under:

1) We're significantly underneath the 9s HARD timeout
2) We're even under a 5s desired HARD timeout
3) We're still well under the goal of 1s fast results.
4) Only concern remaining is cold-cache results. However, given that we are removing a cache that wouldn't be populated unless it was accessed before, we aren't regressing that at all.

My vote is in favor of this change.

Revision history for this message
Данило Шеган (danilo) wrote :

The timing is 325ms with this branch vs 267ms for trunk (with cache pre-populated). I'll be landing this then. Any tips on how to QA this on our existing Launchpad setup?

Revision history for this message
John A Meinel (jameinel) wrote :

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

On 8/17/2011 2:29 PM, Данило Шеган wrote:
> The timing is 325ms with this branch vs 267ms for trunk (with cache
> pre-populated). I'll be landing this then. Any tips on how to QA
> this on our existing Launchpad setup?

I would say, go to revision pages and /changes pages expanding the
revisions that are interesting. And make sure things don't time out.

Note, however, that the existing code timed out when I tried to go to
  http://bazaar.launchpad.net/+branch/launchpad/revision/13402

So what matters is that it doesn't time out *more* than it does today.

So QA seems particularly bad. If you QA and things generally work,
great. If you QA and things fail, it is hard to say that they would have
succeeded.

I'm not sure what branches are available on QAStaging, but I think at
least now you can get to loggerhead.

 http://bazaar.qastaging.launchpad.net/+branch/bzr

Tells me that Launchpad is offline for maintenance, though.

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk5Ltw8ACgkQJdeBCYSNAANdhQCdHLvDWUM+QJt4NjIkjDNqbPlD
yL0An2PFAiXskXhx1iTTJc/I6kT1W4p5
=LWxR
-----END PGP SIGNATURE-----

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 2011-06-28 16:09:37 +0000
3+++ loggerhead/apps/branch.py 2011-08-17 11:13:24 +0000
4@@ -63,23 +63,20 @@
5 self.use_cdn = use_cdn
6
7 def get_history(self):
8- file_cache = None
9 revinfo_disk_cache = None
10 cache_path = self._config.get('cachepath', None)
11 if cache_path is not None:
12 # Only import the cache if we're going to use it.
13 # This makes sqlite optional
14 try:
15- from loggerhead.changecache import (
16- FileChangeCache, RevInfoDiskCache)
17+ from loggerhead.changecache import RevInfoDiskCache
18 except ImportError:
19 self.log.debug("Couldn't load python-sqlite,"
20 " continuing without using a cache")
21 else:
22- file_cache = FileChangeCache(cache_path)
23 revinfo_disk_cache = RevInfoDiskCache(cache_path)
24 return History(
25- self.branch, self.graph_cache, file_cache=file_cache,
26+ self.branch, self.graph_cache,
27 revinfo_disk_cache=revinfo_disk_cache, cache_key=self.friendly_name)
28
29 def url(self, *args, **kw):
30
31=== modified file 'loggerhead/changecache.py'
32--- loggerhead/changecache.py 2009-05-02 14:01:05 +0000
33+++ loggerhead/changecache.py 2011-08-17 11:13:24 +0000
34@@ -99,24 +99,6 @@
35 pass
36
37
38-class FileChangeCache(object):
39-
40- def __init__(self, cache_path):
41-
42- if not os.path.exists(cache_path):
43- os.mkdir(cache_path)
44-
45- self._changes_filename = os.path.join(cache_path, 'filechanges.sql')
46-
47- def get_file_changes(self, entry):
48- cache = FakeShelf(self._changes_filename)
49- changes = cache.get(entry.revid)
50- if changes is None:
51- changes = self.history.get_file_changes_uncached(entry)
52- cache.add(entry.revid, changes)
53- return changes
54-
55-
56 class RevInfoDiskCache(object):
57 """Like `RevInfoMemoryCache` but backed in a sqlite DB."""
58
59
60=== modified file 'loggerhead/history.py'
61--- loggerhead/history.py 2011-03-25 13:09:10 +0000
62+++ loggerhead/history.py 2011-08-17 11:13:24 +0000
63@@ -215,8 +215,6 @@
64 around it, serve the request, throw the History object away, unlock the
65 branch and throw it away.
66
67- :ivar _file_change_cache: An object that caches information about the
68- files that changed between two revisions.
69 :ivar _rev_info: A list of information about revisions. This is by far
70 the most cryptic data structure in loggerhead. At the top level, it
71 is a list of 3-tuples [(merge-info, where-merged, parents)].
72@@ -284,15 +282,10 @@
73 self._rev_indices[revid] = seq
74 self._revno_revid[revno_str] = revid
75
76- def __init__(self, branch, whole_history_data_cache, file_cache=None,
77+ def __init__(self, branch, whole_history_data_cache,
78 revinfo_disk_cache=None, cache_key=None):
79 assert branch.is_locked(), (
80 "Can only construct a History object with a read-locked branch.")
81- if file_cache is not None:
82- self._file_change_cache = file_cache
83- file_cache.history = self
84- else:
85- self._file_change_cache = None
86 self._branch = branch
87 self._branch_tags = None
88 self._inventory_cache = {}
89@@ -759,19 +752,13 @@
90 entry["foreign_revid"] = mapping.vcs.show_foreign_revid(foreign_revid)
91 return util.Container(entry)
92
93- def get_file_changes_uncached(self, entry):
94+ def get_file_changes(self, entry):
95 if entry.parents:
96 old_revid = entry.parents[0].revid
97 else:
98 old_revid = bzrlib.revision.NULL_REVISION
99 return self.file_changes_for_revision_ids(old_revid, entry.revid)
100
101- def get_file_changes(self, entry):
102- if self._file_change_cache is None:
103- return self.get_file_changes_uncached(entry)
104- else:
105- return self._file_change_cache.get_file_changes(entry)
106-
107 def add_changes(self, entry):
108 changes = self.get_file_changes(entry)
109 entry.changes = changes

Subscribers

People subscribed via source and target branches