( )⚙ D8861 hg-core: make parse_dirstate return references rather than update hashmaps

This is an archive of the discontinued Mercurial Phabricator instance.

Differential D8861

hg-core: make parse_dirstate return references rather than update hashmaps
ClosedPublic

Authored by acezar on Aug 3 2020, 10:25 AM.

Details

Reviewers
Alphare
indygreg
Group Reviewers
hg-reviewers
Commits
rHG27424779c5b8: hg-core: make parse_dirstate return references rather than update hashmaps
Summary

Returing a vec is faster than updating a hashmap when the hashmap is not needed
like in hg files which just list tracked files.

Returning references avoid copying data when not needed improving performence
for large repositories.

Diff Detail

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

Event Timeline

acezar created this revision.Aug 3 2020, 10:25 AM
Alphare accepted this revision.Aug 3 2020, 10:59 AM
Alphare added a subscriber: Alphare.
Alphare added inline comments.
rust/hg-cpython/src/parsers.rs
34

These should be a collect() instead of default + extend.

acezar updated this revision to Diff 22245.Aug 4 2020, 4:57 AM
acezar updated this revision to Diff 22267.Aug 5 2020, 4:58 AM
indygreg accepted this revision.Aug 8 2020, 3:43 PM
This revision is now accepted and ready to land.Aug 8 2020, 3:43 PM

Revision Contents
Changeset List

Diff 22245

rust/hg-core/src/dirstate/dirstate_map.rs

pub fn read( pub fn read(
&mut self, &mut self,
file_contents: &[u8], file_contents: &[u8],
) -> Result<Option<DirstateParents>, DirstateError> { ) -> Result<Option<DirstateParents>, DirstateError> {
if file_contents.is_empty() { if file_contents.is_empty() {
return Ok(None); return Ok(None);
} }
let parents = parse_dirstate( let (parents, entries, copies) = parse_dirstate(file_contents)?;
&mut self.state_map, self.state_map.extend(
&mut self.copy_map, entries
file_contents, .into_iter()
)?; .map(|(path, entry)| (path.to_owned(), entry)),
);
self.copy_map.extend(
copies
.into_iter()
.map(|(path, copy)| (path.to_owned(), copy.to_owned())),
);
if !self.dirty_parents { if !self.dirty_parents {
self.set_parents(&parents); self.set_parents(&parents);
} }
Ok(Some(parents)) Ok(Some(parents))
} }

rust/hg-core/src/dirstate/parsers.rs

use std::io::Cursor; use std::io::Cursor;
use std::time::Duration; use std::time::Duration;
/// Parents are stored in the dirstate as byte hashes. /// Parents are stored in the dirstate as byte hashes.
pub const PARENT_SIZE: usize = 20; pub const PARENT_SIZE: usize = 20;
/// Dirstate entries have a static part of 8 + 32 + 32 + 32 + 32 bits. /// Dirstate entries have a static part of 8 + 32 + 32 + 32 + 32 bits.
const MIN_ENTRY_SIZE: usize = 17; const MIN_ENTRY_SIZE: usize = 17;
// TODO parse/pack: is mutate-on-loop better for performance? type ParseResult<'a> = (
DirstateParents,
Vec<(&'a HgPath, DirstateEntry)>,
Vec<(&'a HgPath, &'a HgPath)>,
);
#[timed] #[timed]
pub fn parse_dirstate( pub fn parse_dirstate(
state_map: &mut StateMap,
copy_map: &mut CopyMap,
contents: &[u8], contents: &[u8],
) -> Result<DirstateParents, DirstateParseError> { ) -> Result<ParseResult, DirstateParseError> {
if contents.len() < PARENT_SIZE * 2 { if contents.len() < PARENT_SIZE * 2 {
return Err(DirstateParseError::TooLittleData); return Err(DirstateParseError::TooLittleData);
} }
let mut copies = vec![];
let mut entries = vec![];
let mut curr_pos = PARENT_SIZE * 2; let mut curr_pos = PARENT_SIZE * 2;
let parents = DirstateParents { let parents = DirstateParents {
p1: contents[..PARENT_SIZE].try_into().unwrap(), p1: contents[..PARENT_SIZE].try_into().unwrap(),
p2: contents[PARENT_SIZE..curr_pos].try_into().unwrap(), p2: contents[PARENT_SIZE..curr_pos].try_into().unwrap(),
}; };
while curr_pos < contents.len() { while curr_pos < contents.len() {
let path = &entry_bytes[MIN_ENTRY_SIZE..MIN_ENTRY_SIZE + (path_len)]; let path = &entry_bytes[MIN_ENTRY_SIZE..MIN_ENTRY_SIZE + (path_len)];
let (path, copy) = match memchr::memchr(0, path) { let (path, copy) = match memchr::memchr(0, path) {
None => (path, None), None => (path, None),
Some(i) => (&path[..i], Some(&path[(i + 1)..])), Some(i) => (&path[..i], Some(&path[(i + 1)..])),
}; };
if let Some(copy_path) = copy { if let Some(copy_path) = copy {
copy_map.insert( copies.push((HgPath::new(path), HgPath::new(copy_path)));
HgPath::new(path).to_owned(),
HgPath::new(copy_path).to_owned(),
);
}; };
state_map.insert( entries.push((
HgPath::new(path).to_owned(), HgPath::new(path),
DirstateEntry { DirstateEntry {
state, state,
mode, mode,
size, size,
mtime, mtime,
}, },
); ));
curr_pos = curr_pos + MIN_ENTRY_SIZE + (path_len); curr_pos = curr_pos + MIN_ENTRY_SIZE + (path_len);
} }
Ok(parents) Ok((parents, entries, copies))
} }
/// `now` is the duration in seconds since the Unix epoch /// `now` is the duration in seconds since the Unix epoch
pub fn pack_dirstate( pub fn pack_dirstate(
state_map: &mut StateMap, state_map: &mut StateMap,
copy_map: &CopyMap, copy_map: &CopyMap,
parents: DirstateParents, parents: DirstateParents,
now: Duration, now: Duration,
p1: *b"12345678910111213141", p1: *b"12345678910111213141",
p2: *b"00000000000000000000", p2: *b"00000000000000000000",
}; };
let now = Duration::new(15000000, 0); let now = Duration::new(15000000, 0);
let result = let result =
pack_dirstate(&mut state_map, &copymap, parents.clone(), now) pack_dirstate(&mut state_map, &copymap, parents.clone(), now)
.unwrap(); .unwrap();
let mut new_state_map: StateMap = FastHashMap::default(); let (new_parents, entries, copies) =
let mut new_copy_map: CopyMap = FastHashMap::default(); parse_dirstate(result.as_slice()).unwrap();
let new_parents = parse_dirstate( let new_state_map: StateMap = entries
&mut new_state_map, .into_iter()
&mut new_copy_map, .map(|(path, entry)| (path.to_owned(), entry))
result.as_slice(), .collect();
) let new_copy_map: CopyMap = copies
.unwrap(); .into_iter()
.map(|(path, copy)| (path.to_owned(), copy.to_owned()))
.collect();
assert_eq!( assert_eq!(
(parents, state_map, copymap), (parents, state_map, copymap),
(new_parents, new_state_map, new_copy_map) (new_parents, new_state_map, new_copy_map)
) )
} }
#[test] #[test]
fn test_parse_pack_multiple_entries_with_copy() { fn test_parse_pack_multiple_entries_with_copy() {
p1: *b"12345678910111213141", p1: *b"12345678910111213141",
p2: *b"00000000000000000000", p2: *b"00000000000000000000",
}; };
let now = Duration::new(15000000, 0); let now = Duration::new(15000000, 0);
let result = let result =
pack_dirstate(&mut state_map, &copymap, parents.clone(), now) pack_dirstate(&mut state_map, &copymap, parents.clone(), now)
.unwrap(); .unwrap();
let mut new_state_map: StateMap = FastHashMap::default(); let (new_parents, entries, copies) =
let mut new_copy_map: CopyMap = FastHashMap::default(); parse_dirstate(result.as_slice()).unwrap();
let new_parents = parse_dirstate( let new_state_map: StateMap = entries
&mut new_state_map, .into_iter()
&mut new_copy_map, .map(|(path, entry)| (path.to_owned(), entry))
result.as_slice(), .collect();
) let new_copy_map: CopyMap = copies
.unwrap(); .into_iter()
.map(|(path, copy)| (path.to_owned(), copy.to_owned()))
.collect();
assert_eq!( assert_eq!(
(parents, state_map, copymap), (parents, state_map, copymap),
(new_parents, new_state_map, new_copy_map) (new_parents, new_state_map, new_copy_map)
) )
} }
#[test] #[test]
/// https://www.mercurial-scm.org/repo/hg/rev/af3f26b6bba4 /// https://www.mercurial-scm.org/repo/hg/rev/af3f26b6bba4
p1: *b"12345678910111213141", p1: *b"12345678910111213141",
p2: *b"00000000000000000000", p2: *b"00000000000000000000",
}; };
let now = Duration::new(15000000, 0); let now = Duration::new(15000000, 0);
let result = let result =
pack_dirstate(&mut state_map, &copymap, parents.clone(), now) pack_dirstate(&mut state_map, &copymap, parents.clone(), now)
.unwrap(); .unwrap();
let mut new_state_map: StateMap = FastHashMap::default(); let (new_parents, entries, copies) =
let mut new_copy_map: CopyMap = FastHashMap::default(); parse_dirstate(result.as_slice()).unwrap();
let new_parents = parse_dirstate( let new_state_map: StateMap = entries
&mut new_state_map, .into_iter()
&mut new_copy_map, .map(|(path, entry)| (path.to_owned(), entry))
result.as_slice(), .collect();
) let new_copy_map: CopyMap = FastHashMap::default();
.unwrap(); copies
.into_iter()
.map(|(path, copy)| (path.to_owned(), copy.to_owned()))
.collect();
assert_eq!( assert_eq!(
( (
parents, parents,
[( [(
HgPathBuf::from_bytes(b"f1"), HgPathBuf::from_bytes(b"f1"),
DirstateEntry { DirstateEntry {
state: EntryState::Normal, state: EntryState::Normal,

rust/hg-cpython/src/parsers.rs

// parsers.rs // parsers.rs
// //
// Copyright 2019 Raphaël Gomès <rgomes@octobus.net> // Copyright 2019 Raphaël Gomès <rgomes@octobus.net>
// //
// This software may be used and distributed according to the terms of the // This software may be used and distributed according to the terms of the
// GNU General Public License version 2 or any later version. // GNU General Public License version 2 or any later version.
//! Bindings for the `hg::dirstate::parsers` module provided by the //! Bindings for the `hg::dirstate::parsers` module provided by the
//! `hg-core` package. //! `hg-core` package.
//! //!
//! From Python, this will be seen as `mercurial.rustext.parsers` //! From Python, this will be seen as `mercurial.rustext.parsers`
use cpython::{ use cpython::{
exc, PyBytes, PyDict, PyErr, PyInt, PyModule, PyResult, PyTuple, Python, exc, PyBytes, PyDict, PyErr, PyInt, PyModule, PyResult, PyTuple, Python,
PythonObject, ToPyObject, PythonObject, ToPyObject,
}; };
use hg::{ use hg::{
pack_dirstate, parse_dirstate, utils::hg_path::HgPathBuf, pack_dirstate, parse_dirstate, utils::hg_path::HgPathBuf, DirstateEntry,
DirstatePackError, DirstateParents, DirstateParseError, FastHashMap, DirstatePackError, DirstateParents, DirstateParseError, FastHashMap,
PARENT_SIZE, PARENT_SIZE,
}; };
use std::convert::TryInto; use std::convert::TryInto;
use crate::dirstate::{extract_dirstate, make_dirstate_tuple}; use crate::dirstate::{extract_dirstate, make_dirstate_tuple};
use std::time::Duration; use std::time::Duration;
fn parse_dirstate_wrapper( fn parse_dirstate_wrapper(
py: Python, py: Python,
dmap: PyDict, dmap: PyDict,
copymap: PyDict, copymap: PyDict,
st: PyBytes, st: PyBytes,
) -> PyResult<PyTuple> { ) -> PyResult<PyTuple> {
let mut dirstate_map = FastHashMap::default(); match parse_dirstate(st.data(py)) {
let mut copies = FastHashMap::default(); Ok((parents, entries, copies)) => {
let dirstate_map: FastHashMap<HgPathBuf, DirstateEntry> = entries
AlphareUnsubmitted
Not Done

These should be a collect() instead of default + extend.

Alphare: These should be a `collect()` instead of `default` + `extend`.
.into_iter()
.map(|(path, entry)| (path.to_owned(), entry))
.collect();
let copy_map: FastHashMap<HgPathBuf, HgPathBuf> = copies
.into_iter()
.map(|(path, copy)| (path.to_owned(), copy.to_owned()))
.collect();
match parse_dirstate(&mut dirstate_map, &mut copies, st.data(py)) {
Ok(parents) => {
for (filename, entry) in &dirstate_map { for (filename, entry) in &dirstate_map {
dmap.set_item( dmap.set_item(
py, py,
PyBytes::new(py, filename.as_bytes()), PyBytes::new(py, filename.as_bytes()),
make_dirstate_tuple(py, entry)?, make_dirstate_tuple(py, entry)?,
)?; )?;
} }
for (path, copy_path) in copies { for (path, copy_path) in copy_map {
copymap.set_item( copymap.set_item(
py, py,
PyBytes::new(py, path.as_bytes()), PyBytes::new(py, path.as_bytes()),
PyBytes::new(py, copy_path.as_bytes()), PyBytes::new(py, copy_path.as_bytes()),
)?; )?;
} }
Ok( Ok(
(PyBytes::new(py, &parents.p1), PyBytes::new(py, &parents.p2)) (PyBytes::new(py, &parents.p1), PyBytes::new(py, &parents.p2))