( )⚙ D7321 revlog: deal with nodemap deletion within the index

This is an archive of the discontinued Mercurial Phabricator instance.

Differential D7321

revlog: deal with nodemap deletion within the index
ClosedPublic

Authored by marmoute on Nov 8 2019, 4:29 AM.

Details

Reviewers
indygreg
Group Reviewers
hg-reviewers
Commits
rHG642433629e20: revlog: deal with nodemap deletion within the index
rHG5b556d46671d: revlog: deal with nodemap deletion within the index
Summary

Since the nodemap data now live in the index, it should be the index
responsibility to ensure the data are up to date.

The C version of the index is already dealing with such deletion.

This work is part of a refactoring to unify the revlog index and the nodemap.
This unification prepare the use of a persistent nodemap.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

marmoute created this revision.Nov 8 2019, 4:29 AM
marmoute updated this revision to Diff 17738.Nov 8 2019, 7:28 AM
indygreg added a comment.Nov 8 2019, 2:41 PM

I approve of the direction. In addition to inline comments, same deal as last patch: is there a sentinel entry to worry about?

mercurial/pure/parsers.py
60

pycompat.xrange.

mercurial/revlog.py
223

pycompat.xrange.

indygreg requested changes to this revision.Nov 8 2019, 2:41 PM
This revision now requires changes to proceed.Nov 8 2019, 2:41 PM
marmoute requested review of this revision.Nov 8 2019, 7:31 PM

regarding xrange: Same feedback as for D7313, this is a non performance critical section since this is about pure.

regarding the sentinel value: The revision we access during deletion are the same as the one actually removed from the index, so this seems correct. In addition, all test are happy.

martinvonz added a subscriber: martinvonz.Nov 8 2019, 8:27 PM

