Skip to content
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

feat(allocator): add Vec2::retain_mut method #9655

Merged
merged 1 commit into from
Mar 13, 2025

Conversation

Dunqing
Copy link
Member

@Dunqing Dunqing commented Mar 10, 2025

OXC has a few places using this API, so we need to add this method before replacing allocator-api2's Vec. The implementation is copied from the https://doc.rust-lang.org/src/alloc/vec/mod.rs.html#2121-2123, and modified a little to make it fit the Vec2.

@github-actions github-actions bot added the C-enhancement Category - New feature or request label Mar 10, 2025
Copy link
Member Author

Dunqing commented Mar 10, 2025


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.

Copy link

codspeed-hq bot commented Mar 10, 2025

CodSpeed Performance Report

Merging #9655 will not alter performance

Comparing 03-11-feat_allocator_add_vec2_retain_mut_method (65d9662) with main (a61a50b)

Summary

✅ 33 untouched benchmarks

@Dunqing Dunqing force-pushed the 03-11-feat_allocator_add_vec2_retain_mut_method branch from 450c4d3 to b3bbb9e Compare March 11, 2025 10:26
@Dunqing Dunqing force-pushed the 03-10-feat_allocator_add_vec2_module_and_make_it_compile branch from 9580d1e to 642f881 Compare March 11, 2025 10:26
@Dunqing Dunqing marked this pull request as ready for review March 11, 2025 10:36
@overlookmotel overlookmotel force-pushed the 03-10-feat_allocator_add_vec2_module_and_make_it_compile branch 2 times, most recently from d9a6cb3 to 415d24f Compare March 12, 2025 08:58
@overlookmotel overlookmotel force-pushed the 03-11-feat_allocator_add_vec2_retain_mut_method branch from b3bbb9e to 14c989a Compare March 12, 2025 08:58
@overlookmotel overlookmotel force-pushed the 03-10-feat_allocator_add_vec2_module_and_make_it_compile branch from 415d24f to c068933 Compare March 13, 2025 01:42
@overlookmotel overlookmotel force-pushed the 03-11-feat_allocator_add_vec2_retain_mut_method branch from 14c989a to 8bdcb8c Compare March 13, 2025 01:42
@overlookmotel
Copy link
Contributor

Are you able to summarize what "modified a little" means?

@overlookmotel overlookmotel changed the base branch from 03-10-feat_allocator_add_vec2_module_and_make_it_compile to graphite-base/9655 March 13, 2025 02:45
@Dunqing
Copy link
Member Author

Dunqing commented Mar 13, 2025

Are you able to summarize what "modified a little" means?

Different from standard implementation, Bumpalo accepts a bump lifetime instead of an Allocator generic type, so all changes made revolve around this difference. See the following diff you will know what I am talking about.

diff --git a/crates/oxc_allocator/src/vec2/mod.rs b/crates/oxc_allocator/src/vec2/mod.rs
index 945eaf4e7..a0185a10d 100644
--- a/crates/oxc_allocator/src/vec2/mod.rs
+++ b/crates/oxc_allocator/src/vec2/mod.rs
@@ -1318,7 +1318,10 @@ impl<'bump, T: 'bump> Vec<'bump, T> {
     /// });
     /// assert_eq!(vec, [2, 3, 4]);
     /// ```
-    #[stable(feature = "vec_retain_mut", since = "1.61.0")]
+    // The implementation is based on the [`std::vec::Vec::retain_mut`].
+    //
+    // Allowing the following clippy rules just to make the code same as the original implementation.
+    #[expect(clippy::items_after_statements, clippy::redundant_else)]
     pub fn retain_mut<F>(&mut self, mut f: F)
     where
         F: FnMut(&mut T) -> bool,
@@ -1345,14 +1348,14 @@ impl<'bump, T: 'bump> Vec<'bump, T> {
         // This drop guard will be invoked when predicate or `drop` of element panicked.
         // It shifts unchecked elements to cover holes and `set_len` to the correct length.
         // In cases when predicate and `drop` never panick, it will be optimized out.
