Skip to content

Implement ordering chaining #37053

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Closed
rednum opened this issue Oct 9, 2016 · 11 comments
Closed

Implement ordering chaining #37053

rednum opened this issue Oct 9, 2016 · 11 comments
Labels
B-unstable Blocker: Implemented in the nightly compiler and unstable. final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@rednum
Copy link
Contributor

rednum commented Oct 9, 2016

As discussed in: rust-lang/rfcs#1677

I've already started working on this and will open a PR soon.

bors added a commit that referenced this issue Nov 2, 2016
Add or and or_else for ordering.

Fixes #37053 (see discussion in rust-lang/rfcs#1677).
@apasel422
Copy link
Contributor

Reopening because this is a tracking issue.

@apasel422 apasel422 reopened this Nov 19, 2016
@apasel422 apasel422 added the B-unstable Blocker: Implemented in the nightly compiler and unstable. label Nov 19, 2016
@sfackler
Copy link
Member

@rfcbot fcp merge

@sfackler sfackler added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Jan 31, 2017
@SimonSapin
Copy link
Contributor

Looks like @rfcbot missed @sfackler’s message?

I also think this addition looks good, and would like it to be stabilized.

@aturon
Copy link
Member

aturon commented Feb 24, 2017

@rfcbot fcp merge

@rfcbot
Copy link
Collaborator

rfcbot commented Feb 24, 2017

Team member @aturon has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@alexcrichton
Copy link
Member

ping @BurntSushi (checkbox)

@rfcbot
Copy link
Collaborator

rfcbot commented Feb 28, 2017

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

@rfcbot rfcbot added the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label Feb 28, 2017
@rfcbot
Copy link
Collaborator

rfcbot commented Feb 28, 2017

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

@rfcbot
Copy link
Collaborator

rfcbot commented Mar 10, 2017

The final comment period is now complete.

@jongiddy
Copy link
Contributor

I have some reservations about this change.

I changed the code at https://github.com/jongiddy/gidsort/blob/229720e12ac9602b844c623e584dd6f876d1e3a3/src/lib.rs#L71-L74

        let mut leg = compare(value, &buffer[trial]);
        if leg == Ordering::Equal {
            leg = when_equal;
        };

to https://github.com/jongiddy/gidsort/blob/2e0280362e36d7ca257d9057f2141a25b83dba6f/src/lib.rs#L73

        let leg = compare(value, &buffer[trial]).then(when_equal);

First, it's nice to have fewer lines and one less mut.

But the benchmark penalty was surprising. The most appropriate benchmark, large_ascending_gidsort, which calls the insertion_point function a lot but does not do much other work, went from 23,733 ns/iter (+/- 1,835) to 39,037 ns/iter (+/- 1,719).

Also, the big plus for the explicit version is that a non-Rust programmer could easily understand it. then is meaningless. As a fairly uncommon method, something like when_equal would be more obvious. this also allows the calling version to be when_equal_then, which is more consistent with Option and_then than the new then_with form.

@ghost
Copy link

ghost commented Mar 14, 2017

@jongiddy I diagnosed and fixed the problem - we were simply missing #[inline] attributes :)

frewsxcv added a commit to frewsxcv/rust that referenced this issue Mar 15, 2017
…excrichton

Inline functions Ordering::{then, then_with}

@jongiddy noticed bad performance due to the lack of inlining on `then`
and `then_with`. I confirmed that inlining really is the culprit by
creating a custom `then` function and repeating his benchmark on my
machine with and without the `#[inline]` attribute.

The numbers were exactly the same on my machine without the attribute.
With `#[inline]` I got the same performance as I did with manually
inlined implementation.

The problem was reported in rust-lang#37053.
frewsxcv pushed a commit to frewsxcv/rust that referenced this issue Mar 17, 2017
frewsxcv added a commit to frewsxcv/rust that referenced this issue Mar 17, 2017
…excrichton

Inline functions Ordering::{then, then_with}

@jongiddy noticed bad performance due to the lack of inlining on `then`
and `then_with`. I confirmed that inlining really is the culprit by
creating a custom `then` function and repeating his benchmark on my
machine with and without the `#[inline]` attribute.

The numbers were exactly the same on my machine without the attribute.
With `#[inline]` I got the same performance as I did with manually
inlined implementation.

The problem was reported in rust-lang#37053.
alexcrichton pushed a commit to alexcrichton/rust that referenced this issue Mar 17, 2017
@bors bors closed this as completed in 48890d4 Mar 19, 2017
spikespaz pushed a commit to spikespaz/dotwalk-rs that referenced this issue Aug 29, 2024
Add or and or_else for ordering.

Fixes rust-lang/rust#37053 (see discussion in rust-lang/rfcs#1677).
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
B-unstable Blocker: Implemented in the nightly compiler and unstable. final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

8 participants