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.

Sign up for GitHub

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

Jump to bottom

Impl Extend for LiteMap and avoid quadratic behavior in from_iter and deserialize #6132

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Impl Extend for LiteMap and avoid quadratic behavior in from_iter and deserialize #6132

wants to merge 8 commits into from

Conversation

arthurprs
Copy link
Contributor

@arthurprs arthurprs commented Feb 15, 2025 •
edited
Loading

Adds lm_extend to the StoreMut trait. The Vec implementation is optimized to have O(N) performance on sorted inputs and an asymptotic worst case of O(NLog(N)), effectively avoiding quadratic worst cases.

This PR takes advantage of the new extend optimization and changes the FromIterator and Deserialize implementations to also avoid quadratic worst-case.

Before

litemap/from_iter_rand/small
                        time:   [916.60 ns 941.77 ns 965.82 ns]
litemap/from_iter_rand/large
                        time:   [2.3779 s 2.3863 s 2.3946 s]
litemap/from_iter_sorted/small
                        time:   [84.010 ns 84.084 ns 84.155 ns]
litemap/from_iter_sorted/large
                        time:   [725.31 µs 739.88 µs 755.20 µs]

After

litemap/from_iter_rand/small
                        time:   [354.62 ns 365.31 ns 376.02 ns]
litemap/from_iter_rand/large
                        time:   [25.129 ms 25.415 ms 25.704 ms]
litemap/from_iter_sorted/small
                        time:   [76.593 ns 76.767 ns 76.938 ns]
litemap/from_iter_sorted/large
                        time:   [662.70 µs 673.66 µs 686.83 µs]
litemap/extend_rand/large
                        time:   [66.221 ms 67.467 ms 68.814 ms]
litemap/extend_rand_dups/large
                        time:   [190.24 ms 196.05 ms 202.13 ms]
litemap/extend_from_litemap_rand/large
                        time:   [2.0401 s 2.0488 s 2.0575 s]

This is a followup of #6068

@arthurprs arthurprs requested review from Manishearth, sffc and a team as code owners February 15, 2025 22:25
@arthurprs arthurprs force-pushed the litemap-extend-and-quadratic-worst-case branch 3 times, most recently from 82433f8 to 1f7e87a Compare February 15, 2025 23:02
@arthurprs arthurprs force-pushed the litemap-extend-and-quadratic-worst-case branch from 1f7e87a to 8bd4d3e Compare February 16, 2025 00:09
arthurprs
arthurprs commented Feb 16, 2025
View reviewed changes
utils/litemap/src/serde.rs
// Note: this effectively selection sorts the map,
// which isn't efficient for large maps
map.insert(key, value);
out_of_order.push((key, value));
Copy link
Contributor Author

@arthurprs arthurprs Feb 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I considered making this based on StoreMut::lm_push and Store::lm_into_iter by adding the StoreIntoIterator bound to R, but that would be a breaking change.

arthurprs
arthurprs commented Feb 16, 2025
View reviewed changes
utils/litemap/Cargo.toml
@@ -43,7 +44,7 @@ criterion = { workspace = true }
default = ["alloc"]
alloc = []
databake = ["dep:databake"]
serde = ["dep:serde"]
serde = ["dep:serde", "alloc"]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note for reviewers: The addition of alloc was theoretically a breaking change for people using "no default features" so we may want to bump litemap to 0.8.

robertbastian
robertbastian reviewed Feb 26, 2025
View reviewed changes
utils/litemap/src/store/mod.rs
@@ -133,6 +133,24 @@ pub trait StoreMut<K, V>: Store<K, V> {
}
}
}

/// Extends this store with items from an iterator.
fn lm_extend<I>(&mut self, other: I)
Copy link
Member

Choose a reason for hiding this comment

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

issue: having the quadratic algorithm in the default implementation is a footgun. same for lm_retain

Copy link
Contributor Author

@arthurprs arthurprs Feb 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

To clarify, are you suggesting that the default impl for lm_extend and lm_retain* should be removed?

* not part of the PR

Copy link
Member

Choose a reason for hiding this comment

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

yes

Copy link
Member

Choose a reason for hiding this comment

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

(if we're happy to make breaking changes)

robertbastian
robertbastian reviewed Feb 27, 2025
View reviewed changes
@arthurprs arthurprs force-pushed the litemap-extend-and-quadratic-worst-case branch from d239648 to 78edff4 Compare February 27, 2025 20:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@robertbastian robertbastian robertbastian left review comments

@Manishearth Manishearth Awaiting requested review from Manishearth Manishearth is a code owner

@sffc sffc Awaiting requested review from sffc sffc is a code owner

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@arthurprs @robertbastian