Skip to content

Tracking Issue for partition_point #73831

Closed
@VillSnow

Description

@VillSnow
Contributor

Feature gate: #![feature(partition_point)]

This is a tracking issue for [T]::partition_point.

See also rust-lang/rfcs#2184

Public API

impl<T> [T] {
    pub fn partition_point<P>(&self, mut pred: P) -> usize
    where
        P: FnMut(&T) -> bool;
}

Steps / History

Unresolved Questions

  • Is there a better name? partition_point matches C++, but might be hard to recognize as a binary search.

Activity

added
C-tracking-issueCategory: An issue tracking the progress of sth. like the implementation of an RFC
on Jun 28, 2020
added
B-unstableBlocker: Implemented in the nightly compiler and unstable.
T-libs-apiRelevant to the library API team, which will review and decide on the PR/issue.
on Jun 28, 2020
timvermeulen

timvermeulen commented on Jul 12, 2020

@timvermeulen
Contributor

In order to deduplicate this code with binary_search_by's implementation, I think we can simply implement partition_point in terms of binary_search_by like so:

use crate::cmp::Ordering::{Less, Greater};

impl<T> [T] {
    pub fn partition_point<P>(&self, mut pred: P) -> usize
    where
        P: FnMut(&T) -> bool,
    {
        self.binary_search_by(|x| if pred(x) { Less } else { Greater })
            .unwrap_or_else(|i| i)
    }
}
VillSnow

VillSnow commented on Jul 13, 2020

@VillSnow
ContributorAuthor

@timvermeulen

I prefer my partition_point code than current binary_search_by code.
I found current binary_search_by has a problem that it usually calls one extra comparison.

I tried

  • previous binary_search_by logic (=logic1),
  • current binary_search_by logic (=logic2), and
  • partition_point logic (=logic3).

In addition, I also tried to

  • remove jump from partition_point (logic4).

I'm not sure succeeded to remove, or possibly already done from current code.

https://github.com/VillSnow/compare-binary-searches/blob/master/src/main.rs

You can find logic1, 3, 4 need 3 comparisons to find from 0..7, while logic2 needs 4 calls.
I think this causes significant cost more than optimization trick if users can pass their own predictions.

Here is a typical result of bench. Find 0 from 0..65536 and find 0 from 7 but inserted sleep(1ms) in comparison.
Logic2 is much slower than others in case of custom predication.

logic [i32; 65536] [Slow; 7]
logic1 30ns 5.4ms
logic2 9ns 7.2ms
logic3 10ns 5.6ms
logic4 10ns 5.6ms
timvermeulen

timvermeulen commented on Jul 13, 2020

@timvermeulen
Contributor

I found current binary_search_by has a problem that it usually calls one extra comparison.

I believe this is intended, it is not optimized for doing the minimum number of comparisons. Make sure to run the benchmarks in https://github.com/rust-lang/rust/blob/master/src/libcore/benches/slice.rs before drawing any conclusions.

And regardless of what the ideal implementation of binary_search_by looks like, partition_point should probably still be implemented in terms of it in the way I outlined above 🙂

VillSnow

VillSnow commented on Jul 14, 2020

@VillSnow
ContributorAuthor

@timvermeulen

I found current binary_search_by has a problem that it usually calls one extra comparison.

I believe this is intended, it is not optimized for doing the minimum number of comparisons. Make sure to run the benchmarks in https://github.com/rust-lang/rust/blob/master/src/libcore/benches/slice.rs before drawing any conclusions.

AFAIK, this bench is written when logic1 is replaced into logic2, not after logic3. The bench tries to find random element from usize slice, not user type which needs large cost to compare.
If it does not optimize for minimum number of comparisons, itself is a problem, I think.
If user need 1ns faster binary search, they can write by themselves or use 3rd party crate, as well as HashMap.

And regardless of what the ideal implementation of binary_search_by looks like, partition_point should probably still be implemented in terms of it in the way I outlined above 🙂

Is it means partition_point should call binary_search_by instead of binary_search_by calls partition_point?
I agree because binary_search_by has more information and the cost would be small.

