Try not to copy when allocating from iterators, take 3

[bugadani]

The problem I'm trying to solve here (and in previous PRs) is that, TypedArena::alloc_from_iter first creates a vector from the iterator's contents, then copies it into the arena. I'm trying to reuse the allocated vector instead of copying it's contents.

Previous attempts have failed because of either unsoundness (#77945) or bad performance (#78067). This PR tried to address the issue by creating two, parallel arenas as part of a single TypedArena: one for "scalar" allocations, which includes slices of known size, and one for "vector" allocation, that store the vector objects created by alloc_from_iter. While this increases the size of TypedArena, I hope this split results in better performance since allocating from an iter means moving the Vec object to an already known address, instead of having to compute it through a vector. This change also wasn't able to achieve the desired performance, the increased size and complexity also caused slight regressions.

Current state of the PR: I've reverted most of the above changes and only kept special casing the allocation where the type inside TypedArena does not need special considerations when dropping.

[rust-highfive]

r? @oli-obk

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

[bugadani]

Oh, technically, this is a work in progress: I still need to put some time into naming things and clean up, but I don't want to do that before knowing if the change makes sense.

[oli-obk]

cc @est31 @Marwes

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion

[bors]

⌛ Trying commit 1690a82962ad3f9bd05bfbf04faccc18136756a9 with merge 7601a5e99583e48f583a8d083096b4882fb269ea...

[bors]

☀️ Try build successful - checks-actions, checks-azure
Build commit: 7601a5e99583e48f583a8d083096b4882fb269ea (7601a5e99583e48f583a8d083096b4882fb269ea)

[rust-timer]

Queued 7601a5e99583e48f583a8d083096b4882fb269ea with parent 9832374, future comparison URL.

[rust-timer]

Finished benchmarking try commit (7601a5e99583e48f583a8d083096b4882fb269ea): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never

[bugadani]

Here's what (I think) I know:

• duplicating the arena struct like this is definitely not worth it
• the !needs_drop "copy immediately into the arena" can result in small, but measurable improvement, as demonstrated in TypedArena: Eliminate intermediate copy if Iterator size is known #77945 - it really sucks that's unsound.
• the above speedup isn't significant looking at the "big picture"
• I think SmallVec also causes a perf hit here

I wonder if keeping the !needs_drop special case would be worth it. Normally, those should be allocated in DroplessArena I think, but the query system uses TypedArena because it can handle both, so there is actual usage of TypedArenas for no-drop type data.

[oli-obk]

So... you're suggesting we dispatch on whether something needs drop even earlier? Like have the check in the query system and then switch to DroplessArena?

[bugadani]

I think using DroplessArena wherever it's possible would indeed be preferrable, but I don't see how that can be done right now.

[bors]

☔ The latest upstream changes (presumably #78569) made this pull request unmergeable. Please resolve the merge conflicts.

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

[bugadani]

Well I rebased this patch, but thinking about it more, these changes should have no or negligible effect so I'm closing it.

• The query system only allocates single values, so these changes don't matter there
• I could only find a handful of cases where allocation doesn't go through TyCtxt which already dispatches based on needs_drop.
• Those handful of cases seem to need the drop glue so they are probably unaffected by this change.