( )⚙ D11703 rust-nodemap: backed out mitigation for issue 6554

This is an archive of the discontinued Mercurial Phabricator instance.

Differential D11703

rust-nodemap: backed out mitigation for issue 6554
ClosedPublic

Authored by gracinet on Oct 19 2021, 6:46 PM.

Details

Reviewers
Alphare
Group Reviewers
hg-reviewers
Commits
rHGaa88fb60ecb4: rust-nodemap: backed out mitigation for issue 6554
Summary

This is a backout of changeset 3fffb48539ee.

Issue 6554 is now considered solved, hence its mitigation
has to be removed, if only for its performance cost.

Diff Detail

Repository
rHG Mercurial
Branch
default
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

gracinet created this revision.Oct 19 2021, 6:46 PM
Alphare accepted this revision.Oct 20 2021, 3:20 AM
Alphare added a subscriber: Alphare.

Thanks, just in time for 5.9.3 and 6.0, we forgot about it :)

This revision is now accepted and ready to land.Oct 20 2021, 3:20 AM
Alphare added a comment.Oct 20 2021, 3:21 AM

Note that this should have been on stable, but I think the rebase will go fine.

Alphare added a comment.Oct 20 2021, 5:34 AM

Not sure why phab hasn't picked up on it yet, but this has been queued.

gracinet added a commit: rHGaa88fb60ecb4: rust-nodemap: backed out mitigation for issue 6554.Oct 21 2021, 3:27 AM
Closed by commit rHGaa88fb60ecb4: rust-nodemap: backed out mitigation for issue 6554 (authored by gracinet). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 30923

rust/hg-cpython/src/revlog.rs

