This is an archive of the discontinued Mercurial Phabricator instance.

Differential D2068

revlog: do not use delta for lfs revisions
ClosedPublic

Authored by quark on Feb 6 2018, 8:14 PM.

Details

Reviewers
indygreg
ryanmce
Group Reviewers
hg-reviewers
Commits
rHG33275ab5e837: revlog: do not use delta for lfs revisions
rHG5c47fa90bf13: revlog: do not use delta for lfs revisions
Summary

This is similar to what we have done for changegroups. It is needed to make
sure the delta application code path can assume deltas are always against
vanilla (ex. non-LFS) rawtext so the next fix becomes possible.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

quark created this revision.Feb 6 2018, 8:14 PM
quark updated this revision to Diff 5270.Feb 6 2018, 9:13 PM
quark updated this revision to Diff 5274.Feb 6 2018, 10:18 PM
indygreg requested changes to this revision.Feb 7 2018, 5:35 PM
indygreg added inline comments.
mercurial/revlog.py
411

Same comment as previous review: this leaves a foot gun if we ever introduce revision flags that aren't related to content presence. We need something to help prevent this footgun.

This revision now requires changes to proceed.Feb 7 2018, 5:35 PM
quark updated this revision to Diff 5328.Feb 7 2018, 7:35 PM
quark updated this revision to Diff 5329.Feb 7 2018, 7:38 PM
quark updated this revision to Diff 5651.Feb 13 2018, 2:41 PM
ryanmce accepted this revision.Mar 3 2018, 2:36 PM
ryanmce added inline comments.Mar 3 2018, 2:48 PM
mercurial/revlog.py
2110

Why is this not candelta()?

quark added inline comments.Mar 3 2018, 2:52 PM
mercurial/revlog.py
2110

candelta takes two revisions. Here there is only one revision. It's possible to pass a useless revision but that has unnecessary overhead.

candelta also fetches flags, here we already know the values flags so it's faster to avoid fetching it again.

ryanmce added inline comments.Mar 3 2018, 3:21 PM
mercurial/revlog.py
2110

Then why is this REVIDX_KNOWN_FLAGS and not REVIDX_RAWTEXT_CHANGING_FLAGS?

Given the number of questions here from me and others, I think the comment could use extension at least so future readers understand why this is the way it is.

quark updated this revision to Diff 6470.Mar 3 2018, 3:27 PM
quark added inline comments.Mar 3 2018, 6:43 PM
mercurial/revlog.py
2110

Should be REVIDX_RAWTEXT_CHANGING_FLAGS. I'll make the change.

quark updated this revision to Diff 6668.Mar 6 2018, 12:42 PM
indygreg accepted this revision.Mar 6 2018, 1:03 PM
indygreg added inline comments.
tests/test-revlog-raw.py
156–164

I feel like this should be checking against specific flags. But since this is a test, I'm fine accepting this. We can fix in a follow-up if the logic is wrong.

This revision is now accepted and ready to land.Mar 6 2018, 1:03 PM
quark added inline comments.Mar 6 2018, 1:09 PM
tests/test-revlog-raw.py
156–164

Seems I missed this line somehow. Will change in a minute.

Closed by commit rHG5c47fa90bf13: revlog: do not use delta for lfs revisions (authored by quark). · Explain WhyMar 6 2018, 1:10 PM
This revision was automatically updated to reflect the committed changes.
quark updated this revision to Diff 6672.Mar 6 2018, 1:15 PM

Revision Contents
Changeset List

PathPackages
Mmercurial/revlog.py (12 lines)
Mtests/test-lfs-bundle.t (5 lines)
Mtests/test-revlog-raw.py (15 lines)

Diff 6672

mercurial/revlog.py

p1 = revinfo.p1 p1 = revinfo.p1
p2 = revinfo.p2 p2 = revinfo.p2
revlog = self.revlog revlog = self.revlog
deltainfo = None deltainfo = None
for candidaterevs in self._getcandidaterevs(p1, p2, cachedelta): for candidaterevs in self._getcandidaterevs(p1, p2, cachedelta):
nominateddeltas = [] nominateddeltas = []
for candidaterev in candidaterevs: for candidaterev in candidaterevs:
# no delta for rawtext-changing revs (see "candelta" for why)
if revlog.flags(candidaterev) & REVIDX_RAWTEXT_CHANGING_FLAGS:
indygregUnsubmitted
Not Done

Same comment as previous review: this leaves a foot gun if we ever introduce revision flags that aren't related to content presence. We need something to help prevent this footgun.

indygreg: Same comment as previous review: this leaves a foot gun if we ever introduce revision flags…
continue
candidatedelta = self._builddeltainfo(revinfo, candidaterev, fh) candidatedelta = self._builddeltainfo(revinfo, candidaterev, fh)
if revlog._isgooddeltainfo(candidatedelta, revinfo.textlen): if revlog._isgooddeltainfo(candidatedelta, revinfo.textlen):
nominateddeltas.append(candidatedelta) nominateddeltas.append(candidatedelta)
if nominateddeltas: if nominateddeltas:
deltainfo = min(nominateddeltas, key=lambda x: x.deltalen) deltainfo = min(nominateddeltas, key=lambda x: x.deltalen)
break break
return deltainfo return deltainfo
cachedelta[1]) cachedelta[1])
else: else:
textlen = len(rawtext) textlen = len(rawtext)
if deltacomputer is None: if deltacomputer is None:
deltacomputer = _deltacomputer(self) deltacomputer = _deltacomputer(self)
revinfo = _revisioninfo(node, p1, p2, btext, textlen, cachedelta, flags) revinfo = _revisioninfo(node, p1, p2, btext, textlen, cachedelta, flags)
# no delta for flag processor revision (see "candelta" for why)
# not calling candelta since only one revision needs test, also to
ryanmceUnsubmitted
Not Done

Why is this not candelta()?

ryanmce: Why is this not `candelta()`?
quarkAuthorUnsubmitted
Not Done

candelta takes two revisions. Here there is only one revision. It's possible to pass a useless revision but that has unnecessary overhead.

candelta also fetches flags, here we already know the values flags so it's faster to avoid fetching it again.

quark: `candelta` takes two revisions. Here there is only one revision. It's possible to pass a…
ryanmceUnsubmitted
Not Done

Then why is this REVIDX_KNOWN_FLAGS and not REVIDX_RAWTEXT_CHANGING_FLAGS?

Given the number of questions here from me and others, I think the comment could use extension at least so future readers understand why this is the way it is.

ryanmce: Then why is this `REVIDX_KNOWN_FLAGS` and not `REVIDX_RAWTEXT_CHANGING_FLAGS`? Given the…
quarkAuthorUnsubmitted
Not Done

Should be REVIDX_RAWTEXT_CHANGING_FLAGS. I'll make the change.

quark: Should be `REVIDX_RAWTEXT_CHANGING_FLAGS`. I'll make the change.
# avoid overhead fetching flags again.
if flags & REVIDX_RAWTEXT_CHANGING_FLAGS:
deltainfo = None
else:
deltainfo = deltacomputer.finddeltainfo(revinfo, fh) deltainfo = deltacomputer.finddeltainfo(revinfo, fh)
if deltainfo is not None: if deltainfo is not None:
base = deltainfo.base base = deltainfo.base
chainbase = deltainfo.chainbase chainbase = deltainfo.chainbase
data = deltainfo.data data = deltainfo.data
l = deltainfo.deltalen l = deltainfo.deltalen
else: else:
rawtext = deltacomputer.buildtext(revinfo, fh) rawtext = deltacomputer.buildtext(revinfo, fh)

tests/test-lfs-bundle.t

> else > else
> echo CRASHED > echo CRASHED
> fi > fi
> done > done
> done > done
---- Applying src-normal.bundle to dst-normal ---- ---- Applying src-normal.bundle to dst-normal ----
OK OK
---- Applying src-normal.bundle to dst-lfs ---- ---- Applying src-normal.bundle to dst-lfs ----
X@2: unpacking bcc7d23fa6b7: integrity check failed on data/X.i:2 CRASHED
Y@2: unpacking 46017a6640e7: integrity check failed on data/Y.i:2
2 integrity errors encountered!
(first damaged changeset appears to be 2)
---- Applying src-lfs.bundle to dst-normal ---- ---- Applying src-lfs.bundle to dst-normal ----
OK OK
---- Applying src-lfs.bundle to dst-lfs ---- ---- Applying src-lfs.bundle to dst-lfs ----
OK OK

tests/test-revlog-raw.py

r = parentrev + 1 r = parentrev + 1
if r >= len(rlog): if r >= len(rlog):
return {} return {}
if optimaldelta: if optimaldelta:
deltaparent = parentrev deltaparent = parentrev
else: else:
# suboptimal deltaparent # suboptimal deltaparent
deltaparent = min(0, parentrev) deltaparent = min(0, parentrev)
if not rlog.candelta(deltaparent, r):
deltaparent = -1
return {'node': rlog.node(r), 'p1': pnode, 'p2': node.nullid, return {'node': rlog.node(r), 'p1': pnode, 'p2': node.nullid,
'cs': rlog.node(rlog.linkrev(r)), 'flags': rlog.flags(r), 'cs': rlog.node(rlog.linkrev(r)), 'flags': rlog.flags(r),
'deltabase': rlog.node(deltaparent), 'deltabase': rlog.node(deltaparent),
'delta': rlog.revdiff(deltaparent, r)} 'delta': rlog.revdiff(deltaparent, r)}
def deltaiter(self): def deltaiter(self):
chain = None chain = None
for chunkdata in iter(lambda: self.deltachunk(chain), {}): for chunkdata in iter(lambda: self.deltachunk(chain), {}):
'''Like addgroupcopy, but use the low level revlog._addrevision directly. '''Like addgroupcopy, but use the low level revlog._addrevision directly.
It exercises some code paths that are hard to reach easily otherwise. It exercises some code paths that are hard to reach easily otherwise.
''' '''
dlog = newrevlog(destname, recreate=True) dlog = newrevlog(destname, recreate=True)
for r in rlog: for r in rlog:
p1 = rlog.node(r - 1) p1 = rlog.node(r - 1)
p2 = node.nullid p2 = node.nullid
if r == 0: if r == 0 or (rlog.flags(r) & revlog.REVIDX_EXTSTORED):
text = rlog.revision(r, raw=True) text = rlog.revision(r, raw=True)
cachedelta = None cachedelta = None
else: else:
# deltaparent is more interesting if it has the EXTSTORED flag. # deltaparent cannot have EXTSTORED flag.
deltaparent = max([0] + [p for p in range(r - 2) if rlog.flags(p)]) deltaparent = max([-1] +
[p for p in range(r)
if rlog.flags(p) & revlog.REVIDX_EXTSTORED == 0])
text = None text = None
indygregUnsubmitted
Not Done

I feel like this should be checking against specific flags. But since this is a test, I'm fine accepting this. We can fix in a follow-up if the logic is wrong.

indygreg: I feel like this should be checking against specific flags. But since this is a test, I'm fine…
quarkAuthorUnsubmitted
Not Done

Seems I missed this line somehow. Will change in a minute.

quark: Seems I missed this line somehow. Will change in a minute.
cachedelta = (deltaparent, rlog.revdiff(deltaparent, r)) cachedelta = (deltaparent, rlog.revdiff(deltaparent, r))
flags = rlog.flags(r) flags = rlog.flags(r)
ifh = dfh = None ifh = dfh = None
try: try:
ifh = dlog.opener(dlog.indexfile, 'a+') ifh = dlog.opener(dlog.indexfile, 'a+')
if not dlog._inline: if not dlog._inline:
dfh = dlog.opener(dlog.datafile, 'a+') dfh = dlog.opener(dlog.datafile, 'a+')
dlog._addrevision(rlog.node(r), text, tr, r, p1, p2, flags, dlog._addrevision(rlog.node(r), text, tr, r, p1, p2, flags,
abort('rev %d: wrong rawsize' % rev) abort('rev %d: wrong rawsize' % rev)
if rlog.revision(rev, raw=False) != text: if rlog.revision(rev, raw=False) != text:
abort('rev %d: wrong text' % rev) abort('rev %d: wrong text' % rev)
if rlog.revision(rev, raw=True) != rawtext: if rlog.revision(rev, raw=True) != rawtext:
abort('rev %d: wrong rawtext' % rev) abort('rev %d: wrong rawtext' % rev)
result.append((text, rawtext)) result.append((text, rawtext))
# Verify flags like isdelta, isext work as expected # Verify flags like isdelta, isext work as expected
if bool(rlog.deltaparent(rev) > -1) != isdelta: # isdelta can be overridden to False if this or p1 has isext set
abort('rev %d: isdelta is ineffective' % rev) if bool(rlog.deltaparent(rev) > -1) and not isdelta:
abort('rev %d: isdelta is unexpected' % rev)
if bool(rlog.flags(rev)) != isext: if bool(rlog.flags(rev)) != isext:
abort('rev %d: isext is ineffective' % rev) abort('rev %d: isext is ineffective' % rev)
return result return result
# Main test and checking # Main test and checking
def checkrevlog(rlog, expected): def checkrevlog(rlog, expected):
'''Check if revlog has expected contents. expected is [(text, rawtext)]''' '''Check if revlog has expected contents. expected is [(text, rawtext)]'''