Use zeroed allocations in the mir interpreter instead eagerly touching the memory #87777

the8472 · 2021-08-04T23:04:32Z

#86255 introduced a 30% regression in page faults and a 3% regression in max-rss in the ctfe-stress benchmarks.
That's most likely happened because it separated allocation from initialization of the vec which defeats the zero-optimization.

Currently there's no allocation API that is fallible, zeroing and returns a slice, so this PR introduces one and then uses that to solve the problem. In principle vec.resize(len, 0) could be optimized to use alloc::grow_zeroed where appropriate but that would require new specializations and new plumbing in RawVec.

Currently there is no API that allows fallible zero-allocation of a Vec. Vec.try_reserve is not appropriate for this job since it doesn't know whether it should zero or arbitrary uninitialized memory is fine. Since Box currently holds most of the zeroing/uninit/slice allocation APIs it's the best place to add yet another entry into this feature matrix.

rust-highfive · 2021-08-04T23:04:34Z

Some changes occured to the CTFE / Miri engine

cc @rust-lang/miri

rust-highfive · 2021-08-04T23:04:35Z

r? @joshtriplett

(rust-highfive has picked a reviewer for you, use r? to override)

the8472 · 2021-08-04T23:06:24Z

@bors try @rust-timer queue

rust-timer · 2021-08-04T23:06:26Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2021-08-04T23:06:33Z

⌛ Trying commit 83b01b9 with merge 03a355da4d3c576621f95121eda74d06b5d46623...

bors · 2021-08-05T00:06:54Z

☀️ Try build successful - checks-actions
Build commit: 03a355da4d3c576621f95121eda74d06b5d46623 (03a355da4d3c576621f95121eda74d06b5d46623)

Aaron1011 · 2021-08-05T00:13:07Z

library/alloc/src/boxed.rs

@@ -589,6 +589,38 @@ impl<T> Box<[T]> {
    pub fn new_zeroed_slice(len: usize) -> Box<[mem::MaybeUninit<T>]> {
        unsafe { RawVec::with_capacity_zeroed(len).into_box(len) }
    }
+
+    /// Constructs a new boxed slice with uninitialized contents, with the memory


Saying 'uninitialized contents' sounds confusing when we also state that the memory is zeroed. I think it would be better to write out MaybeUninit<T>, to make it clear that the allocated memory is initialized.

I copied that from another _zeroed function, should I fix those too?

the8472 · 2021-08-05T07:31:11Z

Oh wait, I've used the wrong conversion API to turn it into a Vec.

the8472 · 2021-08-05T07:33:14Z

@bors try @rust-timer queue

rust-timer · 2021-08-05T07:33:15Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2021-08-05T10:35:39Z

⌛ Trying commit f408d4b with merge 59d8565f8dd0e5f33615fac8de3d585ceb47b7fe...

bors · 2021-08-05T11:28:05Z

☀️ Try build successful - checks-actions
Build commit: 59d8565f8dd0e5f33615fac8de3d585ceb47b7fe (59d8565f8dd0e5f33615fac8de3d585ceb47b7fe)

rust-timer · 2021-08-05T11:28:07Z

Queued 59d8565f8dd0e5f33615fac8de3d585ceb47b7fe with parent 2ddb65c, future comparison URL.

rust-timer · 2021-08-05T13:55:54Z

Finished benchmarking try commit (59d8565f8dd0e5f33615fac8de3d585ceb47b7fe): comparison url.

Summary: This benchmark run did not return any significant changes.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf -perf-regression

the8472 · 2021-08-05T16:01:28Z

That yields the expected improvements in page faults and max-rss. The max-rss results have a question mark next to them but the results are larger than the normal variance range, so it should be a non-spurious improvement.

oli-obk · 2021-08-05T16:22:33Z

CI is failing in a fulldeps test due to cfged out imports. Other than that, lgtm.

Independently: maybe the data field should just be a Box<[u8]>?

also cc @rust-lang/wg-const-eval the allocation scheme of Allocation::undef is being changed in small ways

… code

the8472 · 2021-08-05T17:56:15Z

CI is failing in a fulldeps test due to cfged out imports.

Just wanted to get a perf report first. Should be fixed now.

Independently: maybe the data field should just be a Box<[u8]>?

Done.

library/alloc/src/boxed.rs

oli-obk · 2021-08-05T19:01:21Z

lgtm now, but someone from libs should look at the allocator API addition

r? rust-lang/libs-api

oli-obk · 2021-08-05T19:04:37Z

looks like highfive doesn't like @rust-lang/libs-api sorry to ping y'all, but can someone take a quick look a the (unstable) box API changes?

the8472 · 2021-08-05T19:06:53Z

josh is part of the libs team ;)