I think our comments up to here have been addressed (I'm pretty confident that the start + 1 thing is correct), so I'll queue this part. The only thing left on the rest of the series is the module policy thing I just sent a comment about on the next patch.

martinvonz added a comment.Nov 8 2019, 8:51 PM

I get a traceback from test-copytrace-heuristics.t with this patch.

marmoute added a comment.Nov 9 2019, 12:06 AM
In D7321#107805, @martinvonz wrote:

I get a traceback from test-copytrace-heuristics.t with this patch.

Well… I don't. Can you share some details? How are you running your tests?

marmoute updated this revision to Diff 17834.Nov 9 2019, 12:15 AM
martinvonz added a comment.Nov 9 2019, 1:58 AM
In D7321#107839, @marmoute wrote:
In D7321#107805, @martinvonz wrote:

I get a traceback from test-copytrace-heuristics.t with this patch.

Well… I don't. Can you share some details? How are you running your tests?

I think I figured it out. And so did you, I think. We just didn't know that were talking about the same failure :) On D7320, you wrote:

Without this change, D7321 crash horribly, and debugging show that the value left out is clearly a non-sentinel value that should be deleted.
With this change, D7321 pass all test flawlessly.

I think the "crash horribly" there was referring to test-copytrace-heuristics.t. If I don't recompile after applying D7320 and D7321 (this patch), then I get a crash ending like this:

+    File "/usr/local/google/home/martinvonz/hg/mercurial/bundle2.py", line 489, in _processchangegroup
+      ret = cg.apply(op.repo, tr, source, url, **kwargs)
+    File "/usr/local/google/home/martinvonz/hg/mercurial/changegroup.py", line 345, in apply
+      self._unpackmanifests(repo, revmap, trp, progress)
+    File "/usr/local/google/home/martinvonz/hg/mercurial/changegroup.py", line 258, in _unpackmanifests
+      repo.manifestlog.getstorage(b'').addgroup(deltas, revmap, trp)
+    File "/usr/local/google/home/martinvonz/hg/mercurial/manifest.py", line 1762, in addgroup
+      deltas, linkmapper, transaction, addrevisioncb=addrevisioncb
+    File "/usr/local/google/home/martinvonz/hg/mercurial/revlog.py", line 2305, in addgroup
+      if node in self.nodemap:
+  IndexError: could not access rev 3
+  [1]

I assume that's what you also saw, but please confirm.

marmoute added a comment.Nov 9 2019, 3:07 AM
In D7321#107872, @martinvonz wrote:
In D7321#107839, @marmoute wrote:
In D7321#107805, @martinvonz wrote:

I get a traceback from test-copytrace-heuristics.t with this patch.

Well… I don't. Can you share some details? How are you running your tests?

I think I figured it out. And so did you, I think. We just didn't know that were talking about the same failure :) On D7320, you wrote:

Without this change, D7321 crash horribly, and debugging show that the value left out is clearly a non-sentinel value that should be deleted.
With this change, D7321 pass all test flawlessly.

I think the "crash horribly" there was referring to test-copytrace-heuristics.t. If I don't recompile after applying D7320 and D7321 (this patch), then I get a crash ending like this:

+    File "/usr/local/google/home/martinvonz/hg/mercurial/bundle2.py", line 489, in _processchangegroup
+      ret = cg.apply(op.repo, tr, source, url, **kwargs)
+    File "/usr/local/google/home/martinvonz/hg/mercurial/changegroup.py", line 345, in apply
+      self._unpackmanifests(repo, revmap, trp, progress)
+    File "/usr/local/google/home/martinvonz/hg/mercurial/changegroup.py", line 258, in _unpackmanifests
+      repo.manifestlog.getstorage(b'').addgroup(deltas, revmap, trp)
+    File "/usr/local/google/home/martinvonz/hg/mercurial/manifest.py", line 1762, in addgroup
+      deltas, linkmapper, transaction, addrevisioncb=addrevisioncb
+    File "/usr/local/google/home/martinvonz/hg/mercurial/revlog.py", line 2305, in addgroup
+      if node in self.nodemap:
+  IndexError: could not access rev 3
+  [1]

I assume that's what you also saw, but please confirm.

Yeah, this is a lack of recompilation of the C code. (maybe I should bump the version at very single patch touching the c file ?)

marmoute added a commit: rHG5b556d46671d: revlog: deal with nodemap deletion within the index.Nov 9 2019, 3:38 AM
This revision was not accepted when it landed; it landed in state Needs Review.
Closed by commit rHG5b556d46671d: revlog: deal with nodemap deletion within the index (authored by marmoute). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 17864

mercurial/bundlerepo.py

-1, -1,
baserev, baserev,
link, link,
self.rev(p1), self.rev(p1),
self.rev(p2), self.rev(p2),
node, node,
) )
self.index.append(e) self.index.append(e)
self.nodemap[node] = n
self.bundlerevs.add(n) self.bundlerevs.add(n)
n += 1 n += 1
def _chunk(self, rev, df=None): def _chunk(self, rev, df=None):
# Warning: in case of bundle, the diff is against what we stored as # Warning: in case of bundle, the diff is against what we stored as
# delta base, not against rev - 1 # delta base, not against rev - 1
# XXX: could use some caching # XXX: could use some caching
if rev <= self.repotiprev: if rev <= self.repotiprev:

mercurial/pure/parsers.py

@util.propertycache @util.propertycache
def nodemap(self): def nodemap(self):
nodemap = revlogutils.NodeMap({nullid: nullrev}) nodemap = revlogutils.NodeMap({nullid: nullrev})
for r in range(0, len(self)): for r in range(0, len(self)):
n = self[r][7] n = self[r][7]
nodemap[n] = r nodemap[n] = r
return nodemap return nodemap
def _stripnodes(self, start):
if 'nodemap' in vars(self):
for r in range(start, len(self)):
indygregUnsubmitted
Not Done

pycompat.xrange.

