This is an archive of the discontinued Mercurial Phabricator instance.

rust-nodemap: pure Rust example
ClosedPublic

Authored by gracinet on Jan 6 2020, 2:26 PM.

Details

Summary

To run, use cargo run --release --example nodemap

This demonstrates that simple scenarios entirely written
in Rust can content themselves with NodeTree<T>.

The example mmaps both the nodemap file and the changelog index.
We had of course to include an implementation of RevlogIndex
directly, which isn't much at this stage. It felt a bit
prematurate to include it in the lib.

Here are some first performance measurements, obtained with
this example, on a clone of mozilla-central with 440000
changesets:

(create) Nodemap constructed in RAM in 153.638305ms
(query CAE63161B68962) found in 22.362us: Ok(Some(269489))
(bench) Did 3 queries in 36.418µs (mean 12.139µs)
(bench) Did 50 queries in 184.318µs (mean 3.686µs)
(bench) Did 100000 queries in 31.053461ms (mean 310ns)

To be fair, even between bench runs, results tend to depend whether
the file is still in kernel caches, and it's not so easy to
get back to a real cold start. The worst we've seen was in the
50us ballpark.

In any busy server setting, the pages would always be in RAM.

We hope it's good enough not to be significantly slower on any
concrete Mercurial operation than the C nodetree when fully in RAM,
and of course this implementation has the serious headstart advantage
of persistence.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

gracinet created this revision.Jan 6 2020, 2:26 PM
gracinet edited the summary of this revision. (Show Details)Jan 27 2020, 10:50 AM
gracinet updated this revision to Diff 19637.
marmoute accepted this revision.Feb 7 2020, 2:46 PM
marmoute added a subscriber: marmoute.

This example looks good to me.

marmoute updated this revision to Diff 20026.Feb 8 2020, 12:45 PM
marmoute updated this revision to Diff 20166.Feb 11 2020, 7:03 AM
kevincox accepted this revision.Feb 12 2020, 10:54 AM
kevincox added inline comments.
rust/hg-core/examples/nodemap/index.rs
7

Since this is a file-level comment it should use //! https://doc.rust-lang.org/reference/comments.html#examples

48

I would make this an early return and remove the else. It seems to me like an unexpected condition. If it should never happen then please add a debug_assert as well.

77

Could you add at least a debug_assert that the alignment is correct?

85

Please put the unsafe block around only the unsafe operation. This makes it more obvious what I should look at.

let mmap = unsafe { MmapOptions::new().map(&file) }.unwrap();
85

It seems like we should handle this error. At the very least we should be providing context such as "your index file {:?} is missing".

rust/hg-core/examples/nodemap/main.rs
9

We should be using Rust 2018 so externs aren't required.

42

Do you create this at the start to avoid the work if you don't have permission? If not it seems to be that it would be better to avoid creating the file if creating the nodemap fails. Otherwise we are leaving an empty file around. If so please document your reasoning.

Alphare added inline comments.
rust/hg-core/examples/nodemap/index.rs
48

This is expected to happen, as it is the current interface. Whether to change it is a bigger undertaking. I'm not sure an early return is much better in that case.

77

I'm not sure of the point of doing so with an mmap. We have to trust what the mmap is giving us.

rust/hg-core/examples/nodemap/main.rs
42

As of right now, it's Python code that is responsible for the file IO of this persistent nodemap. The create subcommand is there as scaffolding to provide a file. If that answers your question, I'll add a comment saying so.

Alphare updated this revision to Diff 20216.Feb 14 2020, 6:02 AM
kevincox accepted this revision.Feb 14 2020, 6:55 AM
kevincox added inline comments.
rust/hg-core/examples/nodemap/index.rs
77

In general an assertion is better than a comment because it expresses the things that you are assuming in code. I get that "it should always be" which is why I didn't suggest an assert but I think it is still worth codifying the assumption with a debug_assert.

Alphare added inline comments.Feb 18 2020, 8:46 AM
rust/hg-core/examples/nodemap/index.rs
77

I am not sure exactly what the debug_assert should be? Do you want to check that the len is a multiple of the align_of::<IndexEntry>?

kevincox added inline comments.Feb 18 2020, 9:47 AM
rust/hg-core/examples/nodemap/index.rs
77

Yes, that seems reasonable. It shows what data you are ignoring and will alert us if we manage to hit that case in tests.

Alphare updated this revision to Diff 20277.Feb 24 2020, 5:13 AM
This revision was not accepted when it landed; it landed in state Needs Review.
This revision was automatically updated to reflect the committed changes.