Optimize slice.{r}position result bounds check #47333

arthurprs · 2018-01-10T19:37:31Z

Second attempt of #45501
Fixes #45964

kennytm · 2018-01-10T20:30:11Z

src/libcore/slice/mod.rs

+            {
+                // The addition might panic on overflow
+                self.try_fold(0, move |i, x| {
+                    if predicate(x) { LoopState::Break(i) }


failed to resolve. Use of undeclared type or module LoopState

I'm changing it to use Result<,>'s instead.

kennytm · 2018-01-10T20:31:30Z

r? @dtolnay

dtolnay

Thanks for sticking with this!

dtolnay · 2018-01-10T20:45:06Z

@bors r+

bors · 2018-01-10T20:45:06Z

📋 Looks like this PR is still in progress, ignoring approval

kennytm · 2018-01-10T20:47:01Z

Borrowck error. We haven't enabled NLL yet 😄.

[00:03:08] error[E0502]: cannot borrow `self` as immutable because `*self` is also borrowed as mutable
[00:03:08]     --> libcore/slice/mod.rs:1242:32
[00:03:08]      |
[00:03:08] 1242 |                 self.try_rfold(self.len(), move |i, x| {
[00:03:08]      |                 ----           ^^^^ immutable borrow occurs here
[00:03:08]      |                 |
[00:03:08]      |                 mutable borrow occurs here
[00:03:08] ...
[00:03:08] 1246 |                 }).err()
[00:03:08]      |                  - mutable borrow ends here
[00:03:08] ...
[00:03:08] 1422 | iterator!{struct Iter -> *const T, &'a T, make_ref}
[00:03:08]      | --------------------------------------------------- in this macro invocation

arthurprs · 2018-01-10T20:50:06Z

I see 😢 going for the 5th try now.

kennytm · 2018-01-11T08:26:00Z

@bors r=dtolnay

bors · 2018-01-11T08:26:01Z

📌 Commit 1699dc4 has been approved by dtolnay

scottmcm · 2018-01-12T05:52:09Z

src/libcore/slice/mod.rs

+                Self: Sized,
+                P: FnMut(Self::Item) -> bool,
+            {
+                // The addition might panic on overflow


I think this needs either Add::add instead of + or #[rustc_inherit_overflow_checks].

Also, it's a shame there's no way to "call base" here. Would it be worth making a pub(crate) helper here to avoid duplicating the logic? Or using specialization to do the assume for all Self: TrustedLen?

I don't think the duplication is a concern as these are really small. Having a helper would probably end up adding more lines than saving.

Good points about the overflow and specialization though. I have to test if the assume survives the abstraction/inlining.

I couldn't make it work with TrustedLen 😞 https://godbolt.org/g/bFA3tk

Thanks for giving it a try! Good to see this get in 🙂

arthurprs · 2018-01-12T21:59:15Z

I added a #[rustc_inherit_overflow_checks] as per @scottmcm review

kennytm · 2018-01-17T10:18:14Z

@bors r=dtolnay

bors · 2018-01-17T10:18:15Z

📌 Commit 0b56ab0 has been approved by dtolnay

…k, r=dtolnay Optimize slice.{r}position result bounds check Second attempt of rust-lang#45501 Fixes rust-lang#45964 Demo: https://godbolt.org/g/N4mBHp

Rollup of 21 pull requests - Successful merges: #47302, #47333, #47387, #47404, #47407, #47426, #47427, #47436, #47444, #47456, #47458, #47467, #47479, #47481, #47483, #47487, #47497, #47498, #47505, #47509, #47514 - Failed merges:

arthurprs · 2018-01-25T22:50:45Z

Apparently this didn't work 😢 Demo: https://godbolt.org/g/MXg2ae

It has to do with self.len() in this PR calling the iterator len instead of the slice len.

Using self.as_slice().len() does the trick. Demo: https://godbolt.org/g/P8q5aZ I'll make the fix PR later.

Use the slice length to hint the optimizer about iter.position result Using the len of the iterator doesn't give the same result. That's also why we can't generalize it to all TrustedLen iterators. Problem demo: https://godbolt.org/g/MXg2ae Fix demo: https://godbolt.org/g/P8q5aZ Second attempt of #47333 Third attempt of #45501 Fixes #45964

Removed the `assume()` which we assumed is the cause of misoptimization in issue rust-lang#48116.

@eddyb

Try to fix 48116 and 48192 The bug #48116 happens because of a misoptimization of the `import_path_to_string` function, where a `names` slice is empty but the `!names.is_empty()` branch is executed. https://github.com/rust-lang/rust/blob/4d2d3fc5dadf894a8ad709a5860a549f2c0b1032/src/librustc_resolve/resolve_imports.rs#L1015-L1042 Yesterday, @eddyb had locally reproduced the bug, and [came across the `position` function](https://mozilla.logbot.info/rust-infra/20180214#c14296834) where the `assume()` call is found to be suspicious. We have *not* concluded that this `assume()` causes #48116, but given [the reputation of `assume()`](#45501 (comment)), this seems higher relevant. Here we try to see if commenting it out can fix the errors. Later @alexcrichton has bisected and found a potential bug [in the LLVM side](#48116 (comment)). We are currently testing if reverting that LLVM commit is enough to stop the bug. If true, this PR can be reverted (keep the `assume()`) and we could backport the LLVM patch instead. (This PR also includes an earlier commit from #48127 for help debugging ICE happening in compile-fail/parse-fail tests.) The PR also reverts #48059, which seems to cause #48192. r? @alexcrichton cc @eddyb, @arthurprs (#47333)

arthurprs force-pushed the iter-position-bounds-check branch from 30e7c6c to bd4ead6 Compare January 10, 2018 19:39

arthurprs changed the title ~~Optimize slice.{r}position result bounds check~~ [WIP] Optimize slice.{r}position result bounds check Jan 10, 2018

kennytm reviewed Jan 10, 2018

View reviewed changes

arthurprs force-pushed the iter-position-bounds-check branch from bd4ead6 to 2b77cac Compare January 10, 2018 20:31

rust-highfive assigned dtolnay Jan 10, 2018

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

arthurprs force-pushed the iter-position-bounds-check branch from 2b77cac to b95a7b6 Compare January 10, 2018 20:39

dtolnay approved these changes Jan 10, 2018

View reviewed changes

arthurprs force-pushed the iter-position-bounds-check branch 4 times, most recently from 806b403 to 1699dc4 Compare January 10, 2018 21:00

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 10, 2018

arthurprs changed the title ~~[WIP] Optimize slice.{r}position result bounds check~~ Optimize slice.{r}position result bounds check Jan 10, 2018

kennytm 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 11, 2018

scottmcm reviewed Jan 12, 2018

View reviewed changes

Optimize slice.{r}position result bounds check

0b56ab0

arthurprs force-pushed the iter-position-bounds-check branch from 1699dc4 to 0b56ab0 Compare January 12, 2018 21:58

kennytm mentioned this pull request Jan 17, 2018

Rollup of 21 pull requests #47522

Merged

bors merged commit 0b56ab0 into rust-lang:master Jan 17, 2018

arthurprs mentioned this pull request Jan 26, 2018

Use the slice length to hint the optimizer about iter.position result #47772

Merged

kennytm mentioned this pull request Feb 14, 2018

Try to fix 48116 and 48192 #48209

Merged

kennytm added a commit to kennytm/rust that referenced this pull request Feb 14, 2018

Partially revert rust-lang#47333.

ec36e7e

Removed the `assume()` which we assumed is the cause of misoptimization in issue rust-lang#48116.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimize slice.{r}position result bounds check #47333

Optimize slice.{r}position result bounds check #47333

arthurprs commented Jan 10, 2018 •

edited

Loading

kennytm Jan 10, 2018

arthurprs Jan 10, 2018

kennytm commented Jan 10, 2018

dtolnay left a comment

dtolnay commented Jan 10, 2018

bors commented Jan 10, 2018

kennytm commented Jan 10, 2018 •

edited

Loading

arthurprs commented Jan 10, 2018

kennytm commented Jan 11, 2018

bors commented Jan 11, 2018

scottmcm Jan 12, 2018

arthurprs Jan 12, 2018

arthurprs Jan 12, 2018

scottmcm Jan 17, 2018

arthurprs commented Jan 12, 2018

kennytm commented Jan 17, 2018

bors commented Jan 17, 2018

arthurprs commented Jan 25, 2018 •

edited

Loading

Optimize slice.{r}position result bounds check #47333

Optimize slice.{r}position result bounds check #47333

Conversation

arthurprs commented Jan 10, 2018 • edited Loading

kennytm Jan 10, 2018

Choose a reason for hiding this comment

arthurprs Jan 10, 2018

Choose a reason for hiding this comment

kennytm commented Jan 10, 2018

dtolnay left a comment

Choose a reason for hiding this comment

dtolnay commented Jan 10, 2018

bors commented Jan 10, 2018

kennytm commented Jan 10, 2018 • edited Loading

arthurprs commented Jan 10, 2018

kennytm commented Jan 11, 2018

bors commented Jan 11, 2018

scottmcm Jan 12, 2018

Choose a reason for hiding this comment

arthurprs Jan 12, 2018

Choose a reason for hiding this comment

arthurprs Jan 12, 2018

Choose a reason for hiding this comment

scottmcm Jan 17, 2018

Choose a reason for hiding this comment

arthurprs commented Jan 12, 2018

kennytm commented Jan 17, 2018

bors commented Jan 17, 2018

arthurprs commented Jan 25, 2018 • edited Loading

arthurprs commented Jan 10, 2018 •

edited

Loading

kennytm commented Jan 10, 2018 •

edited

Loading

arthurprs commented Jan 25, 2018 •

edited

Loading