This is an archive of the discontinued Mercurial Phabricator instance.

Differential D10025

revlogv2: don't assume that the sidedata of the last rev is right after data
ClosedPublic

Authored by Alphare on Feb 19 2021, 6:16 AM.

Details

Reviewers
indygreg
marmoute
Group Reviewers
hg-reviewers
Commits
rHG4cd214c9948d: revlogv2: don't assume that the sidedata of the last rev is right after data
Summary

We are going to be rewriting sidedata soon, it's going to be appended to the
revlog data file, meaning that the data and the sidedata might not be
contiguous.

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

Alphare created this revision.Feb 19 2021, 6:16 AM
baymax updated this revision to Diff 25992.Mar 1 2021, 11:51 AM

✅ refresh by Heptapod after a successful CI run (🐙 💚)

marmoute requested changes to this revision.Mar 2 2021, 1:48 PM
This revision now requires changes to proceed.Mar 2 2021, 1:48 PM
Alphare updated this revision to Diff 26088.Mar 4 2021, 10:18 AM
marmoute requested changes to this revision.Mar 9 2021, 11:16 AM
marmoute added inline comments.
mercurial/revlog.py
2338–2341

Why don't we simply store the absolute offset for the sidedata instead of needing arithmetic ?

This revision now requires changes to proceed.Mar 9 2021, 11:16 AM
Alphare added inline comments.Mar 10 2021, 8:57 AM
mercurial/revlog.py
2338–2341

We do store the absolute offset, the point here is to find the index after the last entry's (potential) sidedata. Perhaps this variable should be named prev_sidedata_offset to make that clearer?

marmoute added inline comments.Mar 11 2021, 1:07 PM
mercurial/revlog.py
2338–2341

So we are looking to the offset of the first byte that does not contains data?

If so why are we looking for side data in particular and not just actual data too? If not, what is going on here ?

Alphare added inline comments.Mar 12 2021, 3:56 AM
mercurial/revlog.py
2338–2341

Revlog data files are still append-only (even with the re-writing mechanism of the next patches in the stack), and the sidedata is the last thing that is written for any given entry. So if you want to find the offset of all data (in the general sense of the term), then you look for the end of the sidedata of the last entry. Is there something I missed about there being a simpler solution, or should I add a comment explaining this?

marmoute added inline comments.Mar 12 2021, 4:04 AM
mercurial/revlog.py
2338–2341

Is it ? Because if the last entry did not had any sidedata. We need to take look at its actual data instead, isn't it? and actually if the previous entry had sidedata that we needed to compute after the fact, it sidedata will be written after the last entry data… etc.

So the current approach seems quite fragile to me. It need a least a clear comment explaining what is it trying to do. And probably jus to be moved within a function with a clear semantic and docstring. The "last offset" information could be stored in the docket to make all this simpler soonish.

Alphare added inline comments.Mar 12 2021, 4:11 AM
mercurial/revlog.py
2338–2341

if the last entry did not have any sidedata, we need to take look at its actual data instead

Right, hence the check for the sidedata offset being 0, which falls back to the regular end.

if the previous entry had sidedata that we needed to compute after the fact, it sidedata will be written after the last entry data

If the sidedata needs to be computer after the fact, it means - for now - that it doesn't currently have sidedata, so we'll write (correctly) after the end of the data, do so for however many revisions in the group, then compute their potential respective sidedata and append that to the data file one right after the other (so, separate from their data, but always after).

Is that clearer? If so I'll explain better in code.

marmoute added inline comments.Mar 12 2021, 4:28 AM
mercurial/revlog.py
2338–2341

What is the case I described (last revision with no sidedata, previous one with sidedata post-computed) happens for data committed before we start this write.

Alphare added inline comments.Mar 12 2021, 5:34 AM
mercurial/revlog.py
2338–2341

Right, sorry, I misread your previous message. I'm sending a change with a linear scan of the index as a first step, pending the use of a docket file, so at least it's correct in all cases.

Alphare updated this revision to Diff 26253.Mar 12 2021, 6:33 AM
marmoute requested changes to this revision.Mar 12 2021, 1:09 PM
marmoute added inline comments.
mercurial/revlog.py
2429