added
Libs-SmallLibs issues that are considered "small" or self-contained
Libs-TrackedLibs issues that are tracked on the team's project board.
on Jul 29, 2020
scottmcm

scottmcm commented on Aug 26, 2020

@scottmcm
Member

I noticed in passing that this is using FnMut. That seems strange to me, since there's no guarantee which items will be looked at or in which order by this API, so actually mutating in the closure would seem an anti-pattern. Should it maybe use Fn instead? (People could always use interior mutability if they really really really want to update things in the closure, but this would discourage it.)

Though I guess sort_by takes FnMut...

VillSnow

VillSnow commented on Aug 27, 2020

@VillSnow
ContributorAuthor

@scottmcm memoization or exterior (web/DB) API access using mut handle?

matklad

matklad commented on Jan 10, 2021

@matklad
Member

@VillSnow this has been baking in the nightly for a while, I think this is ready for stabilization. Would like to send a stabilization PR?

VillSnow

VillSnow commented on Jan 10, 2021

@VillSnow
ContributorAuthor

If the team think all right, I'm glad.

I still concern about code duplication, and I'm watching this PR #74024.
However, how predicate is called is not documented, so even if the algorithm was changed, it never change the stable branch.

I have no objection to merge this.

matklad

matklad commented on Jan 11, 2021

@matklad
Member

If the team think all right, I'm glad.

Submitting a stabilization PR is a way to figure that out! I am not on the libs team, but this seems stabilization-worthy to me.

I wouldn't worry about impl details -- they can be adjusted after stabilization.

20 remaining items

m-ou-se

m-ou-se commented on Jan 30, 2021

@m-ou-se
Member

Sounds like this is the best name we can come up with.

So, let's make sure the documentation of the binary_search functions point to partition_point as an alternative, and maybe add a doc(alias) for it as well.

If anybody feels like updating the documentation here: PRs welcome. ^^

Let's kick off the FCP for stabilizing this:

@rfcbot resolve name

rfcbot

rfcbot commented on Jan 30, 2021

@rfcbot
Collaborator

🔔 This is now entering its final comment period, as per the review above. 🔔

added
final-comment-periodIn the final comment period and will be merged soon unless new substantive objections are raised.
and removed
proposed-final-comment-periodProposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off.
on Jan 30, 2021
VillSnow

VillSnow commented on Jan 30, 2021

@VillSnow
ContributorAuthor

I don't know much about doc(alias), but does it show this?

2021-01-31 (1)

It looks like to make a flow from other names. For example, some languages has reduce, so type 'reduce' at the search box, then hit Iterator::fold.
However, Rust already has binary_search for who want to find any item by value. It seems a bit different usage to suggest similar methods.

added and removed
final-comment-periodIn the final comment period and will be merged soon unless new substantive objections are raised.
on Feb 9, 2021
rfcbot

rfcbot commented on Feb 9, 2021

@rfcbot
Collaborator

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

The RFC will be merged soon.

added a commit that references this issue on Feb 12, 2021

Rollup merge of rust-lang#81012 - VillSnow:stabilize_partition_point,…

8280abc
matklad

matklad commented on Feb 15, 2021

@matklad
Member

Closed by #81012

Thanks @VillSnow for bringing this all the way from an idea to the stabilization!

VillSnow

VillSnow commented on Feb 16, 2021

@VillSnow
ContributorAuthor

Thanks for much assistance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    A-sliceArea: `[T]`B-unstableBlocker: Implemented in the nightly compiler and unstable.C-tracking-issueCategory: An issue tracking the progress of sth. like the implementation of an RFCLibs-SmallLibs issues that are considered "small" or self-containedLibs-TrackedLibs issues that are tracked on the team's project board.T-libs-apiRelevant to the library API team, which will review and decide on the PR/issue.disposition-mergeThis issue / PR is in PFCP or FCP with a disposition to merge it.finished-final-comment-periodThe final comment period is finished for this PR / Issue.

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

      Development

      No branches or pull requests

        Participants

        @BurntSushi@m-ou-se@matklad@jonas-schievink@timvermeulen

        Issue actions

          Tracking Issue for `partition_point` · Issue #73831 · rust-lang/rust