Detailed panic messages for Vec functions #70573

IgorPerikov · 2020-03-30T16:04:34Z

pass indexes to insert, remove, drain, and split_off panic messages

rust-highfive · 2020-03-30T16:04:37Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @LukasKalbertodt (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

RalfJung · 2020-03-30T16:07:24Z

src/liballoc/vec.rs

@@ -992,7 +992,7 @@ impl<T> Vec<T> {
 #[stable(feature = "rust1", since = "1.0.0")]
 pub fn insert(&mut self, index: usize, element: T) {
 let len = self.len();
- assert!(index <= len);
+ assert!(index <= len, "index out of bounds: the len is {} but the index is {}", len, index);


Given that the bounds check for insert functions is different from indexing (<= rather than <), I wonder if it makes sense to also make the error different?

Also, if we want to be really sure this behaves like indexing, we could make core::panicking::panic_bounds_check public and call it (though until the next bootstrap bump, that requires conditional compilation unfortunately).

I am not sure if that's worth it, though.

Given that the bounds check for insert functions is different from indexing (<= rather than <), I wonder if it makes sense to also make the error different?

I was thinking on how to check all three boxes:

don't fall into bloated error message

introduce argument values

keep it consistent with everything else (basically [] call)

Other than that, I think bringing both conditions and parameters might work better. However, if we want to add some human-readable explanation, it gets really big, like that

assert!("index out of bounds, index should be <= len, got {} <= {}", index, len);

Well, now I'm thinking that is has an appropriate size, but then it will require core::panicking::panic_bounds_check to be updated, to bring condition there too

No the condition would have to stay here.

Can you elaborate? I think I didn't get the context

I'm just saying that function must not change.

But in the end it probably is better to just do what you did, and not try to literally share the message with indexing... I was just bringing up the possibility.

src/liballoc/vec.rs

IgorPerikov · 2020-03-31T17:31:59Z

@RalfJung I've decided to follow approach of having detailed message so it's easy to understand:

what kind of index is that
what was the passed values
what assertion should be satisfied

LukasKalbertodt

Thanks for your PR! I have two main comments:

I don't quite like the second part of your error messages. The first part (stating the condition) is nice. But the second part is not explicit enough for me. For example, instead of

insertion index should be <= len, got {} <= {}", index, len

... I would write something like this:

insertion index should be <= len, but the insertion index is {} and len is {}", index, len

This, of course, is not a particularly nice English as we will repeat the names (indices/len) a lot. I think clarity and explicitness in these error messages is more important than nice English, but maybe we can improve it still. How about one of these?

"assertion `index <= len` failed: insertion index is {} and len is {}"
"index out of bounds: insertion index is {} and len is {}, but `index <= len` should hold"

My second concern is performance. These are methods potentially called in hot loops. Panicking with a static string generates way less code than using the fmt machinery. And making those methods larger influences icache and inlining behavior, having a potentially large impact on performance. This could be (at least partially) mitigated by created special #[cold] functions that simply panic. In any case, I'd like to check with @rust-timer.

LukasKalbertodt · 2020-03-31T21:54:04Z

Ok, let's try talking to a new bot ^_^

@bors try @rust-timer queue

rust-timer · 2020-03-31T21:54:05Z

Awaiting bors try build completion

bors · 2020-03-31T21:54:17Z

⌛ Trying commit e67919adffdb23fc074b0239a4e2ceee15dd1a1f with merge 2b82da36cfffe01ebec5f1e184def6270b747859...

bors · 2020-04-01T00:11:59Z

☀️ Try build successful - checks-azure
Build commit: 2b82da36cfffe01ebec5f1e184def6270b747859 (2b82da36cfffe01ebec5f1e184def6270b747859)

rust-timer · 2020-04-01T00:12:01Z

Queued 2b82da36cfffe01ebec5f1e184def6270b747859 with parent 75ff311, future comparison URL.

IgorPerikov · 2020-04-01T05:21:46Z

I prefer first one among options you mentioned - it's more concise and as informative as second.

"assertion index <= len failed: insertion index is {} and len is {}"

I've been thinking about another approach as well:
removal index {} should be < len {}
and maybe add extra parentheses around parameter values

I saw this idiom somewhere outside Rust ecosystem and I like it, what are your thoughts about it?

LukasKalbertodt · 2020-04-01T07:22:29Z

Slightly modified?

removal index (is {}) should be < len (is {})

I think the "is" make it slightly clearer. What do you think? I'd be fine with that.

EDIT: Although... this one isn't clearly stating a problem. The form "X should Y, but" conveys a problem, as does "assertion X failed". The form "X should Y" only states the condition and the user has to "deduce" that that condition was violated. Mhhhh

IgorPerikov · 2020-04-01T10:08:44Z

I think the "is" make it slightly clearer. What do you think? I'd be fine with that.

I like it either, it's that I used to the template w/o 'is', but I agree that would be better in general.

this one isn't clearly stating a problem

Isn't the fact that panic occurred states it? Maybe a bit implicit, but still

LukasKalbertodt · 2020-04-01T16:28:47Z

Isn't the fact that panic occurred states it? Maybe a bit implicit, but still

Just to be clear, this is just nitpicking on my side. I'm fine with the version I posted in my last comment (with "is"). Just take any of the ones I labeled as "fine" ^_^

IgorPerikov · 2020-04-01T16:31:42Z

Ok, got it ^^
How rust-timer went? I can't understand the outcome of comparison from the result page

LukasKalbertodt · 2020-04-01T16:47:16Z

How rust-timer went? I can't understand the outcome of comparison from the result page

I just noticed it already shows results. I would have expected another comment of the bot here, telling us the comparison is done. Anyway...

The results don't look too good I think. Basically every test case slowed down by between 0.1% and 0.8%. While this sounds minor, if this change is really caused by this change, I'm not particularly comfortable merging this as is. To put the numbers in perspective: PRs that improve compile times by half a percent across all test cases are rare and a really great achievement.

Now I'm not sure if this change could also be caused by something else. I will ask someone :)

RalfJung · 2020-04-01T16:57:29Z

A possible experiment would be go back to my weird idea from earlier, and call panic_bounds_check (which is already optimized for being called in hot loops).

So the checks would become something like

if index >= len { panic_bounds_check(index, len) }

Except this will only work on not(bootstrap) builds... the next bootstrap bump should happen around end of April, this will become easier then. Before that, it will have to be

if index >= len {
  #[cfg(not(bootstrap))]
  panic_bounds_check(index, len);
  #[cfg(bootstrap)]
  panic_bounds_check(Location::caller(), index, len);
}

LukasKalbertodt · 2020-04-01T16:58:51Z

I just talked to @Centril and they also think it's a pretty clear regression, performance wise.

@IgorPerikov Could you rewrite this so that the formatting is done in a separate #[cold] function? For example, for insert, replace the assert!(...) with:

#[cold]
#[inline(never)]  // <-- probably implied by `#[cold]` but no one knows for sure...
fn assert_failed(index: usize, len: usize) -> ! {
    panic!("insertion index (is {}) should be <= len (is {})", index, len);
}

if index > len {
    assert_failed(index, len);
}

(The assert_failed definition within the insert function.)

With that changed for all methods, we can run the perf test again to see if that improved things.

RalfJung · 2020-04-01T17:00:07Z

@LukasKalbertodt potentially assert_failed should have #[track_caller] for better panic locations.

LukasKalbertodt · 2020-04-01T17:01:35Z

@RalfJung Good idea!

As we posted the previous comments at the same time: your suggestion with panic_bounds_check is also just a #[cold] function, right? So by writing custom functions we can provide better error messages, right?

RalfJung · 2020-04-01T17:05:15Z

As we posted the previous comments at the same time: your suggestion with panic_bounds_check is also just a #[cold] function, right? So by writing custom functions we can provide better error messages, right?

Exactly. Specifically, this is the cold function that is used to raise the panic on out-of-bounds slice/array/vector access. So it makes sense to call that whenever the error message should be exactly the same as for indexing.

IgorPerikov · 2020-04-01T17:56:52Z

I definitely lack a lot of fundamental knowledge about how Rust works under the hood. One of the things I don't understand is this statement:

and call 'panic_bounds_check' (which is already optimized for being called in hot loops).

How is that possible if #[cold] means suggests that the attributed function is unlikely to be called? So attribute name and its description implies reverse.

I also don't get it, how can we reuse panic_bounds_check for cases, where we:

check that <= instead of <, decreasing/increasing one of the parameters before passing in would ruin panic message
want customized message, like for drain, where start and end are being compared, not the length, and also mentioning removal/insertion instead of generic Out of bounds

Maybe it's getting too hard for a novice like me 🤷‍♂

LukasKalbertodt · 2020-04-01T18:13:25Z

I don't understand is this statement:

and call 'panic_bounds_check' (which is already optimized for being called in hot loops).

I think what @RalfJung was saying is: it is optimized to be inside hot loops without being actually called at runtime.

Let me explain #[cold] a bit more. It's a hint for the optimizer with the goal of making the code surrounding the call to a cold function faster. We specifically do not optimize the speed of calling the cold function. In fact, we most certainly make calling that function slower, but that's ok for us because it is called so rarely. So optimizing the surrounding code is more important. This is achieved in mainly two ways:

The cold function won't get inlined. This means that the calling function (e.g. insert) gets smaller. (By "smaller", I mean that it contains fewer instructions in the resulting machine code.) That in turn is important for the instruction cache (icache) and more importantly, it makes the calling function more likely to get inlined itself.
The assembly is laid out differently in the calling function (usually placing the call to the cold function at the very end).

I also don't get it, how can we reuse panic_bounds_check for cases, where we: ...

Yes, it would just be useful for the cases with the exact behavior and error message we want, which is only remove, I think. That's why I suggested we write out own cold functions to panic.

Maybe it's getting too hard for a novice like me 🤷‍♂️

Nah, don't worry, you got this. Just ask if anything is unclear. For now, just do what I described in this comment. If anything about that is unclear, just ask. Also feel free to contact me via Zulip or Discord.

RalfJung · 2020-04-01T18:29:55Z

I think what @RalfJung was saying is: it is optimized to be inside hot loops without being actually called at runtime.

Indeed I do, sorry for not being clearer.

rust-highfive · 2020-04-01T19:35:59Z

The job mingw-check of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

2020-04-01T19:31:13.9610153Z ========================== Starting Command Output ===========================
2020-04-01T19:31:13.9613507Z [command]/bin/bash --noprofile --norc /home/vsts/work/_temp/966cf2cc-7524-4150-9c07-ecd98eb4ba14.sh
2020-04-01T19:31:13.9613926Z 
2020-04-01T19:31:13.9618359Z ##[section]Finishing: Disable git automatic line ending conversion
2020-04-01T19:31:13.9636294Z ##[section]Starting: Checkout rust-lang/rust@refs/pull/70573/merge to s
2020-04-01T19:31:13.9639652Z Task         : Get sources
2020-04-01T19:31:13.9639964Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.
2020-04-01T19:31:13.9640282Z Version      : 1.0.0
2020-04-01T19:31:13.9640488Z Author       : Microsoft
---
2020-04-01T19:31:15.2177567Z ##[command]git remote add origin https://github.com/rust-lang/rust
2020-04-01T19:31:15.2183188Z ##[command]git config gc.auto 0
2020-04-01T19:31:15.2187335Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2020-04-01T19:31:15.2190550Z ##[command]git config --get-all http.proxy
2020-04-01T19:31:15.2197047Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/70573/merge:refs/remotes/pull/70573/merge
---
2020-04-01T19:33:36.2996502Z  ---> 3fc1b512c57b
2020-04-01T19:33:36.2998296Z Step 6/7 : ENV RUN_CHECK_WITH_PARALLEL_QUERIES 1
2020-04-01T19:33:36.3001706Z  ---> Using cache
2020-04-01T19:33:36.3002828Z  ---> 5ee4295733f4
2020-04-01T19:33:36.3005898Z Step 7/7 : ENV SCRIPT python2.7 ../x.py test src/tools/expand-yaml-anchors &&            python2.7 ../x.py check --target=i686-pc-windows-gnu --host=i686-pc-windows-gnu &&            python2.7 ../x.py build --stage 0 src/tools/build-manifest &&            python2.7 ../x.py test --stage 0 src/tools/compiletest &&            python2.7 ../x.py test src/tools/tidy &&            /scripts/validate-toolstate.sh
2020-04-01T19:33:36.3011862Z  ---> 3d07a0fa42fe
2020-04-01T19:33:36.3046075Z Successfully built 3d07a0fa42fe
2020-04-01T19:33:36.3096076Z Successfully tagged rust-ci:latest
2020-04-01T19:33:36.3354891Z Built container sha256:3d07a0fa42feb5754fc13bb2f7010ebe13e4b8b8cdbebed0c75d8da320c8c8ad
2020-04-01T19:33:36.3354891Z Built container sha256:3d07a0fa42feb5754fc13bb2f7010ebe13e4b8b8cdbebed0c75d8da320c8c8ad
2020-04-01T19:33:36.3367424Z Looks like docker image is the same as before, not uploading
2020-04-01T19:33:43.0717094Z [CI_JOB_NAME=mingw-check]
2020-04-01T19:33:43.0998324Z [CI_JOB_NAME=mingw-check]
2020-04-01T19:33:43.1027888Z == clock drift check ==
2020-04-01T19:33:43.1035243Z   local time: Wed Apr  1 19:33:43 UTC 2020
2020-04-01T19:33:43.3681318Z   network time: Wed, 01 Apr 2020 19:33:43 GMT
2020-04-01T19:33:43.3707293Z Starting sccache server...
2020-04-01T19:33:43.4545142Z configure: processing command line
2020-04-01T19:33:43.4545355Z configure: 
2020-04-01T19:33:43.4546335Z configure: rust.parallel-compiler := True
---
2020-04-01T19:35:23.1354747Z      |
2020-04-01T19:35:23.1355533Z 1044 |             removal_assert_failed(index, len);
2020-04-01T19:35:23.1356561Z      |             ^^^^^^^^^^^^^^^^^^^^^ not found in this scope
2020-04-01T19:35:23.1357050Z 
2020-04-01T19:35:23.1370328Z error[E0425]: cannot find function `start_lte_end_drain_assert_failed` in this scope
2020-04-01T19:35:23.1371603Z      |
2020-04-01T19:35:23.1371603Z      |
2020-04-01T19:35:23.1372224Z 1310 |             start_lte_end_drain_assert_failed(start, end);
2020-04-01T19:35:23.1373458Z 
2020-04-01T19:35:23.1373458Z 
2020-04-01T19:35:23.1384602Z error[E0425]: cannot find function `end_lte_len_drain_assert_failed` in this scope
2020-04-01T19:35:23.1386220Z      |
2020-04-01T19:35:23.1386220Z      |
2020-04-01T19:35:23.1386836Z 1313 |             end_lte_len_drain_assert_failed(end, len);
2020-04-01T19:35:23.1388054Z 
2020-04-01T19:35:23.1398879Z error[E0425]: cannot find function `split_off_assert_failed` in this scope
2020-04-01T19:35:23.1399498Z     --> src/liballoc/vec.rs:1417:13
2020-04-01T19:35:23.1399934Z      |
2020-04-01T19:35:23.1399934Z      |
2020-04-01T19:35:23.1400460Z 1417 |             split_off_assert_failed(at, self.len());
2020-04-01T19:35:23.1401513Z 
2020-04-01T19:35:23.9744524Z     Checking rustc-demangle v0.1.16
2020-04-01T19:35:24.1351328Z error: aborting due to 5 previous errors
2020-04-01T19:35:24.1351670Z 
---
2020-04-01T19:35:24.2283810Z expected success, got: exit code: 101
2020-04-01T19:35:24.2293449Z failed to run: /checkout/obj/build/bootstrap/debug/bootstrap check
2020-04-01T19:35:24.2293817Z Build completed unsuccessfully in 0:01:40
2020-04-01T19:35:24.2342724Z == clock drift check ==
2020-04-01T19:35:24.2357330Z   local time: Wed Apr  1 19:35:24 UTC 2020
2020-04-01T19:35:24.3095638Z   network time: Wed, 01 Apr 2020 19:35:24 GMT
2020-04-01T19:35:25.3174933Z 
2020-04-01T19:35:25.3174933Z 
2020-04-01T19:35:25.3241209Z ##[error]Bash exited with code '1'.
2020-04-01T19:35:25.3254463Z ##[section]Finishing: Run build
2020-04-01T19:35:25.3306137Z ##[section]Starting: Checkout rust-lang/rust@refs/pull/70573/merge to s
2020-04-01T19:35:25.3311433Z Task         : Get sources
2020-04-01T19:35:25.3311764Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.
2020-04-01T19:35:25.3312086Z Version      : 1.0.0
2020-04-01T19:35:25.3312301Z Author       : Microsoft
2020-04-01T19:35:25.3312301Z Author       : Microsoft
2020-04-01T19:35:25.3312635Z Help         : [More Information](https://go.microsoft.com/fwlink/?LinkId=798199)
2020-04-01T19:35:25.3313042Z ==============================================================================
2020-04-01T19:35:25.6617026Z Cleaning any cached credential from repository: rust-lang/rust (GitHub)
2020-04-01T19:35:25.6659457Z ##[section]Finishing: Checkout rust-lang/rust@refs/pull/70573/merge to s
2020-04-01T19:35:25.6734970Z Cleaning up task key
2020-04-01T19:35:25.6736190Z Start cleaning up orphan processes.
2020-04-01T19:35:25.7085319Z Terminate orphan process: pid (6066) (python)
2020-04-01T19:35:25.7110489Z ##[section]Finishing: Finalize Job

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @rust-lang/infra. (Feature Requests)

rust-highfive · 2020-04-01T19:56:03Z

The job mingw-check of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

2020-04-01T19:50:30.0082967Z ========================== Starting Command Output ===========================
2020-04-01T19:50:30.0100714Z [command]/bin/bash --noprofile --norc /home/vsts/work/_temp/d0f3d78a-a7de-49d7-a66c-4b811c55b7fa.sh
2020-04-01T19:50:30.0265424Z 
2020-04-01T19:50:30.0315696Z ##[section]Finishing: Disable git automatic line ending conversion
2020-04-01T19:50:30.0330830Z ##[section]Starting: Checkout rust-lang/rust@refs/pull/70573/merge to s
2020-04-01T19:50:30.0333483Z Task         : Get sources
2020-04-01T19:50:30.0333699Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.
2020-04-01T19:50:30.0333944Z Version      : 1.0.0
2020-04-01T19:50:30.0334085Z Author       : Microsoft
---
2020-04-01T19:50:30.8324658Z ##[command]git remote add origin https://github.com/rust-lang/rust
2020-04-01T19:50:30.8333908Z ##[command]git config gc.auto 0
2020-04-01T19:50:30.8340342Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2020-04-01T19:50:30.8346111Z ##[command]git config --get-all http.proxy
2020-04-01T19:50:30.8355481Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/70573/merge:refs/remotes/pull/70573/merge
---
2020-04-01T19:52:32.6566679Z  ---> 3fc1b512c57b
2020-04-01T19:52:32.6567059Z Step 6/7 : ENV RUN_CHECK_WITH_PARALLEL_QUERIES 1
2020-04-01T19:52:32.6567364Z  ---> Using cache
2020-04-01T19:52:32.6567625Z  ---> 5ee4295733f4
2020-04-01T19:52:32.6568764Z Step 7/7 : ENV SCRIPT python2.7 ../x.py test src/tools/expand-yaml-anchors &&            python2.7 ../x.py check --target=i686-pc-windows-gnu --host=i686-pc-windows-gnu &&            python2.7 ../x.py build --stage 0 src/tools/build-manifest &&            python2.7 ../x.py test --stage 0 src/tools/compiletest &&            python2.7 ../x.py test src/tools/tidy &&            /scripts/validate-toolstate.sh
2020-04-01T19:52:32.6570039Z  ---> 3d07a0fa42fe
2020-04-01T19:52:32.6570214Z Successfully built 3d07a0fa42fe
2020-04-01T19:52:32.6570565Z Successfully tagged rust-ci:latest
2020-04-01T19:52:32.6818010Z Built container sha256:3d07a0fa42feb5754fc13bb2f7010ebe13e4b8b8cdbebed0c75d8da320c8c8ad
2020-04-01T19:52:32.6818010Z Built container sha256:3d07a0fa42feb5754fc13bb2f7010ebe13e4b8b8cdbebed0c75d8da320c8c8ad
2020-04-01T19:52:32.6835534Z Looks like docker image is the same as before, not uploading
2020-04-01T19:52:40.8101703Z [CI_JOB_NAME=mingw-check]
2020-04-01T19:52:40.8364286Z [CI_JOB_NAME=mingw-check]
2020-04-01T19:52:40.8390379Z == clock drift check ==
2020-04-01T19:52:40.8399979Z   local time: Wed Apr  1 19:52:40 UTC 2020
2020-04-01T19:52:40.8780779Z   network time: Wed, 01 Apr 2020 19:52:40 GMT
2020-04-01T19:52:40.8804191Z Starting sccache server...
2020-04-01T19:52:40.9643825Z configure: processing command line
2020-04-01T19:52:40.9644222Z configure: 
2020-04-01T19:52:40.9645170Z configure: rust.parallel-compiler := True
---
2020-04-01T19:54:26.2471237Z 
2020-04-01T19:54:26.2526966Z error[E0282]: type annotations needed
2020-04-01T19:54:26.2546564Z     --> src/liballoc/vec.rs:1310:13
2020-04-01T19:54:26.2548182Z      |
2020-04-01T19:54:26.2550015Z 1310 |             Vec::start_lte_end_drain_assert_failed(start, end);
2020-04-01T19:54:26.2552117Z 
2020-04-01T19:54:26.2567611Z error[E0282]: type annotations needed
2020-04-01T19:54:26.2586891Z     --> src/liballoc/vec.rs:1417:13
2020-04-01T19:54:26.2587679Z      |
2020-04-01T19:54:26.2587679Z      |
2020-04-01T19:54:26.2589147Z 1417 |             Vec::split_off_assert_failed(at, self.len());
2020-04-01T19:54:26.2590504Z 
2020-04-01T19:54:26.3306012Z error: aborting due to 4 previous errors
2020-04-01T19:54:26.3306339Z 
2020-04-01T19:54:26.3306815Z For more information about this error, try `rustc --explain E0282`.
---
2020-04-01T19:54:26.3695875Z expected success, got: exit code: 101
2020-04-01T19:54:26.3699277Z failed to run: /checkout/obj/build/bootstrap/debug/bootstrap check
2020-04-01T19:54:26.3699581Z Build completed unsuccessfully in 0:01:45
2020-04-01T19:54:26.3749259Z == clock drift check ==
2020-04-01T19:54:26.3761015Z   local time: Wed Apr  1 19:54:26 UTC 2020
2020-04-01T19:54:26.6396190Z   network time: Wed, 01 Apr 2020 19:54:26 GMT
2020-04-01T19:54:27.5478059Z 
2020-04-01T19:54:27.5478059Z 
2020-04-01T19:54:27.5543943Z ##[error]Bash exited with code '1'.
2020-04-01T19:54:27.5554862Z ##[section]Finishing: Run build
2020-04-01T19:54:27.5606772Z ##[section]Starting: Checkout rust-lang/rust@refs/pull/70573/merge to s
2020-04-01T19:54:27.5611654Z Task         : Get sources
2020-04-01T19:54:27.5612231Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.
2020-04-01T19:54:27.5612528Z Version      : 1.0.0
2020-04-01T19:54:27.5613122Z Author       : Microsoft
2020-04-01T19:54:27.5613122Z Author       : Microsoft
2020-04-01T19:54:27.5613827Z Help         : [More Information](https://go.microsoft.com/fwlink/?LinkId=798199)
2020-04-01T19:54:27.5614436Z ==============================================================================
2020-04-01T19:54:27.8759107Z Cleaning any cached credential from repository: rust-lang/rust (GitHub)
2020-04-01T19:54:27.8800422Z ##[section]Finishing: Checkout rust-lang/rust@refs/pull/70573/merge to s
2020-04-01T19:54:27.8876076Z Cleaning up task key
2020-04-01T19:54:27.8877367Z Start cleaning up orphan processes.
2020-04-01T19:54:27.9046197Z Terminate orphan process: pid (5789) (python)
2020-04-01T19:54:27.9175398Z ##[section]Finishing: Finalize Job

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @rust-lang/infra. (Feature Requests)

IgorPerikov · 2020-04-02T15:06:18Z

goes up 2.4%

it means improvement?

RalfJung · 2020-04-02T15:07:07Z

No, runtime goes up by 2.4%.

LukasKalbertodt · 2020-04-02T16:37:51Z

@nnethercote @eddyb @Zoxc Could someone of you maybe help us interpret these perf results? So I guess if we have any measurable regression, we probably don't want to merge this. We are just not sure if this is a regression.

eddyb · 2020-04-02T16:45:15Z

Regression with "clean" but not "baseline incremental" is likely noise, incremental should be strictly slower, and the regression in regression-31157-opt is also likely noise (it's entirely within LLVM).

If I had to guess, this PR doesn't observably change the performance of rustc.

nnethercote · 2020-04-02T23:00:49Z

The 2.6% html5ever-opt-clean regression is the only thing that concerns me. It might be real, it might be noise, but it's not enough to block landing, IMO.

eddyb · 2020-04-02T23:11:09Z

@nnethercote That's the one I was saying I don't believe is real because I would expect "baseline incremental" to regress in the same way as "clean", and it doesn't.

RalfJung · 2020-04-05T23:15:32Z

My PR changing swap_remove landed; could you rebase this and make its panic handling consistent with the rest?

LukasKalbertodt · 2020-04-06T07:11:49Z

Concerning performance: I guess we can land this PR then. If it turns out to have a notably speed decrease in practice, we can still revert or tweak it. Coincidentally, a while back, I wrote a few benchmarks that make heavy use of Vec::insert. I can run those benchmarks once this PR is released as nightly.

RalfJung · 2020-04-06T07:29:16Z

Agreed, but let's also do @bors rollup=never for easier bisecting.

IgorPerikov · 2020-04-06T08:02:47Z

My PR changing swap_remove landed; could you rebase this and make its panic handling consistent with the rest?

Sure, will do!

IgorPerikov · 2020-04-06T12:33:16Z

Soo.. I've messed up something. Tbh got used to strategy of merging whatever I need and then squash-merging into master

I guess it'd be easier to create new pr against master with needed changes and link to this for discussions history, @LukasKalbertodt are u agree?

LukasKalbertodt · 2020-04-06T12:41:55Z

I guess it'd be easier to create new pr against master with needed changes and link to this for discussions history, @LukasKalbertodt are u agree?

I'd be fine with that. Be sure to include r? @LukasKalbertodt in your new PR comment to assign me.

eddyb · 2020-04-06T12:49:32Z

@IgorPerikov Just run git pull --rebase https://github.com/rust-lang/rust master, it should auto-fix everything.

IgorPerikov · 2020-04-06T14:00:40Z

Just run git pull --rebase https://github.com/rust-lang/rust master, it should auto-fix everything.

This didn't work out as I was forced to resolve all the conflicts, solving them and continuing didn't help, so I skipped and force-pushed. I suppose it should be fine now

eddyb · 2020-04-06T14:32:54Z

Ah, hmm, if you had duplicated history compared to master then it could cause conflicts.
git rebase -i upstream/master (wasn't sure what your remote name is though) is the better alternative because it lets you remove redundant commits (which I guess you did with --skip).

Anyway I'm glad the PR is saved :).

LukasKalbertodt · 2020-04-06T14:36:16Z

@IgorPerikov I know this isn't the best time to ask this (as you just had lots of git trouble), but could you squash your commits into one or two? Currently there are many commits that don't compile. This makes the git history somewhat meh. If you need help with squashing, be sure to ask, we can help. (I'm pretty busy right now, but I can provide more information later, if you want.)

IgorPerikov · 2020-04-06T14:39:55Z

@LukasKalbertodt no-no It's okay for sure :) I can do it manually (this time I am familiar with procedure), but isn't squash and merge (in github merge pr dropdown menu) would solve it too?

RalfJung · 2020-04-06T14:41:04Z

Yeah but we don't merge via that button. We have a bot that makes sure full CI (all 30+ runners, not just the few that you can see in this PR already) passes on the merged state before putting anything to master.

That bot unfortunately has no auto-squashing ability.

LukasKalbertodt · 2020-04-06T17:30:29Z

Looks good to me. Thanks a bunch for squashing and updating the PR :)

@bors r+ rollup=never

bors · 2020-04-06T17:30:30Z

📌 Commit 9fc77c0 has been approved by LukasKalbertodt

IgorPerikov · 2020-04-06T17:39:37Z

Thanks everyone for all the valuable tips and infos. Your support was one in a kind actually

bors · 2020-04-06T18:14:55Z

⌛ Testing commit 9fc77c0 with merge 6dee5f1...

bors · 2020-04-06T21:29:43Z

☀️ Test successful - checks-azure
Approved by: LukasKalbertodt
Pushing 6dee5f1 to master...

LukasKalbertodt · 2020-05-02T15:18:20Z

Coincidentally, a while back, I wrote a few benchmarks that make heavy use of Vec::insert. I can run those benchmarks once this PR is released as nightly.

Finally managed to do this. I compared nightly-2020-04-05 and nightly 2020-04-10. I couldn't see any differences in my benchmark apart from noise. So that's another hint that this change doesn't cause a notable performance regression.

rust-highfive assigned LukasKalbertodt Mar 30, 2020

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

RalfJung reviewed Mar 30, 2020

View reviewed changes

src/liballoc/vec.rs Outdated Show resolved Hide resolved

LukasKalbertodt reviewed Mar 31, 2020

View reviewed changes

RalfJung mentioned this pull request Apr 1, 2020

Fix some aliasing issues in Vec #70558

Merged

add detailed panic messages for Vec functions

9fc77c0

LukasKalbertodt approved these changes Apr 6, 2020

View reviewed changes

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 6, 2020

bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 6, 2020

bors merged commit 6dee5f1 into rust-lang:master Apr 6, 2020

Detailed panic messages for Vec functions #70573

Detailed panic messages for Vec functions #70573

Conversation

IgorPerikov commented Mar 30, 2020

rust-highfive commented Mar 30, 2020

RalfJung Mar 30, 2020

Choose a reason for hiding this comment

RalfJung Mar 30, 2020

Choose a reason for hiding this comment

IgorPerikov Mar 30, 2020

Choose a reason for hiding this comment

IgorPerikov Mar 30, 2020

Choose a reason for hiding this comment

RalfJung Mar 30, 2020

Choose a reason for hiding this comment

IgorPerikov Mar 30, 2020

Choose a reason for hiding this comment

RalfJung Mar 30, 2020

Choose a reason for hiding this comment

IgorPerikov commented Mar 31, 2020

LukasKalbertodt left a comment

Choose a reason for hiding this comment

LukasKalbertodt commented Mar 31, 2020

rust-timer commented Mar 31, 2020

bors commented Mar 31, 2020

bors commented Apr 1, 2020

rust-timer commented Apr 1, 2020

IgorPerikov commented Apr 1, 2020

LukasKalbertodt commented Apr 1, 2020 • edited Loading

IgorPerikov commented Apr 1, 2020

LukasKalbertodt commented Apr 1, 2020

IgorPerikov commented Apr 1, 2020

LukasKalbertodt commented Apr 1, 2020

RalfJung commented Apr 1, 2020 • edited Loading

LukasKalbertodt commented Apr 1, 2020

RalfJung commented Apr 1, 2020

LukasKalbertodt commented Apr 1, 2020

RalfJung commented Apr 1, 2020

IgorPerikov commented Apr 1, 2020

LukasKalbertodt commented Apr 1, 2020

RalfJung commented Apr 1, 2020

rust-highfive commented Apr 1, 2020

rust-highfive commented Apr 1, 2020

IgorPerikov commented Apr 2, 2020

RalfJung commented Apr 2, 2020 • edited Loading

LukasKalbertodt commented Apr 2, 2020

eddyb commented Apr 2, 2020

nnethercote commented Apr 2, 2020

eddyb commented Apr 2, 2020

RalfJung commented Apr 5, 2020

LukasKalbertodt commented Apr 6, 2020

RalfJung commented Apr 6, 2020

IgorPerikov commented Apr 6, 2020

IgorPerikov commented Apr 6, 2020 • edited Loading

LukasKalbertodt commented Apr 6, 2020 • edited Loading

eddyb commented Apr 6, 2020

IgorPerikov commented Apr 6, 2020

eddyb commented Apr 6, 2020

LukasKalbertodt commented Apr 6, 2020 • edited Loading

IgorPerikov commented Apr 6, 2020 • edited Loading

RalfJung commented Apr 6, 2020 • edited Loading

LukasKalbertodt commented Apr 6, 2020

bors commented Apr 6, 2020

IgorPerikov commented Apr 6, 2020

bors commented Apr 6, 2020

bors commented Apr 6, 2020

LukasKalbertodt commented May 2, 2020

LukasKalbertodt commented Apr 1, 2020 •

edited

Loading

RalfJung commented Apr 1, 2020 •

edited

Loading

RalfJung commented Apr 2, 2020 •

edited

Loading

IgorPerikov commented Apr 6, 2020 •

edited

Loading

LukasKalbertodt commented Apr 6, 2020 •

edited

Loading

LukasKalbertodt commented Apr 6, 2020 •

edited

Loading

IgorPerikov commented Apr 6, 2020 •

edited

Loading

RalfJung commented Apr 6, 2020 •

edited

Loading