object_id: back id types by Box<[u8]> instead of Vec<u8> - #9930
object_id: back id types by Box<[u8]> instead of Vec<u8>#9930martinvonz wants to merge 1 commit into
Box<[u8]> instead of Vec<u8>#9930Conversation
There was a problem hiding this comment.
optional: It still may be useful to provide some to_vec(), from_vec() functions so callers can migrate.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
It's actually the
Box<[u8]>versions that are missing. We may want to add those and see if we can avoid conversions to/fromVec<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.
There was a problem hiding this comment.
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>)tofrom_vec(Vec<u8>).
Done.
| $(#[$attr])* | ||
| #[derive($crate::content_hash::ContentHash, PartialEq, Eq, PartialOrd, Ord, Clone, Hash)] | ||
| $vis struct $name(Vec<u8>); | ||
| $vis struct $name(Box<[u8]>); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Good point. I think my answer is the same as above: it's hard to know without someone doing some profiling.
There was a problem hiding this comment.
As far as I can tell, the allocation cost isn't significant. What matters more is the cache locality of things like Vec<TreeValue>.
There was a problem hiding this comment.
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 🙃
The ids are immutable, so there is no need to track spare capacity. This makes each id type one `usize` smaller.
87105e2 to
aa4dc36
Compare
5 participants
The ids are immutable, so there is no need to track spare capacity. This makes each id type one
usizesmaller.Checklist
If applicable:
CHANGELOG.mdREADME.md,docs/,demos/)cli/src/config-schema.json)how it works, how it's organized), including any code drafted by an LLM.
an eye towards deleting anything that is irrelevant, clarifying anything
that is confusing, and adding details that are relevant. This includes,
for example, commit descriptions, PR descriptions, and code comments.