Remove duplicated code from Iterator::{ne, lt, le, gt, ge} by timvermeulen · Pull Request #59262 · rust-lang/rust

timvermeulen · 2019-03-17T18:39:34Z

This PR delegates Iterator::ne to Iterator::eq and Iterator::{lt, le, gt, ge} to Iterator::partial_cmp.

Oddly enough, this change actually simplifies the generated assembly in some cases, although I don't understand assembly well enough to see if the longer assembly is doing something clever.

I also added two extremely simple benchmarks:

// before
test iter::bench_lt ... bench: 98,404 ns/iter (+/- 21,008)
test iter::bench_partial_cmp ... bench: 62,437 ns/iter (+/- 5,009)
// after
test iter::bench_lt ... bench: 61,757 ns/iter (+/- 8,770)
test iter::bench_partial_cmp ... bench: 62,151 ns/iter (+/- 13,753)

I have no idea why the current lt/le/gt/ge implementations don't seem to be compiled optimally, but simply having them call partial_cmp seems to be an improvement.

See #44729 for a previous discussion.

rust-highfive · 2019-03-17T18:39:37Z

r? @Kimundi

(rust_highfive has picked a reviewer for you, use r? to override)

hellow554 · 2019-03-18T14:57:53Z

- Some(Ordering::Greater) => return false,
- None => return false,
- }
+ match self.partial_cmp(other) {


Why not self.partial_cmp(other) == Some(Ordering::Less)?

Oh, I see, you probably wanted to keep it consistent across le/lt/ge/gt... I think that's okay then

That was indeed the rationale, but looking at it again, it might actually be worth it. It's a lot simpler and it's still consistent between lt/gt.

So is it worth to also fix std::cmp::PartialOrd?

What do you mean exactly?

hellow554 · 2019-03-18T15:00:03Z

What about eq~~/ne~~ (ne already does !eq)?

hellow554 · 2019-03-18T15:02:00Z

You could also redirect parital_cmp to cmp, e.g. and wrapping it into Some, because they use exactly the same code :/

timvermeulen · 2019-03-18T15:59:35Z

@hellow554 Unfortunately we can't forward partial_cmp to cmp since cmp requires that Self::Item: Ord. I suppose we could instead forward cmp to partial_cmp (+ .unwrap()), but that doesn't seem worth it. The same applies to eq, it can't be forwarded to cmp/partial_cmp.

There's indeed still some code duplication left but I haven't found a nice way to get rid of it without sacrificing readability. I'm open to suggestions though!

hellow554 · 2019-03-19T09:21:32Z

I suppose we could instead forward cmp to partial_cmp (+ .unwrap()), but that doesn't seem worth it.

I think it's worth it, because they share the exact same code (except the Option stuff). Code duplication is almost always bad, so why not remove it?

timvermeulen · 2019-03-19T09:32:30Z

As much as I'd like to get rid of all the duplicate code here, I think that particular abstraction is the wrong one to make (in part because it's not obvious to me whether it will always compile the same way, in case the compiler doesn't figure out the None case is impossible). It would also not get rid of the duplicate code in eq, so maybe we should instead try to come up with a function that eq/partial_cmp/cmp can all forward to (but not necessarily in this PR).

Centril · 2019-03-30T13:39:17Z

r? @scottmcm

scottmcm · 2019-04-02T04:33:00Z

Sure, making the way this is implemented match how le/gt/etc are implemented on PartialOrd sounds good to me.

@bors r+ rollup

(We might do something different if we could abort early for lengths, but we can't for general iterators)

bors · 2019-04-02T04:33:02Z

📌 Commit 075b269 has been approved by scottmcm

bors · 2019-04-02T04:34:34Z

💡 This pull request was already approved, no need to approve it again.

There's another pull request that is currently being tested, blocking this pull request: Add 'partition_at_index/_by/_by_key' for slices. #55448

bors · 2019-04-02T04:34:35Z

📌 Commit 075b269 has been approved by scottmcm

…scottmcm Remove duplicated code from Iterator::{ne, lt, le, gt, ge} This PR delegates `Iterator::ne` to `Iterator::eq` and `Iterator::{lt, le, gt, ge}` to `Iterator::partial_cmp`. Oddly enough, this change actually simplifies the generated assembly [in some cases](https://rust.godbolt.org/z/riBtNe), although I don't understand assembly well enough to see if the longer assembly is doing something clever. I also added two extremely simple benchmarks: ``` // before test iter::bench_lt ... bench: 98,404 ns/iter (+/- 21,008) test iter::bench_partial_cmp ... bench: 62,437 ns/iter (+/- 5,009) // after test iter::bench_lt ... bench: 61,757 ns/iter (+/- 8,770) test iter::bench_partial_cmp ... bench: 62,151 ns/iter (+/- 13,753) ``` I have no idea why the current `lt`/`le`/`gt`/`ge` implementations don't seem to be compiled optimally, but simply having them call `partial_cmp` seems to be an improvement. See rust-lang#44729 for a previous discussion.

@ghost

Rollup of 8 pull requests Successful merges: - #59262 (Remove duplicated code from Iterator::{ne, lt, le, gt, ge}) - #59286 (Refactor async fn return type lowering) - #59444 (Implement useful steps_between for all integers) - #59452 (Speed up rustdoc run a bit) - #59533 (Support allocating iterators with arenas) - #59585 (Fixes for shallow borrows) - #59607 (Renames `EvalErrorKind` to `InterpError`) - #59613 (SGX target: convert a bunch of panics to aborts) Failed merges: - #59630 (Shrink `mir::Statement`.) r? @ghost

timvermeulen added 2 commits March 17, 2019 16:57

Forward Iterator::{ne, lt, le, gt, ge} to Iterator::{eq, partial_cmp}
079d9b1

Add iter::{bench_partial_cmp, bench_lt} benchmarks
6778396

rust-highfive assigned Kimundi Mar 17, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 17, 2019

hellow554 reviewed Mar 18, 2019
View reviewed changes

Simplify Iterator::{lt, gt}
075b269

rust-highfive assigned scottmcm and unassigned Kimundi Mar 30, 2019

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 2, 2019

Centril mentioned this pull request Apr 2, 2019
Rollup of 8 pull requests #59632
Merged

bors merged commit 075b269 into rust-lang:masterApr 2, 2019

Uh oh!

Uh oh!

Conversation

timvermeulen commented Mar 17, 2019

Uh oh!

rust-highfive commented Mar 17, 2019

Uh oh!

hellow554Mar 18, 2019

Choose a reason for hiding this comment

Uh oh!

hellow554Mar 18, 2019

Choose a reason for hiding this comment

Uh oh!

timvermeulenMar 18, 2019

Choose a reason for hiding this comment

Uh oh!

tesujiMar 18, 2019

Choose a reason for hiding this comment

Uh oh!

timvermeulenMar 18, 2019

Choose a reason for hiding this comment

Uh oh!

hellow554 commented Mar 18, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hellow554 commented Mar 18, 2019

Uh oh!

timvermeulen commented Mar 18, 2019

Uh oh!

hellow554 commented Mar 19, 2019

Uh oh!

timvermeulen commented Mar 19, 2019

Uh oh!

Centril commented Mar 30, 2019

Uh oh!

scottmcm commented Apr 2, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bors commented Apr 2, 2019

Uh oh!

bors commented Apr 2, 2019

Uh oh!

bors commented Apr 2, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

hellow554 commented Mar 18, 2019•
edited
Loading

scottmcm commented Apr 2, 2019•
edited
Loading