-        struct BackshiftOnDrop<'a, T, A: Allocator> {
-            v: &'a mut Vec<T, A>,
+        struct BackshiftOnDrop<'bump, 'a, T> {
+            v: &'a mut Vec<'bump, T>,
             processed_len: usize,
             deleted_cnt: usize,
             original_len: usize,
         }
 
-        impl<T, A: Allocator> Drop for BackshiftOnDrop<'_, T, A> {
+        impl<T> Drop for BackshiftOnDrop<'_, '_, T> {
             fn drop(&mut self) {
                 if self.deleted_cnt > 0 {
                     // SAFETY: Trailing unchecked items must be valid since we never touch them.
@@ -1373,10 +1376,10 @@ impl<'bump, T: 'bump> Vec<'bump, T> {
 
         let mut g = BackshiftOnDrop { v: self, processed_len: 0, deleted_cnt: 0, original_len };
 
-        fn process_loop<F, T, A: Allocator, const DELETED: bool>(
+        fn process_loop<F, T, const DELETED: bool>(
             original_len: usize,
             f: &mut F,
-            g: &mut BackshiftOnDrop<'_, T, A>,
+            g: &mut BackshiftOnDrop<'_, '_, T>,
         ) where
             F: FnMut(&mut T) -> bool,
         {
@@ -1409,10 +1412,10 @@ impl<'bump, T: 'bump> Vec<'bump, T> {
         }
 
         // Stage 1: Nothing was deleted.
-        process_loop::<F, T, A, false>(original_len, &mut f, &mut g);
+        process_loop::<F, T, false>(original_len, &mut f, &mut g);
 
         // Stage 2: Some elements were deleted.
-        process_loop::<F, T, A, true>(original_len, &mut f, &mut g);
+        process_loop::<F, T, true>(original_len, &mut f, &mut g);
 
         // All item are processed. This can be optimized to `set_len` by LLVM.
         drop(g);

@overlookmotel overlookmotel force-pushed the 03-11-feat_allocator_add_vec2_retain_mut_method branch from 8bdcb8c to 247b928 Compare March 13, 2025 03:02
@overlookmotel overlookmotel changed the base branch from graphite-base/9655 to main March 13, 2025 03:03
@overlookmotel overlookmotel force-pushed the 03-11-feat_allocator_add_vec2_retain_mut_method branch from 247b928 to fcb5728 Compare March 13, 2025 03:03
@overlookmotel
Copy link
Contributor

overlookmotel commented Mar 13, 2025

Brilliant. Thank you. That really is very little change.

The only odd thing is why have to change the lifetime bounds on Deref and DerefMut. I think possibly those lifetime bounds shouldn't be there anyway (fitzgen/bumpalo#171) and those same lifetime bounds on Vec and other impls are the source of the other lifetime problems in #9656. But I'm not 100% sure. Maybe they should be there, or some of them should, and some shouldn't? 🤔

@Dunqing
Copy link
Member Author

Dunqing commented Mar 13, 2025

The only odd thing is why have to change the lifetime bounds on Deref and DerefMut. I think possibly those lifetime bounds shouldn't be there anyway (fitzgen/bumpalo#171) and those same lifetime bounds on Vec and other impls are the source of the other lifetime problems in #9656. But I'm not 100% sure. Maybe they should be there, or some of them should, and some shouldn't? 🤔

Oh, this is a mistake change, this change is actually what I was trying to fix the lifetime problem in #9656. I don't know why they appear in this PR, anyway I will revert them.

@Dunqing Dunqing force-pushed the 03-11-feat_allocator_add_vec2_retain_mut_method branch from fcb5728 to ec2f8da Compare March 13, 2025 03:51
@overlookmotel
Copy link
Contributor

OK great. We can merge this one too then.

@overlookmotel overlookmotel added the 0-merge Merge with Graphite Merge Queue label Mar 13, 2025
Copy link
Contributor

overlookmotel commented Mar 13, 2025

Merge activity

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

OXC has a few places using this API, so we need to add this method before replacing allocator-api2's `Vec`. The implementation is copied from the https://doc.rust-lang.org/src/alloc/vec/mod.rs.html#2121-2123, and modified a little to make it fit the `Vec2`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category - New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants