This is an archive of the discontinued Mercurial Phabricator instance.

Differential D12494

[RFC] rust-revlog: create nodemap on the fly if it's not persisted
Needs ReviewPublic

Authored by martinvonz on Apr 11 2022, 12:07 PM.

Details

Reviewers
None
Group Reviewers
hg-reviewers
Summary

We can create a nodemap in memory when there isn't a persistent
one. When listing all commits in the hg repo, it sped up the
equivalent of hg log -T '{node}\n' --hidden (but run with my hobby
project's hg backend) from 123 s to 2.55 s.

What do we actually want to do here? Calculate the map after we've
done a few O(n) node-to-rev requests? I think that's what the C
implementation does.

Diff Detail

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

Event Timeline

martinvonz created this revision.Apr 11 2022, 12:07 PM
Alphare added a subscriber: Alphare.Apr 12 2022, 4:51 AM

The C implementation indeed builds the nodemap after 3 misses. We will probably want to do the same thing here, however the Rust implementation will require more work because we don't want our getter methods to all take &mut self.

For now, this looks good. Small revlogs will not be affected and large operations on the changelog/manifest call for a persistent nodemap anyway, so why not. If we end up needing to do something more clever down the line (like I suspect we will, given revlogv2 at least), we will think about how to organize our types to encode that API in a clear fashion... or just use interior mutability for the nodemap.

rust/hg-core/src/revlog/revlog.rs
128

.expect("node should exist in index")

marmoute added a subscriber: marmoute.EditedApr 12 2022, 5:45 AM

Note that this should not be a practical issue right now, as rhg is not accessing multiple revision yet and hg use an intermediate layer that use the C caching if persistent nodemap is missing.
So this is not a problem for mercurial as is. Improving this in general is still a good idea.

martinvonz added a comment.Apr 12 2022, 8:34 AM
In D12494#191335, @marmoute wrote:

So this is not a problem for mercurial as is. Improving this in general is still a good idea.

As mentioned in the commit message, it was when using my hobby project's hg backend. That's https://github.com/martinvonz/jj/commit/be1fd35afe098db0d44e31499cfccf2a64ab217d

martinvonz updated this revision to Diff 33191.Apr 16 2022, 1:47 AM
martinvonz updated this revision to Diff 33396.Mon, May 9, 3:13 PM

Revision Contents
Changeset List

PathPackages
Mrust/hg-core/src/revlog/revlog.rs (27 lines)

Diff 33005

rust/hg-core/src/revlog/revlog.rs

use super::node::{NodePrefix, NODE_BYTES_LENGTH, NULL_NODE}; use super::node::{NodePrefix, NODE_BYTES_LENGTH, NULL_NODE};
use super::nodemap; use super::nodemap;
use super::nodemap::{NodeMap, NodeMapError}; use super::nodemap::{NodeMap, NodeMapError};
use super::nodemap_docket::NodeMapDocket; use super::nodemap_docket::NodeMapDocket;
use super::patch; use super::patch;
use crate::errors::HgError; use crate::errors::HgError;
use crate::repo::Repo; use crate::repo::Repo;
use crate::revlog::Revision; use crate::revlog::Revision;
use crate::{Node, NULL_REVISION}; use crate::{Node, RevlogIndex, NULL_REVISION};
const REVISION_FLAG_CENSORED: u16 = 1 << 15; const REVISION_FLAG_CENSORED: u16 = 1 << 15;
const REVISION_FLAG_ELLIPSIS: u16 = 1 << 14; const REVISION_FLAG_ELLIPSIS: u16 = 1 << 14;
const REVISION_FLAG_EXTSTORED: u16 = 1 << 13; const REVISION_FLAG_EXTSTORED: u16 = 1 << 13;
const REVISION_FLAG_HASCOPIESINFO: u16 = 1 << 12; const REVISION_FLAG_HASCOPIESINFO: u16 = 1 << 12;
// Keep this in sync with REVIDX_KNOWN_FLAGS in // Keep this in sync with REVIDX_KNOWN_FLAGS in
// mercurial/revlogutils/flagutil.py // mercurial/revlogutils/flagutil.py
let data_path = data_path.unwrap_or(&default_data_path); let data_path = data_path.unwrap_or(&default_data_path);
let data_mmap = repo.store_vfs().mmap_open(data_path)?; let data_mmap = repo.store_vfs().mmap_open(data_path)?;
Some(Box::new(data_mmap)) Some(Box::new(data_mmap))
}; };
let nodemap = if index.is_inline() { let nodemap = if index.is_inline() {
None None
} else { } else {
NodeMapDocket::read_from_file(repo, index_path)?.map( if let Some((docket, data)) =
|(docket, data)| { NodeMapDocket::read_from_file(repo, index_path)?
nodemap::NodeTree::load_bytes( {
Some(nodemap::NodeTree::load_bytes(
Box::new(data), Box::new(data),
docket.data_length, docket.data_length,
) ))
}, } else {
) let mut nodemap = nodemap::NodeTree::default();
for rev in 0..index.len() {
let rev = rev as Revision;
nodemap
.insert(&index, index.node(rev).unwrap(), rev)
.unwrap();
AlphareUnsubmitted
Not Done

.expect("node should exist in index")

Alphare: `.expect("node should exist in index")`
}
Some(nodemap)
}
}; };
Ok(Revlog { Ok(Revlog {
index, index,
data_bytes, data_bytes,
nodemap, nodemap,
}) })
} }