Skip to content

rustc_arena: handle recursive allocation, don't trust size_hint #79154

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

Closed

Conversation

tgnottingham
Copy link
Contributor

In DroplessArena::alloc_from_iter, handle allocating from iterators
whose next calls may themselves allocate on the arena. Also, do not
trust the iterator's size_hint implementation for correctness.
These changes were already made for TypedArena in #67003.

In `DroplessArena::alloc_from_iter`, handle allocating from iterators
whose `next` calls may themselves allocate on the arena. Also, do not
trust the iterator's `size_hint` implementation for correctness.
These changes were already made for `TypedArena` in rust-lang#67003.
@rust-highfive
Copy link
Contributor

r? @davidtwco

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 18, 2020
@tgnottingham
Copy link
Contributor Author

r? @Mark-Simulacrum

@tgnottingham
Copy link
Contributor Author

I attempted to implement a performance mitigation to avoid the extra copying in certain cases, but it didn't work out.

If an exact size hint was available, the mitigation would copy into an appropriately-sized TypedArenaChunk instead of a SmallVec. To avoid wasting space, it would still copy the data from that chunk into the existing last chunk if it could fit. But if it couldn't fit in the last chunk, it would just append the chunk as an optimization.

It managed to worsen performance, probably due to a combination of complexity, that it doesn't avoid allocating sometimes like the SmallVec approach does, and that the optimization doesn't kick in all that often because it tries to avoid wasting space. I probably could have explored a SmallVec-like optimization that avoided allocating, but it seems like the potential gains are marginal.

@est31
Copy link
Member

est31 commented Nov 18, 2020

Is this PR a good idea?

First, next allocating doesn't cause any issues for the dropless arena. We do self.alloc_raw before we call the next function in any way.

Regarding the second point, if size_hint's minimum matches the maximum, then it's required to be accurate, otherwise it's a bug in the iterator. This may not cause unsound code, but from what I can tell it doesn't. If the size hint is too low, then write_from_iter aborts early. If it's too high, we have some unused memory left, but that's no problem because the arena is dropless, so we never run any destructor on that unused memory.

cc @bugadani

@jyn514 jyn514 added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Nov 18, 2020
@tgnottingham
Copy link
Contributor Author

Ah, okay. I see I missed the critical difference as to why this was safe for DroplessArena.

With DroplessArena, we can update self.ptr, etc. before iterating, which means that any allocation resulting from a call to next() won't write to the memory reserved for our current iterator. But we can't do this for TypedArena, because if we update self.ptr, etc. prematurely, then if next() panics, we would end up dropping the wrong number of elements.

Nevermind then. :)

@est31
Copy link
Member

est31 commented Nov 18, 2020

With DroplessArena, we can update self.ptr, etc. before iterating, which means that any allocation resulting from a call to next() won't write to the memory reserved for our current iterator. But we can't do this for TypedArena, because if we update self.ptr, etc. prematurely, then if next() panics, we would end up dropping the wrong number of elements.

Yup, that's the logic!

@tgnottingham tgnottingham deleted the dropless_alloc_from_iter branch January 20, 2021 18:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants