This is an archive of the discontinued Mercurial Phabricator instance.

Differential D10031

revlog-index: add `replace_sidedata_info` method
ClosedPublic

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

Details

Reviewers
marmoute
Group Reviewers
hg-reviewers
Commits
rHG502e795b55ac: revlog-index: add `replace_sidedata_info` method
Summary

During a pull operation where the server does not provide sidedata, the client
that requires it should generate them on-the-fly. In the generic case, we need
to wait for the changelog + manifests + filelogs to be added, since we don't
know what the sidedata computers might need: this means rewriting the sidedata
of index entries from within the pull transaction (and no further back) right
after we've added them.

Both Python and C implementations only allow for rewriting the sidedata offset
and length for revs within the transaction where they were created.

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 25998.Mar 1 2021, 11:52 AM

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

marmoute requested changes to this revision.Mar 3 2021, 8:54 AM
marmoute added a subscriber: marmoute.
marmoute added inline comments.
mercurial/cext/revlog.c
513–530

Should we explicitly check that no value other than the sidedata have been changed ?

mercurial/pure/parsers.py
125–129

The changeset description state that we don't check the transaction thing in the python version. I assume that this is because we have some extra information in the C code that the python version don't have. Could we fix it ?

133–139

Same feedback as the C code, could we validate than we don't modify anything else than the side-data index ?

This revision now requires changes to proceed.Mar 3 2021, 8:54 AM
Alphare marked an inline comment as done.Mar 4 2021, 10:06 AM
Alphare added inline comments.
mercurial/cext/revlog.c
513–530

I changed it so the only arguments are sidedata length and offset

mercurial/pure/parsers.py
125–129

I misunderstood the Python implementation, this is now fixed.

Alphare edited the summary of this revision. (Show Details)Mar 4 2021, 10:18 AM
Alphare updated this revision to Diff 26094.
marmoute requested changes to this revision.Mar 9 2021, 11:52 AM

Great, I just have a final feedback. We should probably rename this method to something less generic than replace. What about replace_sidedata_info ?

This revision now requires changes to proceed.Mar 9 2021, 11:52 AM
Alphare added a comment.Mar 10 2021, 1:46 PM
In D10031#154121, @marmoute wrote:

Great, I just have a final feedback. We should probably rename this method to something less generic than replace. What about replace_sidedata_info ?

Sure

Alphare updated this revision to Diff 26225.Mar 10 2021, 2:12 PM
marmoute accepted this revision.Mar 11 2021, 1:16 PM
Alphare retitled this revision from revlog-index: add `replace` method to revlog-index: add `replace_sidedata_info` method.Mar 12 2021, 6:34 AM
Alphare edited the summary of this revision. (Show Details)
Alphare added a commit: rHG502e795b55ac: revlog-index: add `replace_sidedata_info` method.Mar 15 2021, 2:35 PM
This revision was not accepted when it landed; it landed in state Needs Review.
Closed by commit rHG502e795b55ac: revlog-index: add `replace_sidedata_info` method (authored by Alphare). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mmercurial/cext/revlog.c (52 lines)
Mmercurial/pure/parsers.py (21 lines)

Diff 26383

mercurial/cext/revlog.c