joshtriplett · 2021-08-06T10:26:43Z

👍 for the changes to unstable allocator APIs. We can decide later if we want to stabilize those APIs, but adding them to nightly for use in the compiler itself seems fine here.

oli-obk · 2021-08-06T10:28:55Z

@bors r=oli-obk,joshtriplett

bors · 2021-08-06T10:28:57Z

📌 Commit 1c21373 has been approved by oli-obk,joshtriplett

bors · 2021-08-06T12:11:33Z

⌛ Testing commit 1c21373 with merge 4c29cc8...

bors · 2021-08-06T15:34:55Z

☀️ Test successful - checks-actions
Approved by: oli-obk,joshtriplett
Pushing 4c29cc8 to master...

RalfJung · 2024-03-15T10:35:22Z

compiler/rustc_middle/src/mir/interpret/allocation.rs

@@ -28,7 +28,7 @@ use crate::ty;
 pub struct Allocation<Tag = AllocId, Extra = ()> {
    /// The actual bytes of the allocation.
    /// Note that the bytes of a pointer represent the offset of the pointer.
-    bytes: Vec<u8>,
+    bytes: Box<[u8]>,


Turns out changing this from Vec to Box introduced UB into the interpreter itself... also see Zulip

…li-obk interpret/allocation: fix aliasing issue in interpreter and refactor getters a bit That new raw getter will be needed to let Miri pass pointers to natively executed FFI code ("extern-so" mode). While doing that I realized our get_bytes_mut are named less scary than get_bytes_unchecked so I rectified that. Also I realized `mem_copy_repeatedly` would break if we called it for multiple overlapping copies so I made sure this does not happen. And I realized that we are actually [violating Stacked Borrows in the interpreter](https://rust-lang.zulipchat.com/#narrow/stream/136281-t-opsem/topic/I.20think.20Miri.20violates.20Stacked.20Borrows.20.F0.9F.99.88).^^ That was introduced in rust-lang#87777. r? `@oli-obk`

…li-obk interpret/allocation: fix aliasing issue in interpreter and refactor getters a bit That new raw getter will be needed to let Miri pass pointers to natively executed FFI code ("extern-so" mode). While doing that I realized our get_bytes_mut are named less scary than get_bytes_unchecked so I rectified that. Also I realized `mem_copy_repeatedly` would break if we called it for multiple overlapping copies so I made sure this does not happen. And I realized that we are actually [violating Stacked Borrows in the interpreter](https://rust-lang.zulipchat.com/#narrow/stream/136281-t-opsem/topic/I.20think.20Miri.20violates.20Stacked.20Borrows.20.F0.9F.99.88).^^ That was introduced in rust-lang#87777. r? ``@oli-obk``

…li-obk interpret/allocation: fix aliasing issue in interpreter and refactor getters a bit That new raw getter will be needed to let Miri pass pointers to natively executed FFI code ("extern-so" mode). While doing that I realized our get_bytes_mut are named less scary than get_bytes_unchecked so I rectified that. Also I realized `mem_copy_repeatedly` would break if we called it for multiple overlapping copies so I made sure this does not happen. And I realized that we are actually [violating Stacked Borrows in the interpreter](https://rust-lang.zulipchat.com/#narrow/stream/136281-t-opsem/topic/I.20think.20Miri.20violates.20Stacked.20Borrows.20.F0.9F.99.88).^^ That was introduced in rust-lang#87777. r? ```@oli-obk```

Rollup merge of rust-lang#122537 - RalfJung:interpret-allocation, r=oli-obk interpret/allocation: fix aliasing issue in interpreter and refactor getters a bit That new raw getter will be needed to let Miri pass pointers to natively executed FFI code ("extern-so" mode). While doing that I realized our get_bytes_mut are named less scary than get_bytes_unchecked so I rectified that. Also I realized `mem_copy_repeatedly` would break if we called it for multiple overlapping copies so I made sure this does not happen. And I realized that we are actually [violating Stacked Borrows in the interpreter](https://rust-lang.zulipchat.com/#narrow/stream/136281-t-opsem/topic/I.20think.20Miri.20violates.20Stacked.20Borrows.20.F0.9F.99.88).^^ That was introduced in rust-lang#87777. r? ```@oli-obk```

the8472 added 2 commits August 5, 2021 00:23

use zeroed allocation instead of eagerly initializing the memory

83b01b9

the8472 added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Aug 4, 2021

rust-highfive assigned joshtriplett Aug 4, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 4, 2021

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Aug 4, 2021

This comment has been minimized.

Sign in to view

Aaron1011 reviewed Aug 5, 2021

View reviewed changes

This comment has been minimized.

Sign in to view

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Aug 5, 2021

This comment has been minimized.

Sign in to view

use box->vec conversion API that doesn't reallocate

f408d4b

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Aug 5, 2021

This comment has been minimized.

Sign in to view

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Aug 5, 2021

the8472 added 2 commits August 5, 2021 19:45

remove cfg gate on use RawVec since it is now also used in fallible…

6ed2d87

… code

replace Vec<u8> with Box<[u8]>

55def12

bjorn3 reviewed Aug 5, 2021

View reviewed changes

library/alloc/src/boxed.rs Show resolved Hide resolved

the8472 mentioned this pull request Aug 5, 2021

Box::new flavors keep multiplying rust-lang/wg-allocators#90

Open

add Box::try_new_uninit_slice for symmetry

1c21373

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 Aug 6, 2021

bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 6, 2021

bors merged commit 4c29cc8 into rust-lang:master Aug 6, 2021

rustbot added this to the 1.56.0 milestone Aug 6, 2021

RalfJung reviewed Mar 15, 2024

View reviewed changes

RalfJung mentioned this pull request Mar 15, 2024

interpret/allocation: fix aliasing issue in interpreter and refactor getters a bit #122537

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use zeroed allocations in the mir interpreter instead eagerly touching the memory #87777

Use zeroed allocations in the mir interpreter instead eagerly touching the memory #87777

the8472 commented Aug 4, 2021 •

edited

Loading

rust-highfive commented Aug 4, 2021

rust-highfive commented Aug 4, 2021

the8472 commented Aug 4, 2021

rust-timer commented Aug 4, 2021

bors commented Aug 4, 2021

This comment has been minimized.

bors commented Aug 5, 2021

This comment has been minimized.

Aaron1011 Aug 5, 2021

the8472 Aug 5, 2021

This comment has been minimized.

This comment has been minimized.

the8472 commented Aug 5, 2021

the8472 commented Aug 5, 2021

rust-timer commented Aug 5, 2021

This comment has been minimized.

bors commented Aug 5, 2021

bors commented Aug 5, 2021

rust-timer commented Aug 5, 2021

rust-timer commented Aug 5, 2021

the8472 commented Aug 5, 2021

oli-obk commented Aug 5, 2021

the8472 commented Aug 5, 2021

oli-obk commented Aug 5, 2021

oli-obk commented Aug 5, 2021

the8472 commented Aug 5, 2021

joshtriplett commented Aug 6, 2021

oli-obk commented Aug 6, 2021

bors commented Aug 6, 2021

bors commented Aug 6, 2021

bors commented Aug 6, 2021

RalfJung Mar 15, 2024

Use zeroed allocations in the mir interpreter instead eagerly touching the memory #87777

Use zeroed allocations in the mir interpreter instead eagerly touching the memory #87777

Conversation

the8472 commented Aug 4, 2021 • edited Loading

rust-highfive commented Aug 4, 2021

rust-highfive commented Aug 4, 2021

the8472 commented Aug 4, 2021

rust-timer commented Aug 4, 2021

bors commented Aug 4, 2021

This comment has been minimized.

bors commented Aug 5, 2021

This comment has been minimized.

Aaron1011 Aug 5, 2021

Choose a reason for hiding this comment

the8472 Aug 5, 2021

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

the8472 commented Aug 5, 2021

the8472 commented Aug 5, 2021

rust-timer commented Aug 5, 2021

This comment has been minimized.

bors commented Aug 5, 2021

bors commented Aug 5, 2021

rust-timer commented Aug 5, 2021

rust-timer commented Aug 5, 2021

the8472 commented Aug 5, 2021

oli-obk commented Aug 5, 2021

the8472 commented Aug 5, 2021

oli-obk commented Aug 5, 2021

oli-obk commented Aug 5, 2021

the8472 commented Aug 5, 2021

joshtriplett commented Aug 6, 2021

oli-obk commented Aug 6, 2021

bors commented Aug 6, 2021

bors commented Aug 6, 2021

bors commented Aug 6, 2021

RalfJung Mar 15, 2024

Choose a reason for hiding this comment

the8472 commented Aug 4, 2021 •

edited

Loading