From 787aaad977b058cd40ab1dba8e4089af8478b3c5 Mon Sep 17 00:00:00 2001
From: overlookmotel <557937+overlookmotel@users.noreply.github.com>
Date: Mon, 20 Jan 2025 12:30:20 +0000
Subject: [PATCH] perf(allocator): make `String` non-drop (#8617)

Wrap `bumpalo::collections::String` in `ManuallyDrop` inside our `String` type.

This has 2 advantages:

1. Perf improvement (although it's very minor, because we don't use owned `String` type much).
2. `String`s can be stored in `Allocator` if you want to (#8570 made that impossible, if `String` is `Drop`).
---
 crates/oxc_allocator/src/string.rs | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/crates/oxc_allocator/src/string.rs b/crates/oxc_allocator/src/string.rs
index 796f2b3b509da..f6684af5dcecd 100644
--- a/crates/oxc_allocator/src/string.rs
+++ b/crates/oxc_allocator/src/string.rs
@@ -22,8 +22,15 @@ use crate::{Allocator, Vec};
 ///
 /// UTF-8 encoded, growable string. Identical to [`std::string::String`] except that it stores
 /// string contents in arena allocator.
+//
+// We wrap the inner `BumpaloString` in `ManuallyDrop` to make `String` non-Drop.
+// `bumpalo::collections::String` is a wrapper around `bumpalo::collections::Vec<u8>`.
+// Even though a `Vec<u8>` cannot require dropping (because `u8` is not `Drop`), `Vec<u8>` still has
+// a `Drop` impl, which means `bumpalo::collections::String` is `Drop` too.
+// We want to make it clear to compiler that `String` doesn't require dropping, so it doesn't
+// produce pointless "drop guard" code to handle dropping a `String` in case of a panic.
 #[derive(PartialOrd, Eq, Ord)]
-pub struct String<'alloc>(BumpaloString<'alloc>);
+pub struct String<'alloc>(ManuallyDrop<BumpaloString<'alloc>>);
 
 impl<'alloc> String<'alloc> {
     /// Creates a new empty [`String`].
@@ -38,7 +45,7 @@ impl<'alloc> String<'alloc> {
     /// [`with_capacity_in`]: String::with_capacity_in
     #[inline(always)]
     pub fn new_in(allocator: &'alloc Allocator) -> String<'alloc> {
-        Self(BumpaloString::new_in(allocator.bump()))
+        Self(ManuallyDrop::new(BumpaloString::new_in(allocator.bump())))
     }
 
     /// Creates a new empty [`String`] with specified capacity.
@@ -57,7 +64,7 @@ impl<'alloc> String<'alloc> {
     /// [`new_in`]: String::new_in
     #[inline(always)]
     pub fn with_capacity_in(capacity: usize, allocator: &'alloc Allocator) -> String<'alloc> {
-        Self(BumpaloString::with_capacity_in(capacity, allocator.bump()))
+        Self(ManuallyDrop::new(BumpaloString::with_capacity_in(capacity, allocator.bump())))
     }
 
     /// Construct a new [`String`] from a string slice.
@@ -73,7 +80,7 @@ impl<'alloc> String<'alloc> {
     /// ```
     #[inline(always)]
     pub fn from_str_in(s: &str, allocator: &'alloc Allocator) -> String<'alloc> {
-        Self(BumpaloString::from_str_in(s, allocator.bump()))
+        Self(ManuallyDrop::new(BumpaloString::from_str_in(s, allocator.bump())))
     }
 
     /// Convert `Vec<u8>` into [`String`].
@@ -105,7 +112,7 @@ impl<'alloc> String<'alloc> {
         // Lifetime of returned `String` is same as lifetime of original `Vec<u8>`.
         let inner = ManuallyDrop::into_inner(bytes.0);
         let (ptr, len, capacity, bump) = inner.into_raw_parts_with_alloc();
-        Self(BumpaloString::from_raw_parts_in(ptr, len, capacity, bump))
+        Self(ManuallyDrop::new(BumpaloString::from_raw_parts_in(ptr, len, capacity, bump)))
     }
 
     /// Creates a new [`String`] from a length, capacity, and pointer.
@@ -137,8 +144,6 @@ impl<'alloc> String<'alloc> {
     ///     let len = s.len();
     ///     let capacity = s.capacity();
     ///
-    ///     mem::forget(s);
-    ///
     ///     let s = String::from_raw_parts_in(ptr, len, capacity, &allocator);
     ///
     ///     assert_eq!(s, "hello");
@@ -153,7 +158,8 @@ impl<'alloc> String<'alloc> {
         allocator: &'alloc Allocator,
     ) -> String<'alloc> {
         // SAFETY: Safety conditions of this method are the same as `BumpaloString`'s method
-        Self(BumpaloString::from_raw_parts_in(buf, length, capacity, allocator.bump()))
+        let inner = BumpaloString::from_raw_parts_in(buf, length, capacity, allocator.bump());
+        Self(ManuallyDrop::new(inner))
     }
 
     /// Convert this `String<'alloc>` into an `&'alloc str`. This is analogous to
@@ -170,7 +176,8 @@ impl<'alloc> String<'alloc> {
     /// ```
     #[inline(always)]
     pub fn into_bump_str(self) -> &'alloc str {
-        self.0.into_bump_str()
+        let inner = ManuallyDrop::into_inner(self.0);
+        inner.into_bump_str()
     }
 }