-
Notifications
You must be signed in to change notification settings - Fork 12.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support allocating iterators with arenas #59533
Conversation
This comment has been minimized.
This comment has been minimized.
@@ -161,21 +186,60 @@ impl<T> TypedArena<T> { | |||
where | |||
T: Copy, | |||
{ | |||
unsafe { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A comment would be good here.
29cef27
to
3c51be5
Compare
Introduce an arena type which may be used to allocate a list of types with destructors You can also specify that you want deserializers for `&'tcx [T]` and `&'tcx T` for a type in the list, which will allocate those using the arena. Based on #59517 and #59533. Look at the last commit for the interesting changes. An alternative to #56448. cc @michaelwoerister @eddyb r? @oli-obk
src/libarena/lib.rs
Outdated
assert!(len != 0); | ||
|
||
if !self.can_allocate(len) { | ||
self.grow(len); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add a debug_assert!(self.can_allocate(len))
here, just to be sure.
src/libarena/lib.rs
Outdated
} | ||
match size_hint { | ||
(min, Some(max)) if min == max => { | ||
if min == 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a let len = min;
binding here? I think that makes it more readable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@michaelwoerister that seems like an odd use of let bindings... perhaps a comment instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd do both actually, as in:
// In this branch we know the exact length
let len = min;
src/libarena/lib.rs
Outdated
let len = vec.len(); | ||
let start_ptr = self.alloc_raw_slice(len); | ||
vec.as_ptr().copy_to_nonoverlapping(start_ptr, len); | ||
mem::forget(vec.drain()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From looking at the implementation of SmallVec
, it looks like this does the right thing, but I think it would be clearer to directly call vec.set_len(0)
here.
@@ -189,6 +257,7 @@ impl<T> TypedArena<T> { | |||
if let Some(last_chunk) = chunks.last_mut() { | |||
let used_bytes = self.ptr.get() as usize - last_chunk.start() as usize; | |||
let currently_used_cap = used_bytes / mem::size_of::<T>(); | |||
last_chunk.entries = currently_used_cap; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this was an existing bug, right? Good find!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There previously wasn't a way to end up with unused memory at the end of a chunk for types with destructors, so not an existing bug.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No? It looks like a similar case can also happen already in alloc_slice
:
Line 170 in f694222
self.grow(slice.len()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That requires Copy
types, so it can't trigger it.
📌 Commit 59ff059 has been approved by |
Support allocating iterators with arenas Split out from rust-lang#57173. r? @michaelwoerister
Rollup of 8 pull requests Successful merges: - #59262 (Remove duplicated code from Iterator::{ne, lt, le, gt, ge}) - #59286 (Refactor async fn return type lowering) - #59444 (Implement useful steps_between for all integers) - #59452 (Speed up rustdoc run a bit) - #59533 (Support allocating iterators with arenas) - #59585 (Fixes for shallow borrows) - #59607 (Renames `EvalErrorKind` to `InterpError`) - #59613 (SGX target: convert a bunch of panics to aborts) Failed merges: - #59630 (Shrink `mir::Statement`.) r? @ghost
Introduce an arena type which may be used to allocate a list of types with destructors You can also specify that you want deserializers for `&'tcx [T]` and `&'tcx T` for a type in the list, which will allocate those using the arena. Based on #59517 and #59533. Look at the last commit for the interesting changes. An alternative to #56448. cc @michaelwoerister @eddyb r? @oli-obk
Split out from #57173.
r? @michaelwoerister