indygreg: `pycompat.xrange`.
n = self[r][7]
del self.nodemap[n]
def clearcaches(self): def clearcaches(self):
self.__dict__.pop('nodemap', None) self.__dict__.pop('nodemap', None)
def __len__(self): def __len__(self):
return self._lgt + len(self._extra) return self._lgt + len(self._extra)
def append(self, tup): def append(self, tup):
if 'nodemap' in vars(self): if 'nodemap' in vars(self):
def _calculate_index(self, i): def _calculate_index(self, i):
return i * indexsize return i * indexsize
def __delitem__(self, i): def __delitem__(self, i):
if not isinstance(i, slice) or not i.stop == -1 or i.step is not None: if not isinstance(i, slice) or not i.stop == -1 or i.step is not None:
raise ValueError(b"deleting slices only supports a:-1 with step 1") raise ValueError(b"deleting slices only supports a:-1 with step 1")
i = i.start i = i.start
self._check_index(i) self._check_index(i)
self._stripnodes(i)
if i < self._lgt: if i < self._lgt:
self._data = self._data[: i * indexsize] self._data = self._data[: i * indexsize]
self._lgt = i self._lgt = i
self._extra = [] self._extra = []
else: else:
self._extra = self._extra[: i - self._lgt] self._extra = self._extra[: i - self._lgt]
raise ValueError(b"corrupted data") raise ValueError(b"corrupted data")
return count return count
def __delitem__(self, i): def __delitem__(self, i):
if not isinstance(i, slice) or not i.stop == -1 or i.step is not None: if not isinstance(i, slice) or not i.stop == -1 or i.step is not None:
raise ValueError(b"deleting slices only supports a:-1 with step 1") raise ValueError(b"deleting slices only supports a:-1 with step 1")
i = i.start i = i.start
self._check_index(i) self._check_index(i)
self._stripnodes(i)
if i < self._lgt: if i < self._lgt:
self._offsets = self._offsets[:i] self._offsets = self._offsets[:i]
self._lgt = i self._lgt = i
self._extra = [] self._extra = []
else: else:
self._extra = self._extra[: i - self._lgt] self._extra = self._extra[: i - self._lgt]
def _calculate_index(self, i): def _calculate_index(self, i):

mercurial/revlog.py

n = self[r][7] n = self[r][7]
nodemap[n] = r nodemap[n] = r
return nodemap return nodemap
def append(self, tup): def append(self, tup):
self.nodemap[tup[7]] = len(self) self.nodemap[tup[7]] = len(self)
super(revlogoldindex, self).append(tup) super(revlogoldindex, self).append(tup)
def __delitem__(self, i):
if not isinstance(i, slice) or not i.stop == -1 or i.step is not None:
raise ValueError(b"deleting slices only supports a:-1 with step 1")
for r in pycompat.xrange(i.start, len(self)):
indygregUnsubmitted
Not Done

pycompat.xrange.

indygreg: `pycompat.xrange`.
del self.nodemap[self[r][7]]
super(revlogoldindex, self).__delitem__(i)
def clearcaches(self): def clearcaches(self):
self.__dict__.pop('nodemap', None) self.__dict__.pop('nodemap', None)
def __getitem__(self, i): def __getitem__(self, i):
if i == -1: if i == -1:
return (0, 0, 0, -1, -1, -1, -1, nullid) return (0, 0, 0, -1, -1, -1, -1, nullid)
return list.__getitem__(self, i) return list.__getitem__(self, i)
end += rev * self._io.size end += rev * self._io.size
transaction.add(self.indexfile, end) transaction.add(self.indexfile, end)
# then reset internal state in memory to forget those revisions # then reset internal state in memory to forget those revisions
self._revisioncache = None self._revisioncache = None
self._chaininfocache = {} self._chaininfocache = {}
self._chunkclear() self._chunkclear()
for x in pycompat.xrange(rev, len(self)):
del self.nodemap[self.node(x)]
del self.index[rev:-1] del self.index[rev:-1]
self._nodepos = None self._nodepos = None
def checksize(self): def checksize(self):
"""Check size of index and data files """Check size of index and data files
return a (dd, di) tuple. return a (dd, di) tuple.

mercurial/unionrepo.py

rsize, rsize,
base, base,
link, link,
self.rev(p1node), self.rev(p1node),
self.rev(p2node), self.rev(p2node),
node, node,
) )
self.index.append(e) self.index.append(e)
self.nodemap[node] = n
self.bundlerevs.add(n) self.bundlerevs.add(n)
n += 1 n += 1
def _chunk(self, rev): def _chunk(self, rev):
if rev <= self.repotiprev: if rev <= self.repotiprev:
return revlog.revlog._chunk(self, rev) return revlog.revlog._chunk(self, rev)
return self.revlog2._chunk(self.node(rev)) return self.revlog2._chunk(self.node(rev))