before getting out of experimental actually. We should add the TODO next the associated config in config item too.

2441

We should take the max of both no matter what would we not ? offset = max(offset, self.end(rev), self.end_sidedata(rev)) or something similar.

This revision now requires changes to proceed.Mar 12 2021, 1:09 PM
Alphare marked 2 inline comments as done.Mar 15 2021, 6:24 AM
Alphare updated this revision to Diff 26331.
marmoute accepted this revision.Mar 15 2021, 6:59 AM
Alphare added a commit: rHG4cd214c9948d: revlogv2: don't assume that the sidedata of the last rev is right after data.Mar 15 2021, 2:31 PM
This revision was not accepted when it landed; it landed in state Needs Review.
Closed by commit rHG4cd214c9948d: revlogv2: don't assume that the sidedata of the last rev is right after data (authored by Alphare). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mmercurial/revlog.py (25 lines)

Diff 26376

mercurial/revlog.py

except IndexError: except IndexError:
if rev == wdirrev: if rev == wdirrev:
raise error.WdirUnsupported raise error.WdirUnsupported
raise raise
# Derived from index values. # Derived from index values.
def end(self, rev): def end(self, rev):
return self.start(rev) + self.length(rev) + self.sidedata_length(rev) return self.start(rev) + self.length(rev)
def parents(self, node): def parents(self, node):
i = self.index i = self.index
d = i[self.rev(node)] d = i[self.rev(node)]
return i[d[5]][7], i[d[6]][7] # map revisions to nodes inline return i[d[5]][7], i[d[6]][7] # map revisions to nodes inline
def chainlen(self, rev): def chainlen(self, rev):
return self._chaininfo(rev)[0] return self._chaininfo(rev)[0]
fh = ifh fh = ifh
else: else:
fh = dfh fh = dfh
btext = [rawtext] btext = [rawtext]
curr = len(self) curr = len(self)
prev = curr - 1 prev = curr - 1
offset = self.end(prev)
offset = self._get_data_offset(prev)
if self._concurrencychecker: if self._concurrencychecker:
if self._inline: if self._inline:
# offset is "as if" it were in the .d file, so we need to add on # offset is "as if" it were in the .d file, so we need to add on
# the size of the entry metadata. # the size of the entry metadata.
self._concurrencychecker( self._concurrencychecker(
marmouteUnsubmitted
Not Done

Why don't we simply store the absolute offset for the sidedata instead of needing arithmetic ?

marmoute: Why don't we simply store the absolute offset for the sidedata instead of needing arithmetic ?
AlphareAuthorUnsubmitted
Done

We do store the absolute offset, the point here is to find the index after the last entry's (potential) sidedata. Perhaps this variable should be named prev_sidedata_offset to make that clearer?

Alphare: We do store the absolute offset, the point here is to find the index after the last entry's…
marmouteUnsubmitted
Not Done

So we are looking to the offset of the first byte that does not contains data?

If so why are we looking for side data in particular and not just actual data too? If not, what is going on here ?

marmoute: So we are looking to the offset of the first byte that does not contains data? If so why are…
AlphareAuthorUnsubmitted
Done

Revlog data files are still append-only (even with the re-writing mechanism of the next patches in the stack), and the sidedata is the last thing that is written for any given entry. So if you want to find the offset of all data (in the general sense of the term), then you look for the end of the sidedata of the last entry. Is there something I missed about there being a simpler solution, or should I add a comment explaining this?

Alphare: Revlog data files are still append-only (even with the re-writing mechanism of the next patches…
marmouteUnsubmitted
Not Done

Is it ? Because if the last entry did not had any sidedata. We need to take look at its actual data instead, isn't it? and actually if the previous entry had sidedata that we needed to compute after the fact, it sidedata will be written after the last entry data… etc.

So the current approach seems quite fragile to me. It need a least a clear comment explaining what is it trying to do. And probably jus to be moved within a function with a clear semantic and docstring. The "last offset" information could be stored in the docket to make all this simpler soonish.

marmoute: Is it ? Because if the last entry did not had any sidedata. We need to take look at its actual…
AlphareAuthorUnsubmitted
Done

if the last entry did not have any sidedata, we need to take look at its actual data instead

Right, hence the check for the sidedata offset being 0, which falls back to the regular end.

if the previous entry had sidedata that we needed to compute after the fact, it sidedata will be written after the last entry data

If the sidedata needs to be computer after the fact, it means - for now - that it doesn't currently have sidedata, so we'll write (correctly) after the end of the data, do so for however many revisions in the group, then compute their potential respective sidedata and append that to the data file one right after the other (so, separate from their data, but always after).

Is that clearer? If so I'll explain better in code.

Alphare: > if the last entry did not have any sidedata, we need to take look at its actual data instead…
marmouteUnsubmitted
Not Done

What is the case I described (last revision with no sidedata, previous one with sidedata post-computed) happens for data committed before we start this write.

marmoute: What is the case I described (last revision with no sidedata, previous one with sidedata post…
AlphareAuthorUnsubmitted
Done

Right, sorry, I misread your previous message. I'm sending a change with a linear scan of the index as a first step, pending the use of a docket file, so at least it's correct in all cases.

Alphare: Right, sorry, I misread your previous message. I'm sending a change with a linear scan of the…
ifh, self.indexfile, offset + curr * self._io.size ifh, self.indexfile, offset + curr * self._io.size
) )
else: else:
# Entries in the .i are a consistent size. # Entries in the .i are a consistent size.
self._concurrencychecker( self._concurrencychecker(
ifh, self.indexfile, curr * self._io.size ifh, self.indexfile, curr * self._io.size
) )
self._concurrencychecker(dfh, self.datafile, offset) self._concurrencychecker(dfh, self.datafile, offset)
if alwayscache and rawtext is None: if alwayscache and rawtext is None:
rawtext = deltacomputer.buildtext(revinfo, fh) rawtext = deltacomputer.buildtext(revinfo, fh)
if type(rawtext) == bytes: # only accept immutable objects if type(rawtext) == bytes: # only accept immutable objects
self._revisioncache = (node, curr, rawtext) self._revisioncache = (node, curr, rawtext)
self._chainbasecache[curr] = deltainfo.chainbase self._chainbasecache[curr] = deltainfo.chainbase
return curr return curr
def _get_data_offset(self, prev):
"""Returns the current offset in the (in-transaction) data file.
Versions < 2 of the revlog can get this 0(1), revlog v2 needs a docket
file to store that information: since sidedata can be rewritten to the
end of the data file within a transaction, you can have cases where, for
example, rev `n` does not have sidedata while rev `n - 1` does, leading
to `n - 1`'s sidedata being written after `n`'s data.
TODO cache this in a docket file before getting out of experimental."""
marmouteUnsubmitted
Done

before getting out of experimental actually. We should add the TODO next the associated config in config item too.

marmoute: before getting out of experimental actually. We should add the TODO next the associated config…
if self.version & 0xFFFF != REVLOGV2:
return self.end(prev)
offset = 0
for rev, entry in enumerate(self.index):
sidedata_end = entry[8] + entry[9]
# Sidedata for a previous rev has potentially been written after
# this rev's end, so take the max.
offset = max(self.end(rev), offset, sidedata_end)
return offset
def _writeentry( def _writeentry(
marmouteUnsubmitted
Done

We should take the max of both no matter what would we not ? offset = max(offset, self.end(rev), self.end_sidedata(rev)) or something similar.

marmoute: We should take the max of both no matter what would we not ? `offset = max(offset, self.end…
self, transaction, ifh, dfh, entry, data, link, offset, sidedata self, transaction, ifh, dfh, entry, data, link, offset, sidedata
): ):
# Files opened in a+ mode have inconsistent behavior on various # Files opened in a+ mode have inconsistent behavior on various
# platforms. Windows requires that a file positioning call be made # platforms. Windows requires that a file positioning call be made
# when the file handle transitions between reads and writes. See # when the file handle transitions between reads and writes. See
# 3686fa2b8eee and the mixedfilemodewrapper in windows.py. On other # 3686fa2b8eee and the mixedfilemodewrapper in windows.py. On other
# platforms, Python or the platform itself can be buggy. Some versions # platforms, Python or the platform itself can be buggy. Some versions
# of Solaris have been observed to not append at the end of the file # of Solaris have been observed to not append at the end of the file