Skip to content

object_id: back id types by Box<[u8]> instead of Vec<u8> #9930

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
martinvonz wants to merge 1 commit into
base: main
Choose a base branch
from
Open

object_id: back id types by Box<[u8]> instead of Vec<u8> #9930

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions cli/src/commit_ref_list.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -313,9 +313,9 @@ mod tests {
Arc::new(backend::Commit {
parents: vec![],
predecessors: vec![],
root_tree: Merge::resolved(TreeId::new(vec![])),
root_tree: Merge::resolved(TreeId::from_vec(vec![])),
conflict_labels: Merge::resolved(String::new()),
change_id: ChangeId::new(vec![]),
change_id: ChangeId::from_vec(vec![]),
description: String::new(),
author,
committer,
Expand All @@ -335,7 +335,7 @@ mod tests {
}

fn commit_id_generator() -> impl FnMut() -> CommitId {
let mut iter = (1_u128..).map(|n| CommitId::new(n.to_le_bytes().into()));
let mut iter = (1_u128..).map(|n| CommitId::from_vec(n.to_le_bytes().into()));
move || iter.next().unwrap()
}

Expand Down
4 changes: 2 additions & 2 deletions lib/src/backend.rs
View file
Open in desktop

@PhilipMetzger PhilipMetzger Aug 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

optional: It still may be useful to provide some to_vec(), from_vec() functions so callers can migrate.

@martinvonz martinvonz Aug 6, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new() function still takes a Vec<u8> and to_bytes() still returns Vec<u8>, so that should be fine. It's actually the Box<[u8]> versions that are missing. We may want to add those and see if we can avoid conversions to/from Vec<u8> in some places.

@yuja yuja Aug 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's actually the Box<[u8]> versions that are missing. We may want to add those and see if we can avoid conversions to/from Vec<u8> in some places.

Yes, we should probably rename new(Vec<u8>) to from_vec(Vec<u8>).

I have no idea if saving 8 bytes matters, but I'm not against it. If we decide to inline up to 32 or 64 bytes, there's room for a capacity field.

@martinvonz martinvonz Aug 7, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no idea if saving 8 bytes matters, but I'm not against it.

I don't know if it matters either, and I don't feel strongly either way.

If we decide to inline up to 32 or 64 bytes, there's room for a capacity field.

Do you mean using something like SmallVec<[u8; 32]>? I suppose that's another option. I don't know how to decide if that's better without someone spending time doing some profiling.

Yes, we should probably rename new(Vec<u8>) to from_vec(Vec<u8>).

Done.

Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ id_type!(
impl ChangeId {
/// Parses the given "reverse" hex string into a `ChangeId`.
pub fn try_from_reverse_hex(hex: impl AsRef<[u8]>) -> Option<Self> {
hex_util::decode_reverse_hex(hex).map(Self)
hex_util::decode_reverse_hex(hex).map(Self::from_vec)
}

/// Returns the hex string representation of this ID, which uses `z-k`
Expand All @@ -89,7 +89,7 @@ impl CopyId {
/// id yet.
// TODO: Delete this
pub fn placeholder() -> Self {
Self::new(vec![])
Self::from_vec(vec![])
}
}

Expand Down
6 changes: 6 additions & 0 deletions lib/src/content_hash.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,12 @@ impl<T: ContentHash> ContentHash for Vec<T> {
}
}

impl<T: ContentHash + ?Sized> ContentHash for Box<T> {
fn hash(&self, state: &mut impl DigestUpdate) {
(**self).hash(state);
}
}

impl ContentHash for str {
fn hash(&self, state: &mut impl DigestUpdate) {
self.as_bytes().hash(state);
Expand Down
4 changes: 2 additions & 2 deletions lib/src/default_index/bit_set.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,13 +211,13 @@ mod tests {

/// Generator of unique 16-byte CommitId excluding root id
fn commit_id_generator() -> impl FnMut() -> CommitId {
let mut iter = (1_u128..).map(|n| CommitId::new(n.to_le_bytes().into()));
let mut iter = (1_u128..).map(|n| CommitId::from_vec(n.to_le_bytes().into()));
move || iter.next().unwrap()
}

/// Generator of unique 16-byte ChangeId excluding root id
fn change_id_generator() -> impl FnMut() -> ChangeId {
let mut iter = (1_u128..).map(|n| ChangeId::new(n.to_le_bytes().into()));
let mut iter = (1_u128..).map(|n| ChangeId::from_vec(n.to_le_bytes().into()));
move || iter.next().unwrap()
}

Expand Down
4 changes: 2 additions & 2 deletions lib/src/default_index/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,13 +89,13 @@ mod tests {

/// Generator of unique 16-byte CommitId excluding root id
fn commit_id_generator() -> impl FnMut() -> CommitId {
let mut iter = (1_u128..).map(|n| CommitId::new(n.to_le_bytes().into()));
let mut iter = (1_u128..).map(|n| CommitId::from_vec(n.to_le_bytes().into()));
move || iter.next().unwrap()
}

/// Generator of unique 16-byte ChangeId excluding root id
fn change_id_generator() -> impl FnMut() -> ChangeId {
let mut iter = (1_u128..).map(|n| ChangeId::new(n.to_le_bytes().into()));
let mut iter = (1_u128..).map(|n| ChangeId::from_vec(n.to_le_bytes().into()));
move || iter.next().unwrap()
}

Expand Down
2 changes: 1 addition & 1 deletion lib/src/default_index/rev_walk.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -704,7 +704,7 @@ mod tests {

/// Generator of unique 16-byte ChangeId excluding root id
fn change_id_generator() -> impl FnMut() -> ChangeId {
let mut iter = (1_u128..).map(|n| ChangeId::new(n.to_le_bytes().into()));
let mut iter = (1_u128..).map(|n| ChangeId::from_vec(n.to_le_bytes().into()));
move || iter.next().unwrap()
}

Expand Down
2 changes: 1 addition & 1 deletion lib/src/default_index/revset_engine.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1593,7 +1593,7 @@ mod tests {

/// Generator of unique 16-byte ChangeId excluding root id
fn change_id_generator() -> impl FnMut() -> ChangeId {
let mut iter = (1_u128..).map(|n| ChangeId::new(n.to_le_bytes().into()));
let mut iter = (1_u128..).map(|n| ChangeId::from_vec(n.to_le_bytes().into()));
move || iter.next().unwrap()
}

Expand Down
4 changes: 2 additions & 2 deletions lib/src/default_index/store.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,14 +192,14 @@ impl DefaultIndexStore {
.map_err(|err| io::Error::new(io::ErrorKind::InvalidData, err))
.context(&op_link_file)
.map_err(DefaultIndexStoreError::LoadAssociation)?;
let commit_segment_id = CommitIndexSegmentId::new(proto.commit_segment_id);
let commit_segment_id = CommitIndexSegmentId::from_vec(proto.commit_segment_id);
let changed_path_start_commit_pos = proto
.changed_path_start_commit_pos
.map(GlobalCommitPosition);
let changed_path_segment_ids = proto
.changed_path_segment_ids
.into_iter()
.map(ChangedPathIndexSegmentId::new)
.map(ChangedPathIndexSegmentId::from_vec)
.collect_vec();

let commits = ReadonlyCommitIndexSegment::load(
Expand Down
12 changes: 6 additions & 6 deletions lib/src/git_backend.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -729,7 +729,7 @@ pub fn synthetic_change_id_from_git_commit_id(id: &CommitId) -> ChangeId {
.rev()
.map(|b| b.reverse_bits())
.collect();
ChangeId::new(bytes)
ChangeId::from_vec(bytes)
}

const EMPTY_STRING_PLACEHOLDER: &str = "JJ_EMPTY_STRING";
Expand Down Expand Up @@ -798,7 +798,7 @@ fn serialize_extras(commit: &Commit) -> Vec<u8> {
fn deserialize_extras(commit: &mut Commit, bytes: &[u8]) {
let proto = crate::protos::git_store::Commit::decode(bytes).unwrap();
if !proto.change_id.is_empty() {
commit.change_id = ChangeId::new(proto.change_id);
commit.change_id = ChangeId::from_vec(proto.change_id);
}
if commit.root_tree.is_resolved()
&& proto.uses_tree_conflict_format
Expand Down Expand Up @@ -1074,7 +1074,7 @@ impl Backend for GitBackend {
contents.read_to_end(&mut bytes).await.unwrap();

let oid = self.write_blob(&bytes, "file")?;
Ok(FileId::new(oid.as_bytes().to_vec()))
Ok(FileId::from_vec(oid.as_bytes().to_vec()))
}

async fn read_symlink(&self, _path: &RepoPath, id: &SymlinkId) -> BackendResult<String> {
Expand All @@ -1092,7 +1092,7 @@ impl Backend for GitBackend {

async fn write_symlink(&self, _path: &RepoPath, target: &str) -> BackendResult<SymlinkId> {
let oid = self.write_blob(target.as_bytes(), "symlink")?;
Ok(SymlinkId::new(oid.as_bytes().to_vec()))
Ok(SymlinkId::from_vec(oid.as_bytes().to_vec()))
}

async fn read_copy(&self, _id: &CopyId) -> BackendResult<CopyHistory> {
Expand Down Expand Up @@ -2296,7 +2296,7 @@ mod tests {
predecessors: vec![],
root_tree: Merge::resolved(backend.empty_tree_id().clone()),
conflict_labels: Merge::resolved(String::new()),
change_id: ChangeId::new(vec![42; 16]),
change_id: ChangeId::from_vec(vec![42; 16]),
description: "initial".to_string(),
author: signature.clone(),
committer: signature,
Expand Down Expand Up @@ -2469,7 +2469,7 @@ mod tests {
predecessors: vec![],
root_tree: Merge::resolved(backend.empty_tree_id().clone()),
conflict_labels: Merge::resolved(String::new()),
change_id: ChangeId::new(vec![42; 16]),
change_id: ChangeId::from_vec(vec![42; 16]),
description: "initial".to_string(),
author: create_signature(),
committer: create_signature(),
Expand Down
6 changes: 3 additions & 3 deletions lib/src/local_working_copy.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1154,13 +1154,13 @@ impl TreeState {
if proto.tree_ids.is_empty() {
self.tree = MergedTree::resolved(
self.store.clone(),
TreeId::new(proto.legacy_tree_id.clone()),
TreeId::from_vec(proto.legacy_tree_id.clone()),
);
} else {
let tree_ids_builder: MergeBuilder<TreeId> = proto
.tree_ids
.iter()
.map(|id| TreeId::new(id.clone()))
.map(|id| TreeId::from_vec(id.clone()))
.collect();
self.tree = MergedTree::new(
self.store.clone(),
Expand Down Expand Up @@ -2575,7 +2575,7 @@ impl CheckoutState {
let proto = crate::protos::local_working_copy::Checkout::decode(&*buf)
.map_err(|err| wrap_err(err.into()))?;
Ok(Self {
operation_id: OperationId::new(proto.operation_id),
operation_id: OperationId::from_vec(proto.operation_id),
workspace_name: if proto.workspace_name.is_empty() {
// For compatibility with old working copies.
// TODO: Delete in mid 2022 or so
Expand Down
18 changes: 9 additions & 9 deletions lib/src/object_id.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,9 @@ pub trait ObjectId {
}

// Defines a new struct type with visibility `vis` and name `ident` containing
// a single Vec<u8> used to store an identifier (typically the output of a hash
// function) as bytes. Types defined using this macro automatically implement
// the `ObjectId` and `ContentHash` traits.
// a single Box<[u8]> used to store an identifier (typically the output of a
// hash function) as bytes. Types defined using this macro automatically
// implement the `ObjectId` and `ContentHash` traits.
// Documentation comments written inside the macro definition will be captured
// and associated with the type defined by the macro.
//
Expand All @@ -46,7 +46,7 @@ macro_rules! id_type {
) => {
$(#[$attr])*
#[derive($crate::content_hash::ContentHash, PartialEq, Eq, PartialOrd, Ord, Clone, Hash)]
$vis struct $name(Vec<u8>);
$vis struct $name(Box<[u8]>);

@scott2000 scott2000 Aug 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might also be worth considering Arc<[u8]>, since IDs are cloned fairly often. I'm not sure whether the overhead of cloning is higher than the overhead of an atomic reference count though.

@martinvonz martinvonz Aug 7, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I think my answer is the same as above: it's hard to know without someone doing some profiling.

@yuja yuja Aug 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can tell, the allocation cost isn't significant. What matters more is the cache locality of things like Vec<TreeValue>.

@josephlou5 josephlou5 Aug 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am by no means a Rust expert, but I just happened to come across a YouTube video called "Use Arc instead of Vec" by Logan Smith (no link to avoid coming across as spam :p), and in it he provides arguments for always preferring Arc<[T]> over Vec<T> if you need Clone, and preferring Box<[T]> if you don't need Clone. Since Scott said that IDs are cloned a lot, maybe Arc<[T]> would be slightly faster, though presumably it's fairly cheap to copy in a Box<[T]> anyway. The video also did not mention the overhead cost of the atomic count, and I would not know any better either. Food for thought 🙃

$crate::object_id::impl_id_type!($name, $hex_method);
};
}
Expand All @@ -56,13 +56,13 @@ macro_rules! impl_id_type {
#[allow(dead_code)]
impl $name {
/// Creates a new instance of this id type from the given bytes.
pub fn new(value: Vec<u8>) -> Self {
Self(value)
pub fn from_vec(value: Vec<u8>) -> Self {
Self(value.into())
}

/// Creates a new instance of this id type from the given byte slice.
pub fn from_bytes(bytes: &[u8]) -> Self {
Self(bytes.to_vec())
Self(bytes.into())
}

/// Parses the given hex string into an ObjectId.
Expand All @@ -75,7 +75,7 @@ macro_rules! impl_id_type {

/// Parses the given hex string into an ObjectId.
pub fn try_from_hex(hex: impl AsRef<[u8]>) -> Option<Self> {
$crate::hex_util::decode_hex(hex).map(Self)
$crate::hex_util::decode_hex(hex).map(Self::from_vec)
}
}

Expand Down Expand Up @@ -119,7 +119,7 @@ macro_rules! impl_id_type {
}

fn to_bytes(&self) -> Vec<u8> {
self.0.clone()
self.0.to_vec()
}

fn hex(&self) -> String {
Expand Down
2 changes: 1 addition & 1 deletion lib/src/rewrite.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ pub fn find_recursive_merge_commits(
}

fn merge_next(&mut self, ancestor: Merge<CommitId>) {
let dummy = Merge::resolved(CommitId::new(vec![]));
let dummy = Merge::resolved(CommitId::from_vec(vec![]));
let result = mem::replace(&mut self.result, dummy);
let other = Merge::resolved(self.commit_ids[self.pos].clone());
self.result = Merge::from_vec(vec![result, ancestor, other]).flatten();
Expand Down
2 changes: 1 addition & 1 deletion lib/src/settings.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,7 +301,7 @@ impl JJRng {
pub fn new_change_id(&self, length: usize) -> ChangeId {
let mut rng = self.0.lock().unwrap();
let random_bytes = (0..length).map(|_| rng.random::<u8>()).collect();
ChangeId::new(random_bytes)
ChangeId::from_vec(random_bytes)
}

/// Creates a new RNGs. Could be made public, but we'd like to encourage all
Expand Down
29 changes: 17 additions & 12 deletions lib/src/simple_backend.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,7 @@ impl Backend for SimpleBackend {
hasher.update(bytes);
}
file.flush().map_err(to_other_err)?;
let id = FileId::new(hasher.finalize().to_vec());
let id = FileId::from_vec(hasher.finalize().to_vec());

persist_content_addressed_temp_file(temp_file, self.file_path(&id))
.map_err(to_other_err)?;
Expand All @@ -238,7 +238,7 @@ impl Backend for SimpleBackend {
.map_err(to_other_err)?;
let mut hasher = Blake2b512::new();
hasher.update(target.as_bytes());
let id = SymlinkId::new(hasher.finalize().to_vec());
let id = SymlinkId::from_vec(hasher.finalize().to_vec());

persist_content_addressed_temp_file(temp_file, self.symlink_path(&id))
.map_err(to_other_err)?;
Expand Down Expand Up @@ -281,7 +281,7 @@ impl Backend for SimpleBackend {
.write_all(&proto.encode_to_vec())
.map_err(to_other_err)?;

let id = TreeId::new(blake2b_hash(tree).to_vec());
let id = TreeId::from_vec(blake2b_hash(tree).to_vec());

persist_content_addressed_temp_file(temp_file, self.tree_path(&id))
.map_err(to_other_err)?;
Expand Down Expand Up @@ -331,7 +331,7 @@ impl Backend for SimpleBackend {
.write_all(&proto.encode_to_vec())
.map_err(to_other_err)?;

let id = CommitId::new(blake2b_hash(&commit).to_vec());
let id = CommitId::from_vec(blake2b_hash(&commit).to_vec());

persist_content_addressed_temp_file(temp_file, self.commit_path(&id))
.map_err(to_other_err)?;
Expand Down Expand Up @@ -380,12 +380,17 @@ fn commit_from_proto(mut proto: crate::protos::simple_store::Commit) -> Commit {
sig,
});

let parents = proto.parents.into_iter().map(CommitId::new).collect();
let predecessors = proto.predecessors.into_iter().map(CommitId::new).collect();
let merge_builder: MergeBuilder<_> = proto.root_tree.into_iter().map(TreeId::new).collect();
let parents = proto.parents.into_iter().map(CommitId::from_vec).collect();
let predecessors = proto
.predecessors
.into_iter()
.map(CommitId::from_vec)
.collect();
let merge_builder: MergeBuilder<_> =
proto.root_tree.into_iter().map(TreeId::from_vec).collect();
let root_tree = merge_builder.build();
let conflict_labels = ConflictLabels::from_vec(proto.conflict_labels);
let change_id = ChangeId::new(proto.change_id);
let change_id = ChangeId::from_vec(proto.change_id);
Commit {
parents,
predecessors,
Expand Down Expand Up @@ -461,7 +466,7 @@ fn tree_value_to_proto(value: &TreeValue) -> crate::protos::simple_store::TreeVa
fn tree_value_from_proto(proto: crate::protos::simple_store::TreeValue) -> TreeValue {
match proto.value.unwrap() {
crate::protos::simple_store::tree_value::Value::TreeId(id) => {
TreeValue::Tree(TreeId::new(id))
TreeValue::Tree(TreeId::from_vec(id))
}
crate::protos::simple_store::tree_value::Value::File(
crate::protos::simple_store::tree_value::File {
Expand All @@ -470,12 +475,12 @@ fn tree_value_from_proto(proto: crate::protos::simple_store::TreeValue) -> TreeV
copy_id,
},
) => TreeValue::File {
id: FileId::new(id),
id: FileId::from_vec(id),
executable,
copy_id: CopyId::new(copy_id),
copy_id: CopyId::from_vec(copy_id),
},
crate::protos::simple_store::tree_value::Value::SymlinkId(id) => {
TreeValue::Symlink(SymlinkId::new(id))
TreeValue::Symlink(SymlinkId::from_vec(id))
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion lib/src/simple_op_heads_store.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@ impl OpHeadsStore for SimpleOpHeadsStore {
)
})?;
if let Some(op_head) = hex_util::decode_hex(op_head_file_name) {
op_heads.push(OperationId::new(op_head));
op_heads.push(OperationId::from_vec(op_head));
}
}
op_heads.sort();
Expand Down
Loading
Loading