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

Improve BTreeSet::Intersection::size_hint #64383

Merged
merged 1 commit into from Sep 16, 2019

Conversation

@pcpthm
Copy link
Contributor

commented Sep 11, 2019

A comment on IntersectionInner mentions small_iter should be smaller than other_iter but this condition is broken while iterating because those two iterators can be consumed at a different rate. I added a test to demonstrate this situation.
I made small_iter.len() < other_iter.len() always true by swapping two iterators when that condition became false. This change affects the return value of size_hint. The previous result was also correct but this new version always returns smaller upper bound than the previous version.
I changed size_hint to taking minimum of both lengths of iterators and renamed fields to a and b to match Union iterator.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Sep 11, 2019

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @dtolnay (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@dtolnay

This comment has been minimized.

Copy link
Member

commented Sep 16, 2019

FYI @ssomers -- let me know if you have a preference for how is best to address this.

@dtolnay
Copy link
Member

left a comment

Thanks for catching this, @pcpthm!

@@ -1254,7 +1255,12 @@ impl<'a, T: Ord> Iterator for Intersection<'a, T> {
match Ord::cmp(small_next, other_next) {
Less => small_next = small_iter.next()?,
Greater => other_next = other_iter.next()?,
Equal => return Some(small_next),
Equal => {
if small_iter.len() > other_iter.len() {

This comment has been minimized.

Copy link
@dtolnay

dtolnay Sep 16, 2019

Member

Why swap them? It looks like the implementation of next would be correct without swapping (though we would want to change the name of small_iter as you observed). The reason to avoid swapping would be that this code typically runs many times while size_hint would typically run at most once. We could have instead size_hint take the minimum between the two child lengths.

This comment has been minimized.

Copy link
@pcpthm

pcpthm Sep 16, 2019

Author Contributor

Thank you. I have changed to this way.

Improve BTreeSet::Intersection::size_hint
The commented invariant that an iterator is smaller than other iterator
was violated after next is called and two iterators are consumed at
different rates.

@pcpthm pcpthm force-pushed the pcpthm:btreeset-size-hint branch from ee6af0b to 4333b86 Sep 16, 2019

@dtolnay
Copy link
Member

left a comment

Nice, looks good to me. Thanks!

@dtolnay

This comment has been minimized.

Copy link
Member

commented Sep 16, 2019

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

commented Sep 16, 2019

📌 Commit 4333b86 has been approved by dtolnay

@bors

This comment has been minimized.

Copy link
Contributor

commented Sep 16, 2019

⌛️ Testing commit 4333b86 with merge b6269f2...

bors added a commit that referenced this pull request Sep 16, 2019
Auto merge of #64383 - pcpthm:btreeset-size-hint, r=dtolnay
Improve BTreeSet::Intersection::size_hint

A comment on `IntersectionInner` mentions `small_iter` should be smaller than `other_iter` but this condition is broken while iterating because those two iterators can be consumed at a different rate. I added a test to demonstrate this situation.
<del>I made `small_iter.len() < other_iter.len()` always true by swapping two iterators when that condition became false. This change affects the return value of `size_hint`. The previous result was also correct but this new version always returns smaller upper bound than the previous version.</del>
I changed `size_hint` to taking minimum of both lengths of iterators and renamed fields to `a` and `b` to match `Union` iterator.
@ssomers

This comment has been minimized.

Copy link
Contributor

commented Sep 16, 2019

In my mind, the "small" prefix was about the original sets; and completely out of my mind, was the possibility to get the size hint while intersection is ongoing. Well spotted and fixed.

But I was never really happy about the algorithm. I wanted it to consider the unseen remainder of the sets, instead of the starting position. I thought we couldn't afford to len every iteration, but thanks to this discussion that seems stupid now: just count the seen elements while iterating. Then switch strategies when one of the sets has been drained down to the size of the other, and all that.To be continued...

@bors

This comment has been minimized.

Copy link
Contributor

commented Sep 16, 2019

☀️ Test successful - checks-azure
Approved by: dtolnay
Pushing b6269f2 to master...

@bors bors added the merged-by-bors label Sep 16, 2019

@bors bors merged commit 4333b86 into rust-lang:master Sep 16, 2019

5 checks passed

homu Test successful
Details
pr Build #20190916.5 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-6.0) Linux x86_64-gnu-llvm-6.0 succeeded
Details
pr (LinuxTools) LinuxTools succeeded
Details
@ssomers

This comment has been minimized.

Copy link
Contributor

commented Sep 16, 2019

Now the rust coming out of my brainworks has settled, I was mixing up stuff. The BTreeSet iterator already counts the remaining length. I think the only genuine case is draining down from a similar size to a much smaller size. For example, set A contains the numbers 0 to 999 and 2019, set B contains 1000 to 1999 and 2019. Currently, we iterate all of A and B to finally meet up at 2019. Instead we could see around 990 that it's more efficient to continue searching B for the few remaining items in A. At the cost of comparing the remaining sizes in every iteration (actually, only in the Less or Greater case if we've taken from the smaller one), and the cost of storing references to both sets. And considering that, in more complex examples, searching B will be costlier than the measurement the current rule for deciding up front was based on, because we can only search the entire B, not a set of the same size as the range we have yet to iterate.

@pcpthm pcpthm deleted the pcpthm:btreeset-size-hint branch Sep 16, 2019

@pcpthm

This comment has been minimized.

Copy link
Contributor Author

commented Sep 16, 2019

@ssomers I think both cases (Stitch and Search) can be integrated with optimal complexity if the underlying iterator has a method something like skip_while_less_than(&K).
This method is equivalent to the implementation below:

fn skip_while_less_than(&mut self, target: &K) -> Option<&K> {
  loop {
    let cur = self.next()?;
    if cur >= target {
      return Some(cur);
    }
  }
}

But with more efficient searching.
By skipping subtrees, worst time complexity should be O(log n).

The intersection can be implemented like this:

fn next(&mut self) -> Option<&'a K> {
  loop {
    let a_next = self.a.next()?;
    let b_next = self.b.skip_while_less_than(a_next)?;
    if a_next == b_next {
      return Some(a_next);
    }
    swap(&mut self.a, &mut self.b); // or swap based on remaining size.
  }
}

Do you think it is worth to code?

@ssomers

This comment has been minimized.

Copy link
Contributor

commented Sep 16, 2019

I have trouble wrapping my head around it right now, but it looks like the alternative "swivel" at https://github.com/ssomers/rust_bench_btreeset_intersection. It performed better in purposefully crafted examples, but not in general.

@pcpthm

This comment has been minimized.

Copy link
Contributor Author

commented Sep 16, 2019

@ssomers The operations are the same. However, by adding the lower-bound operation into the iterator, the time complexity should be improved compared to just calling other.range(key..) because range will find the node "from the root" each time unnecessarily while iterator can just inspect neighbors of the current node.

@ssomers

This comment has been minimized.

Copy link
Contributor

commented Sep 16, 2019

But that experimental next already inspects NEXT_COUNT_MAX neighbors before resorting to a search from the root (the value 1 in head is low, but it's still one neighbor, and I tried with more). One thing that's definitely missing is, as you pointed out, after it does a search, it should reassess which set is the small one. It's not possible to reassess which range is the small one, so "small" shouldn't be in the name. Like in the actual code, the only difference between "small" and "other" is for the size_hint.

@ssomers

This comment has been minimized.

Copy link
Contributor

commented Sep 16, 2019

Not sure I should continue to post here in this thread, but I guess it does no harm. I managed to switch from stitch to search when needed, but it's quite an overhaul to change the value of an enum variant in a mutable member function. I think the code is much more clear about what is mutating, but not easier to understand. Surprisingly, no deteriorated performance. Nice boost in test stagger_000_010::vs_x15_future, but it was kind of made for that: after iterating 1 of the 10 elements on the left, the 150 on the right are enough to make it switch to searching.

For the search case, reassessing which set is the small one is probably even more difficult: the existing Range do not have a size concept at all (that I could find). So nothing accomplished there yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.