Skip to content
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

add filter functionality #16

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

cavemanloverboy
Copy link

This provides a drain_filter and filter functionality on the red black tree implementation.

Inspiration: in phoenix-v1's CancelAllOrders instruction, the tree is traversed and matching ids are collected. These ids are then used via book.remove(...) which traverses the tree to remove the entry. Instead of traversing the tree, this implementation records allocator node addresses and removes them all upon dropping the new RedBlackTreeDrainFilter type.

A quick and dirty benchmark of 128 removals from a full tree of 4096 items yields the following results on an M2 Max:

./target/release/examples/bench_filter
average id + remove: 59280 micros
average drain_alloc: 44410 micros
average drain: 43759 micros

which is an O(30%) reduction in the wall time of the removals.

const MAX_SIZE: usize,
> Drop for RedBlackTreeDrainFilter<'a, K, V, P, MAX_SIZE>
{
fn drop(&mut self) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

The gist of the speed up here is that the traversal traversal to remove nodes is O(K) instead of O(K log N) right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually still O(K log N) but you save a log N on the find

Copy link
Author

Choose a reason for hiding this comment

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

Essentially, with remove you have to traverse the tree from the root to the entry to re-find the node addr of the entry you just found.

If you iterate through the elements and record the addr instead of the keys, you don't have to do that traversal to remove the entry. That's the gist

.nodes
.get((ptr - 1) as usize)
.unwrap()
.get_value();
Copy link
Collaborator

Choose a reason for hiding this comment

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

is there not a helper function to perform this lookup?

Copy link
Author

Choose a reason for hiding this comment

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

This part was mostly copied from the iterator implementation, but perhaps

Copy link
Author

Choose a reason for hiding this comment

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

switched to get_node. much neater

Copy link
Collaborator

@jarry-xiao jarry-xiao left a comment

Choose a reason for hiding this comment

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

Clever change! Please bump the version in the Cargo.toml

@cavemanloverboy
Copy link
Author

Clever change! Please bump the version in the Cargo.toml

Will do. Also, it's come to my attention that the drain_filter method for std::vec::Vec that this was supposed to be analogous to has been renamed to extract_if:

rust-lang/rust#43244

I'll try to come up with some better names for the methods.

@cavemanloverboy
Copy link
Author

cavemanloverboy commented Dec 19, 2023

  1. I switched to extract_if to match the method on std collections (btreemap, vec, etc, e.g. https://doc.rust-lang.org/nightly/std/vec/struct.Vec.html#method.extract_if).
  2. I added a docstring w/ test on the method w/ some notes.
  3. I removed the filter method since you can just do extract_if and collect into collection of your choice -- instead of locking user into Vec.
  4. Bumped to 0.4.0

@cavemanloverboy
Copy link
Author

One final thing we may want to do is either implement DoubleEndedIterator or remove the rev fields from the ExtractIf type

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants