Batcher implementation that has no opinions about chains, and columnar. #626
Conversation
frankmcsherry
force-pushed
the
unchained
branch
4 times, most recently
from
f33488a to
e2e49ec
Compare
frankmcsherry
force-pushed
the
unchained
branch
from
e2e49ec to
aaa2103
Compare
| fn partition<I>(container: &mut Self::Container, builders: &mut [Self], mut index: I) | ||
| where | ||
| Self: for<'a> PushInto<<Self::Container as timely::Container>::Item<'a>>, | ||
| I: for<'a> FnMut(&<Self::Container as timely::Container>::Item<'a>) -> usize, | ||
| { | ||
| println!("Exchanging!"); | ||
| for datum in container.drain() { | ||
| let index = index(&datum); | ||
| builders[index].push_into(datum); | ||
| } | ||
| container.clear(); | ||
| } | ||
|
|
There was a problem hiding this comment.
We might have to revisit how to exchange containers that cannot be drained/iterated here. The implementation doesn't actually work because container.drain() is unimplemented.
There was a problem hiding this comment.
Totally. I put it in only to be able to see if it panicked, because I wasn't certain what would happen with the default implementation (pretty sure it would panic too, but copy/pasted to be sure). It's definitely within the PR's power to have an implementation that works, and this is not it. :D
| std::cmp::Ordering::Less => { | ||
| let lower = this_key_range.start; | ||
| gallop(this.keys.borrow(), &mut this_key_range, |x| x < that_key); | ||
| merged.extend_from_keys(&this, lower .. this_key_range.start); |
There was a problem hiding this comment.
(here and below) extend_from_keys/_vals calls extend_from_self, but we don't (or can't!) presize the merged container. Pointing out that this is a potential performance regression if we have to reallocate often.
As we can't know the final size ahead of time, we could allocate a container big enough to hold the sum of the two inputs. In the worst case, this would only waste virtual memory I think.
But, let's first measure and see whether it shows up.
There was a problem hiding this comment.
Elsewhere (datatoad) the presizing makes sense and seems to help a bit. One way to view merging and consolidation is as literally merging the tuples (no consolidation), for which the capacities could just be the sums of the capacities for each layer, followed by a consolidation pass that ends up filtering tuples out (ones that end up as zero), for which .. we may or may not feel bad about the over allocation. In a demand-paging world, I wouldn't feel too bad. In a world where capacities are limited, even if you don't use the data, more complicated.
frankmcsherry
force-pushed
the
unchained
branch
from
d6582e3 to
76c6cf3
Compare
frankmcsherry
changed the title
|
I'm going to merge this on the principle that while the parts may not be final, they are not wrong and we can iterate on them in the repo. |
This PR provides a new
Batcherimplementation that is quite simple, though not as lean as the existing merge batcher. It relies on containers that implement two methods:mergeandsplit, for merging two containers and for splitting a container based on a time frontier. It then handles the LSM details using these implementations.It also dramatically revises the
columnar.rsexample to be containers end-to-end, with the same type on the wire, in batchers, and ultimately backing arrangements. The performance of the revised example improves over thespines.rsexample, but this could be attributed to better on-the-wire performance, or a new batcher that gains in performance at the expense of higher peak memory utilization.Both of these changes are net new, and don't break any existing tools. They should be safe to merge, as long as we are ok relocating or removing them if we find them lacking. There is also a light and weird change to the
ord_neu::keystorage, where we need to have an actual instance of some specified value type, which has been()in examples so far. Something about how the types work end up making an assumed&()incompatible with a container, if I recall correctly. It also seems mostly harmless, and backward compatible.