[WIP] opt: add branch builder push_chunk #568
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
gabriele-0201
force-pushed
the
gm_branch_builder_push_chunk_opt
branch
2 times, most recently
from
3c1f04d to
f049b80
Compare
gabriele-0201
force-pushed
the
gm_bitwise_memcpy
branch
from
228f671 to
e0eef91
Compare
gabriele-0201
force-pushed
the
gm_branch_builder_push_chunk_opt
branch
from
f049b80 to
9e68d06
Compare
|
During the optimization of the branch_stage, I have just found out that it will likely be easier and more accurate to avoid having chunks intersect with non-compressed items. There are multiple reasons for this, and the decision is still pending, heavily dependent on the code I'm working on to introduce binary_search into the branch_stage. Therefore, I will move back this PR to WIP |
gabriele-0201
changed the title
abstract copy and shift bits logic
gabriele-0201
force-pushed
the
gm_bitwise_memcpy
branch
from
e0eef91 to
fdd04bb
Compare
gabriele-0201
force-pushed
the
gm_branch_builder_push_chunk_opt
branch
from
9e68d06 to
3941ad8
Compare
| @@ -107,10 +114,31 @@ impl BranchNode { | |||
| self.as_mut_slice()[start..].view_bits_mut()[..prefix_len].copy_from_bitslice(prefix); | |||
| } | |||
|
|
|||
| pub fn raw_separator(&self, i: usize) -> RawSeparator { | |||
| fn cells_mut(&self) -> &mut [[u8; 2]] { | |||
| // and the relative bit length | ||
| // | ||
| // The raw bytes are always a multiple of 8 bytes in length | ||
| pub type RawSeparator<'a> = (&'a [u8], usize, usize); | ||
| pub type RawSeparators<'a> = (&'a [u8], usize, usize); |
There was a problem hiding this comment.
it's odd to define this type as a plural but then use it like a singular in e.g. fn raw_separator or fn reconstruct_key
| @@ -294,11 +387,305 @@ impl BranchNodeBuilder { | |||
| self.index += 1; | |||
| } | |||
|
|
|||
| pub fn push_chunk(&mut self, base: &BranchNode, from: usize, to: usize) { | |||
There was a problem hiding this comment.
Looking at this function, it looks to me like it internalizes too much complexity, especially around handling edge cases.
the contract of this function is not clear. the typical case I expect we would optimize for is basically just a single bitwise memcpy of the separators and a normal memcpy of the node pointers, which is what we get when the prefix hasn't changed.
I don't see a strong case that this would be more efficient than calling push repeatedly in the edge cases, and we've also pessimized the base case with a bunch of branches and additional logic.
It seems better to have alternative variants for each expected code path.
My suggestion:
- Have a single
push_chunkvariant which assumes that the prefix has not changed. This would be used in our 'base case' as well as any uncompressed separators. - Call
pushin a loop, otherwise.
My gut feeling about optimization here is that we should be mostly memory-constrained, so it makes more sense to keep the code simple and have tight loops and externalize our branches as much as possible.
gabriele-0201
force-pushed
the
gm_bitwise_memcpy
branch
from
fdd04bb to
cfdcca5
Compare
rphmeier
changed the base branch from
gm_bitwise_memcpy
to
graphite-base/568
I'm not a fan of having introduced
RawSeparators, which include the oldRawSeparator, but having both seems very redundant.If you have other ideas, they are welcome!