Add iterator method specialisations to Range* by varkor · Pull Request #47180 · rust-lang/rust

varkor · 2018-01-04T12:41:59Z

Add specialised implementations of max for Range, and last, min and max for RangeInclusive, all of which lead to significant advantages in the generated assembly on x86.

Note that adding specialisations of min and last for Range led to no benefit, and adding sum for Range and RangeInclusive led to type inference issues (though this is possibly still worthwhile considering the performance gain).

This addresses some of the concerns in #39975.

rust-highfive · 2018-01-04T12:42:03Z

r? @withoutboats

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

ollie27 · 2018-01-04T14:43:28Z

+
+ #[inline]
+ fn min(self) -> Option<A> {
+ Some(self.start)


Currently min for RangeFrom will either panic with overflow or loop forever so this is a change of behaviour. As RangeFrom is fundamentally broken right now (#25708) I don't think it's a good idea to specialise methods for it just yet.

Yeah, I was considering not adding this yet. I'll remove it, as it would be better placed in a separate change.

withoutboats · 2018-01-04T23:39:38Z

@bors r? @alexcrichton

alexcrichton · 2018-01-05T15:33:50Z

+
+ #[inline]
+ fn max(self) -> Option<A> {
+ Some(self.end)


Shouldn't this and last return None once the iterator is exhausted?

Yep, it definitely should.

bluss · 2018-01-05T18:05:37Z

You can use either self.next() or self.next_back() in all of these to get the right Some/Noneness

alexcrichton · 2018-01-05T20:42:47Z

Thanks! Although now that it's been brought up, @bluss's suggestion sounds like a great way to future-proof these as well. Want to implement it via that?

alexcrichton · 2018-01-08T15:51:35Z

@bors: r+

Nice!

bors · 2018-01-08T15:51:36Z

📌 Commit 2d83343 has been approved by alexcrichton

varkor · 2018-01-08T16:32:41Z

I've been doing some benchmarks, and I must have overlooked something before, because min for Range does lead to performance gains, although last does seem not to. It might be worth implementing them both for consistency, though. I'm going to push another commit adding them — sorry for the confusion!

Something odd is going on with optimisation for min — I'll look into it, but it's easier to leave it for a follow-up commit.

bors · 2018-01-08T17:26:13Z

⌛ Testing commit 2d83343 with merge c209f57...

Add iterator method specialisations to Range* Add specialised implementations of `max` for `Range`, and `last`, `min` and `max` for `RangeInclusive`, all of which lead to significant advantages in the generated assembly on x86. Note that adding specialisations of `min` and `last` for `Range` led to no benefit, and adding `sum` for `Range` and `RangeInclusive` led to type inference issues (though this is possibly still worthwhile considering the performance gain). This addresses some of the concerns in #39975.

bors · 2018-01-08T20:26:50Z

💔 Test failed - status-appveyor

alexcrichton · 2018-01-08T21:12:54Z

@bors: retry

Spurious appveyor 32-bit test timeouts #46903

bors · 2018-01-08T23:27:53Z

⌛ Testing commit 2d83343 with merge 2248d5b...

Add iterator method specialisations to Range* Add specialised implementations of `max` for `Range`, and `last`, `min` and `max` for `RangeInclusive`, all of which lead to significant advantages in the generated assembly on x86. Note that adding specialisations of `min` and `last` for `Range` led to no benefit, and adding `sum` for `Range` and `RangeInclusive` led to type inference issues (though this is possibly still worthwhile considering the performance gain). This addresses some of the concerns in #39975.

bors · 2018-01-09T02:28:29Z

💔 Test failed - status-appveyor

alexcrichton · 2018-01-09T05:34:11Z

@bors: retry

Spurious appveyor 32-bit test timeouts #46903

bors · 2018-01-09T10:02:32Z

⌛ Testing commit 2d83343 with merge f67c277525fa3fd4fad5624b058b411ee495e1ff...

bors · 2018-01-09T13:03:11Z

💔 Test failed - status-appveyor

kennytm · 2018-01-09T13:07:20Z

@bors r-

I think 3 consecutive timeouts indicate there's some performance problem here (or you are really unlucky). @varkor could you investigate the "min" optimization mentioned #47180 (comment)?

(We could re-r+ if it turns out there's nothing more to do)

varkor · 2018-01-09T19:42:57Z

The min method is now taking ~1 n/s to execute, on par with the other methods. I can't see this being the issue though, because each of the changes in the previous commit only improved the efficiency of the iterator methods. I don't see how any of these changes could be a regression.

alexcrichton · 2018-01-10T01:43:40Z

@bors: r+

bors · 2018-01-10T01:43:41Z

📌 Commit 919d643 has been approved by alexcrichton

bors · 2018-01-10T09:28:31Z

⌛ Testing commit 919d643 with merge 9959d8a...

Add iterator method specialisations to Range* Add specialised implementations of `max` for `Range`, and `last`, `min` and `max` for `RangeInclusive`, all of which lead to significant advantages in the generated assembly on x86. Note that adding specialisations of `min` and `last` for `Range` led to no benefit, and adding `sum` for `Range` and `RangeInclusive` led to type inference issues (though this is possibly still worthwhile considering the performance gain). This addresses some of the concerns in #39975.

bors · 2018-01-10T12:29:02Z

💔 Test failed - status-appveyor

kennytm · 2018-01-11T08:16:27Z

@bors retry

bors · 2018-01-11T12:23:00Z

⌛ Testing commit 919d643 with merge 73ac5d6...

Add iterator method specialisations to Range* Add specialised implementations of `max` for `Range`, and `last`, `min` and `max` for `RangeInclusive`, all of which lead to significant advantages in the generated assembly on x86. Note that adding specialisations of `min` and `last` for `Range` led to no benefit, and adding `sum` for `Range` and `RangeInclusive` led to type inference issues (though this is possibly still worthwhile considering the performance gain). This addresses some of the concerns in #39975.

bors · 2018-01-11T15:19:32Z

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 73ac5d6 to master...

Calling `min` on `RangeFrom` currently causes an infinite loop. Although other methods such as `max` also result in an infinite loop, it is strictly incorrect in the case of `min`. Adding a specialisation fixes this. Separated from rust-lang#47180 because this technically changes behaviour; it’s not just an optimisation, so it’s a little different.

varkor added 5 commits January 4, 2018 00:17

Add min and max specialisations for RangeInclusive
680ebf7

Add min specialisation for RangeFrom and last for RangeInclusive
3d9c36f

Add max and sum specialisations for Range
29e6b10

Remove RangeInclusive::sum
087bffa

Add tests for specialised Range iter methods
f3baa85

rust-highfive assigned withoutboats Jan 4, 2018

ollie27 reviewed Jan 4, 2018
View reviewed changes

Remove min from RangeFrom
439beab

alexcrichton added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 4, 2018

rust-highfive assigned alexcrichton and unassigned withoutboats Jan 4, 2018

alexcrichton reviewed Jan 5, 2018
View reviewed changes

Fix behaviour after iterator exhaustion
c23d450

Use next and next_back
2d83343

kennytm added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 9, 2018

Add min and last specialisations for Range
919d643

kennytm added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 9, 2018

bors merged commit 919d643 into rust-lang:masterJan 11, 2018

varkor mentioned this pull request Jan 11, 2018
Range* should overrride more methods of Iterator #39975
Open

varkor mentioned this pull request Jan 11, 2018
Add min specialisation for RangeFrom#47362
Closed

Uh oh!

Uh oh!

Conversation

varkor commented Jan 4, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rust-highfive commented Jan 4, 2018

Uh oh!

ollie27Jan 4, 2018

Choose a reason for hiding this comment

Uh oh!

varkorJan 4, 2018

Choose a reason for hiding this comment

Uh oh!

withoutboats commented Jan 4, 2018

Uh oh!

alexcrichtonJan 5, 2018

Choose a reason for hiding this comment

Uh oh!

varkorJan 5, 2018

Choose a reason for hiding this comment

Uh oh!

bluss commented Jan 5, 2018

Uh oh!

alexcrichton commented Jan 5, 2018

Uh oh!

alexcrichton commented Jan 8, 2018

Uh oh!

bors commented Jan 8, 2018

Uh oh!

varkor commented Jan 8, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bors commented Jan 8, 2018

Uh oh!

bors commented Jan 8, 2018

Uh oh!

alexcrichton commented Jan 8, 2018

Uh oh!

bors commented Jan 8, 2018

Uh oh!

bors commented Jan 9, 2018

Uh oh!

alexcrichton commented Jan 9, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bors commented Jan 9, 2018

Uh oh!

bors commented Jan 9, 2018

Uh oh!

kennytm commented Jan 9, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

varkor commented Jan 9, 2018

Uh oh!

alexcrichton commented Jan 10, 2018

Uh oh!

bors commented Jan 10, 2018

Uh oh!

bors commented Jan 10, 2018

Uh oh!

bors commented Jan 10, 2018

Uh oh!

kennytm commented Jan 11, 2018

Uh oh!

bors commented Jan 11, 2018

Uh oh!

bors commented Jan 11, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

varkor commented Jan 4, 2018•
edited
Loading

varkor commented Jan 8, 2018•
edited
Loading

alexcrichton commented Jan 9, 2018•
edited
Loading

kennytm commented Jan 9, 2018•
edited
Loading