feat(allocator): remove drop operations from Vec2

[Dunqing]

Just like #6623. Since we have control over the Vec implementation, we can remove the drop operations and eventually revert the changes made in #6623 directly.

[Dunqing]

• refactor(allocator/vec2): rename parameters and method name to align with std #9858 : 2 dependent PRs (#9855 , #9857 )
• feat(allocator/vec2): align the retain method with the standard implementation #9752
• feat(allocator/vec): remove ManuallyDrop wrapper #9742
• feat(allocator): remove drop operations from Vec2 #9679 👈 (View in Graphite)
• feat(allocator): replace allocator_ap2's Vec with Vec2 #9656
• feat(allocator): add Vec2::retain_mut method #9655
• feat(allocator): connect Vec2 module and make it compile #9647
• refactor(allocator/vec): disable lint warnings in vec2 files #9730
• docs(allocator/vec): fix link in doc comment for Vec2 #9729
• refactor(allocator/vec): comment out feature-gated methods #9728
• main

---

How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

• 0-merge - adds this PR to the back of the merge queue
• hotfix - for urgent hot fixes, skip the queue and merge this PR next

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

[codspeed-hq]

CodSpeed Performance Report

Merging #9679 will not alter performance

_{Comparing 03-11-feat_allocator_remove_drop_operations_from_vec2 (65643bc) with main (d13817e)}

Summary

✅ 33 untouched benchmarks

[Dunqing]

I am not sure which drop can be removed, I may need to take a look at Bumpalo's Bump implementation.

[overlookmotel]

I am not sure which drop can be removed, I may need to take a look at Bumpalo's Bump implementation.

We can remove loads. But I'd suggest starting by just removing:

• impl<'bump, T> Drop for Vec<'bump, T>
• impl<'a, T> Drop for RawVec<'a, T>

That'd bring vec2::Vec up to parity with current Vec. It'd be enough to then be able to remove ManuallyDrop from Vec so Vec becomes just struct Vec(vec2::Vec);.

Removing further Drop-related code would be an improvement over what we have now. I'd be interested to see if it affects benchmarks as we take them out one by one.

But we may want to simplify/remove other code first. bumpalo's implementation offers various options which we don't use, and they clutter the code and make it harder to see what's going on. I suggest we discuss next steps once we've got the first parts merged.

[overlookmotel]

Could we limit the scope of this PR to just removing these 2?

• impl<'bump, T> Drop for Vec<'bump, T>
• impl<'a, T> Drop for RawVec<'a, T>

Those 2 Drop impls are definitely fine to remove. The other ones are less clear without reading all the code around them.

I think it's preferable to go step-by-step on this. It'd be very easy to make a mistake somewhere and cause UB, so I think it's better to err on the side of caution.

[Dunqing]

Could we limit the scope of this PR to just removing these 2?

• impl<'bump, T> Drop for Vec<'bump, T>
• impl<'a, T> Drop for RawVec<'a, T>

Those 2 Drop impls are definitely fine to remove. The other ones are less clear without reading all the code around them.

I think it's preferable to go step-by-step on this. It'd be very easy to make a mistake somewhere and cause UB, so I think it's better to err on the side of caution.

This PR is already just removing these 2 which you suggested, additionally, removed mem::forgot because non-drop calling mem::forgot is meaningless.

[overlookmotel]

This PR is already just removing these 2 which you suggested, additionally, removed mem::forgot because non-drop calling mem::forgot is meaningless.

OK. I was being lazy. I didn't want to check what self was referring to in the mem::forget(self) calls that you removed.

I have checked now - self is always a vec2::Vec in these places, so yes you're right the mem::forget calls are pointless.

But could you replace them with comments saying something like this?

// Don't need `mem::forget(self)` here, because `Vec` does not implement `Drop`

[Dunqing]

But could you replace them with comments saying something like this?

// Don't need `mem::forget(self)` here, because `Vec` does not implement `Drop`

Done!

[overlookmotel]

Merge activity

• Mar 14, 3:50 AM EDT: The merge label '0-merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
• Mar 14, 3:50 AM EDT: A user added this pull request to the Graphite merge queue.
• Mar 14, 3:57 AM EDT: A user merged this pull request with the Graphite merge queue.