def get_cindex(&self) -> PyResult<PyObject> { def get_cindex(&self) -> PyResult<PyObject> {
Ok(self.cindex(py).borrow().inner().clone_ref(py)) Ok(self.cindex(py).borrow().inner().clone_ref(py))
} }
// Index API involving nodemap, as defined in mercurial/pure/parsers.py // Index API involving nodemap, as defined in mercurial/pure/parsers.py
/// Return Revision if found, raises a bare `error.RevlogError` /// Return Revision if found, raises a bare `error.RevlogError`
/// in case of ambiguity, same as C version does /// in case of ambiguity, same as C version does
def get_rev(&self, pynode: PyBytes) -> PyResult<Option<Revision>> { def get_rev(&self, node: PyBytes) -> PyResult<Option<Revision>> {
let opt = self.get_nodetree(py)?.borrow(); let opt = self.get_nodetree(py)?.borrow();
let nt = opt.as_ref().unwrap(); let nt = opt.as_ref().unwrap();
let idx = &*self.cindex(py).borrow(); let idx = &*self.cindex(py).borrow();
let node = node_from_py_bytes(py, &pynode)?; let node = node_from_py_bytes(py, &node)?;
match nt.find_bin(idx, node.into()) nt.find_bin(idx, node.into()).map_err(|e| nodemap_error(py, e))
{
Ok(None) =>
// fallback to C implementation, remove once
// https://bz.mercurial-scm.org/show_bug.cgi?id=6554
// is fixed (a simple backout should do)
self.call_cindex(py, "get_rev", &PyTuple::new(py, &[pynode.into_object()]), None)?
.extract(py),
Ok(Some(rev)) => Ok(Some(rev)),
Err(e) => Err(nodemap_error(py, e)),
}
} }
/// same as `get_rev()` but raises a bare `error.RevlogError` if node /// same as `get_rev()` but raises a bare `error.RevlogError` if node
/// is not found. /// is not found.
/// ///
/// No need to repeat `node` in the exception, `mercurial/revlog.py` /// No need to repeat `node` in the exception, `mercurial/revlog.py`
/// will catch and rewrap with it /// will catch and rewrap with it
def rev(&self, node: PyBytes) -> PyResult<Revision> { def rev(&self, node: PyBytes) -> PyResult<Revision> {
match nt.unique_prefix_len_node(idx, &node_from_py_bytes(py, &node)?) match nt.unique_prefix_len_node(idx, &node_from_py_bytes(py, &node)?)
{ {
Ok(Some(l)) => Ok(l), Ok(Some(l)) => Ok(l),
Ok(None) => Err(revlog_error(py)), Ok(None) => Err(revlog_error(py)),
Err(e) => Err(nodemap_error(py, e)), Err(e) => Err(nodemap_error(py, e)),
} }
} }
def partialmatch(&self, pynode: PyObject) -> PyResult<Option<PyBytes>> { def partialmatch(&self, node: PyObject) -> PyResult<Option<PyBytes>> {
let opt = self.get_nodetree(py)?.borrow(); let opt = self.get_nodetree(py)?.borrow();
let nt = opt.as_ref().unwrap(); let nt = opt.as_ref().unwrap();
let idx = &*self.cindex(py).borrow(); let idx = &*self.cindex(py).borrow();
let node_as_string = if cfg!(feature = "python3-sys") { let node_as_string = if cfg!(feature = "python3-sys") {
pynode.cast_as::<PyString>(py)?.to_string(py)?.to_string() node.cast_as::<PyString>(py)?.to_string(py)?.to_string()
} }
else { else {
let node = pynode.extract::<PyBytes>(py)?; let node = node.extract::<PyBytes>(py)?;
String::from_utf8_lossy(node.data(py)).to_string() String::from_utf8_lossy(node.data(py)).to_string()
}; };
let prefix = NodePrefix::from_hex(&node_as_string).map_err(|_| PyErr::new::<ValueError, _>(py, "Invalid node or prefix"))?; let prefix = NodePrefix::from_hex(&node_as_string).map_err(|_| PyErr::new::<ValueError, _>(py, "Invalid node or prefix"))?;
match nt.find_bin(idx, prefix) { nt.find_bin(idx, prefix)
Ok(None) => // TODO make an inner API returning the node directly
// fallback to C implementation, remove once .map(|opt| opt.map(
// https://bz.mercurial-scm.org/show_bug.cgi?id=6554 |rev| PyBytes::new(py, idx.node(rev).unwrap().as_bytes())))
// is fixed (a simple backout should do) .map_err(|e| nodemap_error(py, e))
self.call_cindex(
py, "partialmatch",
&PyTuple::new(py, &[pynode]), None
)?.extract(py),
Ok(Some(rev)) =>
Ok(Some(PyBytes::new(py, idx.node(rev).unwrap().as_bytes()))),
Err(e) => Err(nodemap_error(py, e)),
}
} }
/// append an index entry /// append an index entry
def append(&self, tup: PyTuple) -> PyResult<PyObject> { def append(&self, tup: PyTuple) -> PyResult<PyObject> {
if tup.len(py) < 8 { if tup.len(py) < 8 {
// this is better than the panic promised by tup.get_item() // this is better than the panic promised by tup.get_item()
return Err( return Err(
PyErr::new::<IndexError, _>(py, "tuple index out of range")) PyErr::new::<IndexError, _>(py, "tuple index out of range"))

tests/test-persistent-nodemap.t

$ hg debugupdatecache $ hg debugupdatecache
$ hg debugnodemap --metadata $ hg debugnodemap --metadata
uid: * (glob) uid: * (glob)
tip-rev: 5002 tip-rev: 5002
tip-node: b355ef8adce0949b8bdf6afc72ca853740d65944 tip-node: b355ef8adce0949b8bdf6afc72ca853740d65944
data-length: 121088 data-length: 121088
data-unused: 0 data-unused: 0
data-unused: 0.000% data-unused: 0.000%
Sub-case: fallback for corrupted data file
------------------------------------------
Sabotaging the data file so that nodemap resolutions fail, triggering fallback to
(non-persistent) C implementation.
$ UUID=`hg debugnodemap --metadata| grep 'uid:' | \
> sed 's/uid: //'`
$ FILE=.hg/store/00changelog-"${UUID}".nd
$ python -c "fobj = open('$FILE', 'r+b'); fobj.write(b'\xff' * 121088); fobj.close()"
The nodemap data file is still considered in sync with the docket. This
would fail without the fallback to the (non-persistent) C implementation:
$ hg log -r b355ef8adce0949b8bdf6afc72ca853740d65944 -T '{rev}\n' --traceback
5002
The nodemap data file hasn't been fixed, more tests can be inserted:
$ hg debugnodemap --dump-disk | f --bytes=256 --hexdump --size
size=121088
0000: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................|
0010: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................|
0020: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................|
0030: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................|
0040: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................|
0050: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................|
0060: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................|
0070: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................|
0080: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................|
0090: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................|
00a0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................|
00b0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................|
00c0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................|
00d0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................|
00e0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................|
00f0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................|
$ mv ../tmp-data-file $FILE $ mv ../tmp-data-file $FILE
$ mv ../tmp-docket .hg/store/00changelog.n $ mv ../tmp-docket .hg/store/00changelog.n
Check transaction related property Check transaction related property
================================== ==================================
An up to date nodemap should be available to shell hooks, An up to date nodemap should be available to shell hooks,