Insurance policy in case `iter.size_hint()` lies. by Centril · Pull Request #65749 · rust-lang/rust

Centril · 2019-10-24T01:27:28Z

Follow up to https://github.com/rust-lang/rust/pull/64949/files#r334235076.
(If the perf impact is bad we can use debug_assert! instead.)

The good news is that the UI tests pass locally so iter.size_hint() seems to be honest thus far.
On the other hand, with the status quo we do not have an insurance policy should that change in some case. This is problematic because a) this could possibly make some program be accepted which shouldn't, b) the compiler itself could have memory unsafety if the correctness of the iterator is assumed in unsafe { ... } code (even though the blame lies with the unsafe { ... } block in question.)

r? @RalfJung
cc @nnethercote

Centril · 2019-10-24T01:27:51Z

@bors try @rust-timer queue

rust-timer · 2019-10-24T01:27:54Z

Awaiting bors try build completion

bors · 2019-10-24T01:28:06Z

⌛ Trying commit 256335b with merge bc4dafa90d4ba113c682edfe266ea94a0e2fbc2f...

bors · 2019-10-24T04:36:09Z

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

rust-timer · 2019-10-24T04:36:10Z

Queued bc4dafa90d4ba113c682edfe266ea94a0e2fbc2f with parent 4a8c5b2, future comparison URL.

RalfJung

r=me if perf looks good.

Marwes · 2019-10-24T08:04:33Z

 f(&[t0, t1])
 }
 (0, Some(0)) => {
+ assert!(iter.next().is_none());


If it matters this could be written without unwrap's or asserts.
let a = match iter.next(){None => returnOk(f(&[])),Some(a) => a?,};let b = match iter.next(){None => returnOk(f(&[a])),Some(b) => b?,};let c = match iter.next(){None => returnOk(f(&[a, b])),Some(c) => c?,};letmut vec:SmallVec<[_;8]> = SmallVec::with_capacity(iter.size_hint().0 + 3); vec.push(a); vec.push(b); vec.push(c);for result in iter { vec.push(result?);}Ok(f(&vec))

Nice! OTOH, the original order of the match was chosen by @nnethercote to reflect the frequency of the cases.

@RalfJung Should we perf test this variant?

No opinion. @nnethercote what do you think?

The vec construction could be more concise, e.g. using extend. And I would use t0, t1, t2 instead of a, b, c. And the comment needs updating. Otherwise, fine by me to use this form if the perf isn't hurt.

extend would be better but there isn't a way in std to extend from a Result iterator so you need a workaround like https://github.com/mitsuhiko/redis-rs/blob/418512bf9171589b814da4c5ecb500e8747781e0/src/parser.rs#L21-L54

rust-timer · 2019-10-24T11:42:35Z

Finished benchmarking try commit bc4dafa90d4ba113c682edfe266ea94a0e2fbc2f, comparison URL.

Centril · 2019-10-24T15:34:43Z

Perf looks like noise / possibly an improvement.

Centril · 2019-10-25T01:38:33Z

@bors try @rust-timer queue

rust-timer · 2019-10-25T01:38:35Z

Awaiting bors try build completion

bors · 2019-10-25T01:38:43Z

⌛ Trying commit c85bfc5 with merge 2beb6a3...

@RalfJung

Insurance policy in case `iter.size_hint()` lies. Follow up to https://github.com/rust-lang/rust/pull/64949/files#r334235076. (If the perf impact is bad we can use `debug_assert!` instead.) The good news is that the UI tests pass locally so `iter.size_hint()` seems to be honest *thus far*. On the other hand, with the status quo we do not have an insurance policy should that change in some case. This is problematic because a) this could possibly make some program be accepted which shouldn't, b) the compiler itself could have memory unsafety if the correctness of the iterator is assumed in `unsafe { ... }` code (even though the blame lies with the `unsafe { ... }` block in question.) r? @RalfJung cc @nnethercote

bors · 2019-10-25T04:12:55Z

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

rust-timer · 2019-10-25T04:12:57Z

Queued 2beb6a3 with parent 10a52c2, future comparison URL.

rust-timer · 2019-10-25T10:36:51Z

Finished benchmarking try commit 2beb6a3, comparison URL.

Centril · 2019-10-25T11:20:26Z

@nnethercote Thoughts on ^---? It looks like a slight regression to me.

nnethercote · 2019-10-25T11:25:15Z

Let's go with the first one! :)

mati865 · 2019-10-25T11:26:15Z

I think Clippy even has lint against:

for result in iter {
vec.push(result?);}

IIRC this performs poorly and could be main reason of this regression.

lies, underreporting the number of elements.

Centril · 2019-10-25T15:45:26Z

Dropped the second commits in favor of the first solution which had clean perf; @bors r=RalfJung

bors · 2019-10-25T15:45:27Z

📌 Commit dfcfca2 has been approved by RalfJung

@RalfJung

Insurance policy in case `iter.size_hint()` lies. Follow up to https://github.com/rust-lang/rust/pull/64949/files#r334235076. (If the perf impact is bad we can use `debug_assert!` instead.) The good news is that the UI tests pass locally so `iter.size_hint()` seems to be honest *thus far*. On the other hand, with the status quo we do not have an insurance policy should that change in some case. This is problematic because a) this could possibly make some program be accepted which shouldn't, b) the compiler itself could have memory unsafety if the correctness of the iterator is assumed in `unsafe { ... }` code (even though the blame lies with the `unsafe { ... }` block in question.) r? @RalfJung cc @nnethercote

@ghost

Rollup of 7 pull requests Successful merges: - #63810 (Make <*const/mut T>::offset_from `const fn`) - #65705 (Add {String,Vec}::into_raw_parts) - #65749 (Insurance policy in case `iter.size_hint()` lies.) - #65799 (Fill tracking issue number for `array_value_iter`) - #65800 (self-profiling: Update measureme to 0.4.0 and remove non-RAII methods from profiler.) - #65806 (Add [T]::as_ptr_range() and [T]::as_mut_ptr_range().) - #65810 (SGX: Clear additional flag on enclave entry) Failed merges: r? @ghost

@RalfJung

Insurance policy in case `iter.size_hint()` lies. Follow up to https://github.com/rust-lang/rust/pull/64949/files#r334235076. (If the perf impact is bad we can use `debug_assert!` instead.) The good news is that the UI tests pass locally so `iter.size_hint()` seems to be honest *thus far*. On the other hand, with the status quo we do not have an insurance policy should that change in some case. This is problematic because a) this could possibly make some program be accepted which shouldn't, b) the compiler itself could have memory unsafety if the correctness of the iterator is assumed in `unsafe { ... }` code (even though the blame lies with the `unsafe { ... }` block in question.) r? @RalfJung cc @nnethercote

@ghost

Rollup of 6 pull requests Successful merges: - #65705 (Add {String,Vec}::into_raw_parts) - #65749 (Insurance policy in case `iter.size_hint()` lies.) - #65799 (Fill tracking issue number for `array_value_iter`) - #65800 (self-profiling: Update measureme to 0.4.0 and remove non-RAII methods from profiler.) - #65806 (Add [T]::as_ptr_range() and [T]::as_mut_ptr_range().) - #65810 (SGX: Clear additional flag on enclave entry) Failed merges: r? @ghost

rust-highfive assigned RalfJung Oct 24, 2019

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

RalfJung approved these changes Oct 24, 2019
View reviewed changes

Marwes reviewed Oct 24, 2019
View reviewed changes

Take out an insurance policy in case iter.size_hint()
dfcfca2
lies, underreporting the number of elements.

Centril force-pushed the insurance-policy branch from c85bfc5 to dfcfca2Compare October 25, 2019 15:44

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 Oct 25, 2019

Centril mentioned this pull request Oct 25, 2019
Rollup of 7 pull requests #65820
Closed

JohnTitor mentioned this pull request Oct 25, 2019
Rollup of 6 pull requests #65826
Merged

bors merged commit dfcfca2 into rust-lang:masterOct 25, 2019

Centril deleted the insurance-policy branch October 26, 2019 04:23

Uh oh!

Uh oh!

Conversation

Centril commented Oct 24, 2019

Uh oh!

Centril commented Oct 24, 2019

Uh oh!

rust-timer commented Oct 24, 2019

Uh oh!

bors commented Oct 24, 2019

Uh oh!

bors commented Oct 24, 2019

Uh oh!

rust-timer commented Oct 24, 2019

Uh oh!

RalfJung left a comment

Choose a reason for hiding this comment

Uh oh!

MarwesOct 24, 2019

Choose a reason for hiding this comment

Uh oh!

RalfJungOct 24, 2019

Choose a reason for hiding this comment

Uh oh!

CentrilOct 24, 2019

Choose a reason for hiding this comment

Uh oh!

RalfJungOct 24, 2019

Choose a reason for hiding this comment

Uh oh!

nnethercoteOct 24, 2019

Choose a reason for hiding this comment

Uh oh!

MarwesOct 25, 2019

Choose a reason for hiding this comment

Uh oh!

rust-timer commented Oct 24, 2019

Uh oh!

Centril commented Oct 24, 2019

Uh oh!

Centril commented Oct 25, 2019

Uh oh!

rust-timer commented Oct 25, 2019

Uh oh!

bors commented Oct 25, 2019

Uh oh!

bors commented Oct 25, 2019

Uh oh!

rust-timer commented Oct 25, 2019

Uh oh!

rust-timer commented Oct 25, 2019

Uh oh!

Centril commented Oct 25, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nnethercote commented Oct 25, 2019

Uh oh!

mati865 commented Oct 25, 2019

Uh oh!

Centril commented Oct 25, 2019

Uh oh!

bors commented Oct 25, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

Centril commented Oct 25, 2019•
edited
Loading