if (self->ntinitialized) if (self->ntinitialized)
nt_insert(&self->nt, c_node_id, rev); nt_insert(&self->nt, c_node_id, rev);
Py_CLEAR(self->headrevs); Py_CLEAR(self->headrevs);
Py_RETURN_NONE; Py_RETURN_NONE;
} }
/* Replace an existing index entry's sidedata offset and length with new ones.
This cannot be used outside of the context of sidedata rewriting,
inside the transaction that creates the given revision. */
static PyObject *index_replace_sidedata_info(indexObject *self, PyObject *args)
{
uint64_t sidedata_offset;
int rev;
Py_ssize_t sidedata_comp_len;
char *data;
#if LONG_MAX == 0x7fffffffL
const char *const sidedata_format = PY23("nKi", "nKi");
#else
const char *const sidedata_format = PY23("nki", "nki");
#endif
if (self->hdrsize == v1_hdrsize || self->inlined) {
/*
There is a bug in the transaction handling when going from an
inline revlog to a separate index and data file. Turn it off until
it's fixed, since v2 revlogs sometimes get rewritten on exchange.
See issue6485.
*/
raise_revlog_error();
return NULL;
}
if (!PyArg_ParseTuple(args, sidedata_format, &rev, &sidedata_offset,
&sidedata_comp_len))
return NULL;
if (rev < 0 || rev >= index_length(self)) {
PyErr_SetString(PyExc_IndexError, "revision outside index");
return NULL;
}
if (rev < self->length) {
PyErr_SetString(
PyExc_IndexError,
"cannot rewrite entries outside of this transaction");
return NULL;
}
/* Find the newly added node, offset from the "already on-disk" length */
data = self->added + self->hdrsize * (rev - self->length);
putbe64(sidedata_offset, data + 64);
putbe32(sidedata_comp_len, data + 72);
Py_RETURN_NONE;
}
static PyObject *index_stats(indexObject *self) static PyObject *index_stats(indexObject *self)
{ {
PyObject *obj = PyDict_New(); PyObject *obj = PyDict_New();
PyObject *s = NULL; PyObject *s = NULL;
PyObject *t = NULL; PyObject *t = NULL;
if (obj == NULL) if (obj == NULL)
return NULL; return NULL;
#define istat(__n, __d) \ #define istat(__n, __d) \
do { \ do { \
s = PyBytes_FromString(__d); \ s = PyBytes_FromString(__d); \
t = PyInt_FromSsize_t(self->__n); \ t = PyInt_FromSsize_t(self->__n); \
if (!s || !t) \ if (!s || !t) \
goto bail; \ goto bail; \
if (PyDict_SetItem(obj, s, t) == -1) \ if (PyDict_SetItem(obj, s, t) == -1) \
goto bail; \ goto bail; \
Py_CLEAR(s); \ Py_CLEAR(s); \
Py_CLEAR(t); \ Py_CLEAR(t); \
} while (0) } while (0)
marmouteUnsubmitted
Not Done

Should we explicitly check that no value other than the sidedata have been changed ?

marmoute: Should we explicitly check that no value other than the sidedata have been changed ?
AlphareAuthorUnsubmitted
Done

I changed it so the only arguments are sidedata length and offset

Alphare: I changed it so the only arguments are sidedata length and offset
if (self->added_length) if (self->added_length)
istat(new_length, "index entries added"); istat(new_length, "index entries added");
istat(length, "revs in memory"); istat(length, "revs in memory");
istat(ntlookups, "node trie lookups"); istat(ntlookups, "node trie lookups");
istat(ntmisses, "node trie misses"); istat(ntmisses, "node trie misses");
istat(ntrev, "node trie last rev scanned"); istat(ntrev, "node trie last rev scanned");
if (self->ntinitialized) { if (self->ntinitialized) {
{"has_node", (PyCFunction)index_m_has_node, METH_O, {"has_node", (PyCFunction)index_m_has_node, METH_O,
"return True if the node exist in the index"}, "return True if the node exist in the index"},
{"rev", (PyCFunction)index_m_rev, METH_O, {"rev", (PyCFunction)index_m_rev, METH_O,
"return `rev` associated with a node or raise RevlogError"}, "return `rev` associated with a node or raise RevlogError"},
{"computephasesmapsets", (PyCFunction)compute_phases_map_sets, METH_VARARGS, {"computephasesmapsets", (PyCFunction)compute_phases_map_sets, METH_VARARGS,
"compute phases"}, "compute phases"},
{"reachableroots2", (PyCFunction)reachableroots2, METH_VARARGS, {"reachableroots2", (PyCFunction)reachableroots2, METH_VARARGS,
"reachableroots"}, "reachableroots"},
{"replace_sidedata_info", (PyCFunction)index_replace_sidedata_info,
METH_VARARGS, "replace an existing index entry with a new value"},
{"headrevs", (PyCFunction)index_headrevs, METH_VARARGS, {"headrevs", (PyCFunction)index_headrevs, METH_VARARGS,
"get head revisions"}, /* Can do filtering since 3.2 */ "get head revisions"}, /* Can do filtering since 3.2 */
{"headrevsfiltered", (PyCFunction)index_headrevs, METH_VARARGS, {"headrevsfiltered", (PyCFunction)index_headrevs, METH_VARARGS,
"get filtered head revisions"}, /* Can always do filtering */ "get filtered head revisions"}, /* Can always do filtering */
{"issnapshot", (PyCFunction)index_issnapshot, METH_O, {"issnapshot", (PyCFunction)index_issnapshot, METH_O,
"True if the object is a snapshot"}, "True if the object is a snapshot"},
{"findsnapshots", (PyCFunction)index_findsnapshots, METH_VARARGS, {"findsnapshots", (PyCFunction)index_findsnapshots, METH_VARARGS,
"Gather snapshot data in a cache dict"}, "Gather snapshot data in a cache dict"},

mercurial/pure/parsers.py

index = self._calculate_index(i) index = self._calculate_index(i)
data = self._data[index : index + self.index_size] data = self._data[index : index + self.index_size]
r = _unpack(self.index_format, data) r = _unpack(self.index_format, data)
if self._lgt and i == 0: if self._lgt and i == 0:
r = (offset_type(0, gettype(r[0])),) + r[1:] r = (offset_type(0, gettype(r[0])),) + r[1:]
return r return r
class IndexObject(BaseIndexObject): class IndexObject(BaseIndexObject):
def __init__(self, data): def __init__(self, data):
assert len(data) % self.index_size == 0 assert len(data) % self.index_size == 0
self._data = data self._data = data
self._lgt = len(data) // self.index_size self._lgt = len(data) // self.index_size
marmouteUnsubmitted
Not Done

The changeset description state that we don't check the transaction thing in the python version. I assume that this is because we have some extra information in the C code that the python version don't have. Could we fix it ?

marmoute: The changeset description state that we don't check the transaction thing in the python version.
AlphareAuthorUnsubmitted
Done

I misunderstood the Python implementation, this is now fixed.

Alphare: I misunderstood the Python implementation, this is now fixed.
self._extra = [] self._extra = []
def _calculate_index(self, i): def _calculate_index(self, i):
return i * self.index_size return i * self.index_size
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)
marmouteUnsubmitted
Done

Same feedback as the C code, could we validate than we don't modify anything else than the side-data index ?

marmoute: Same feedback as the C code, could we validate than we don't modify anything else than the side…
self._stripnodes(i) self._stripnodes(i)
if i < self._lgt: if i < self._lgt:
self._data = self._data[: i * self.index_size] self._data = self._data[: i * self.index_size]
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]
# 8 bytes: sidedata offset # 8 bytes: sidedata offset
# 4 bytes: sidedata compressed length # 4 bytes: sidedata compressed length
# 20 bytes: Padding to align to 96 bytes (see RevlogV2Plan wiki page) # 20 bytes: Padding to align to 96 bytes (see RevlogV2Plan wiki page)
index_format = b">Qiiiiii20s12xQi20x" index_format = b">Qiiiiii20s12xQi20x"
index_size = struct.calcsize(index_format) index_size = struct.calcsize(index_format)
assert index_size == 96, index_size assert index_size == 96, index_size
null_item = (0, 0, 0, -1, -1, -1, -1, nullid, 0, 0) null_item = (0, 0, 0, -1, -1, -1, -1, nullid, 0, 0)
def replace_sidedata_info(self, i, sidedata_offset, sidedata_length):
"""
Replace an existing index entry's sidedata offset and length with new
ones.
This cannot be used outside of the context of sidedata rewriting,
inside the transaction that creates the revision `i`.
"""
if i < 0:
raise KeyError
self._check_index(i)
sidedata_format = b">Qi"
packed_size = struct.calcsize(sidedata_format)
if i >= self._lgt:
packed = _pack(sidedata_format, sidedata_offset, sidedata_length)
old = self._extra[i - self._lgt]
new = old[:64] + packed + old[64 + packed_size :]
self._extra[i - self._lgt] = new
else:
msg = b"cannot rewrite entries outside of this transaction"
raise KeyError(msg)
class IndexObject2(Index2Mixin, IndexObject): class IndexObject2(Index2Mixin, IndexObject):
pass pass
class InlinedIndexObject2(Index2Mixin, InlinedIndexObject): class InlinedIndexObject2(Index2Mixin, InlinedIndexObject):
def _inline_scan(self, lgt): def _inline_scan(self, lgt):
sidedata_length_pos = 72 sidedata_length_pos = 72