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.
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
Use ecow::EcoVec
for Record
internals
#12624
base: main
Are you sure you want to change the base?
Conversation
// pub fn drain<R>(&mut self, range: R) -> Drain | ||
// where | ||
// R: RangeBounds<usize> + Clone, | ||
// { | ||
// Drain { | ||
// iter: self.inner.drain(range) | ||
// } | ||
// } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, this is one of the methods that EcoVec
doesn't yet support. I couldn't come up with an easy workaround, so I've just removed this method.
I think this would probably make |
Can you drop some numbers for the full suite? I think we may need some more record mutation heavy benchmarks than the pure growing. If we go that route we probably want to consider upstreaming some convenience APIs like |
Sure thing, here are all the numbers (on my hardware). Top is main (bed2363), bottom is this PR. Table
Looks like there is a little bit of overhead for mutation (e.g., |
An alternative to this proposal would be using persistent data structures, such as ones implemented in rpds or im. Compared to Just curious, but why were we not using a map-like data structure in the first place? |
I remember seeing in a comment that we were using Immutable data structures also often have tradeoffs that make them perform worse for many operations, so there have to be benefits from the immutability (e.g. benefits gained from partial reuse of the data, on a large tree) to make up for it |
That's true. Benchmarking is the only way to know, but it's an option worth considering. |
Description
This PR changes the internal data structure in
Record
from aVec
to anecow::EcoVec
. Theecow
crate is somewhat new and is not yet 1.0, but it's already seeing regular use through several popular Rust projects:So, I'm not that worried about the crate potentially seeing little development in the future.
Rather, switching to an
EcoVec
has several benefits:Value::Record
. Previously, the record data was behind two levels of indirection (aVec
followed by aSharedCow
).EcoVec
s are only 16 bytes in size, so this gives us the freedom to split the columns and values inRecord
again without increasing the current size ofValue
.EcoVec
has most of the methods thatVec
does.Arc
weak count).