From 79e765d53c9db6de8e7c90d5c722a813d5ddee5c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=EA=B0=95=EB=8F=99=EC=9C=A4=20=28Donny=29?= Date: Tue, 18 Feb 2025 19:21:52 +0900 Subject: [PATCH 1/8] Rename --- crates/hstr/src/dynamic.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/crates/hstr/src/dynamic.rs b/crates/hstr/src/dynamic.rs index 1bbe29c50dbe..d52ad7ada16d 100644 --- a/crates/hstr/src/dynamic.rs +++ b/crates/hstr/src/dynamic.rs @@ -70,13 +70,13 @@ impl Default for AtomStore { impl AtomStore { #[inline(always)] pub fn atom<'a>(&mut self, text: impl Into>) -> Atom { - new_atom(self, text.into()) + atom_in(self, &text.into()) } } /// This can create any kind of [Atom], although this lives in the `dynamic` /// module. -pub(crate) fn new_atom(storage: S, text: Cow) -> Atom +pub(crate) fn atom_in(storage: S, text: &str) -> Atom where S: Storage, { @@ -92,7 +92,7 @@ where return Atom { unsafe_data }; } - let hash = calc_hash(&text); + let hash = calc_hash(text); let entry = storage.insert_entry(text, hash); let entry = ThinArc::into_raw(entry.0) as *mut c_void; @@ -107,12 +107,12 @@ where } pub(crate) trait Storage { - fn insert_entry(self, text: Cow, hash: u64) -> Item; + fn insert_entry(self, text: &str, hash: u64) -> Item; } impl Storage for &'_ mut AtomStore { #[inline(never)] - fn insert_entry(self, text: Cow, hash: u64) -> Item { + fn insert_entry(self, text: &str, hash: u64) -> Item { // If the text is too long, interning is not worth it. if text.len() > 512 { return Item(ThinArc::from_header_and_slice( From 4889cd0cee9a1ce9c011e51357978749373ebd0d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=EA=B0=95=EB=8F=99=EC=9C=A4=20=28Donny=29?= Date: Tue, 18 Feb 2025 19:23:14 +0900 Subject: [PATCH 2/8] refactor --- crates/hstr/src/dynamic.rs | 13 +++++++++++++ crates/hstr/src/global_store.rs | 20 ++++---------------- 2 files changed, 17 insertions(+), 16 deletions(-) diff --git a/crates/hstr/src/dynamic.rs b/crates/hstr/src/dynamic.rs index d52ad7ada16d..a7f7acc528b5 100644 --- a/crates/hstr/src/dynamic.rs +++ b/crates/hstr/src/dynamic.rs @@ -1,5 +1,6 @@ use std::{ borrow::Cow, + cell::RefCell, ffi::c_void, hash::{BuildHasherDefault, Hash, Hasher}, mem::ManuallyDrop, @@ -74,6 +75,18 @@ impl AtomStore { } } +pub(crate) fn global_atom(text: &str) -> Atom { + thread_local! { + static GLOBAL_DATA: RefCell = Default::default(); + } + + GLOBAL_DATA.with(|global| { + let mut store = global.borrow_mut(); + + store.atom(text) + }) +} + /// This can create any kind of [Atom], although this lives in the `dynamic` /// module. pub(crate) fn atom_in(storage: S, text: &str) -> Atom diff --git a/crates/hstr/src/global_store.rs b/crates/hstr/src/global_store.rs index af08ad1e0b44..6136c747b6ee 100644 --- a/crates/hstr/src/global_store.rs +++ b/crates/hstr/src/global_store.rs @@ -1,24 +1,12 @@ -use std::{borrow::Cow, cell::RefCell}; +use std::borrow::Cow; -use crate::{Atom, AtomStore}; - -fn atom(text: Cow) -> Atom { - thread_local! { - static GLOBAL_DATA: RefCell = Default::default(); - } - - GLOBAL_DATA.with(|global| { - let mut store = global.borrow_mut(); - - store.atom(text) - }) -} +use crate::{dynamic::global_atom, Atom}; macro_rules! direct_from_impl { ($T:ty) => { impl From<$T> for Atom { fn from(s: $T) -> Self { - atom(s.into()) + global_atom(&s) } } }; @@ -30,6 +18,6 @@ direct_from_impl!(String); impl From> for crate::Atom { fn from(s: Box) -> Self { - atom(Cow::Owned(String::from(s))) + global_atom(&s) } } From b2139afa1c5c90befd06fa57f681de996e7116be Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=EA=B0=95=EB=8F=99=EC=9C=A4=20=28Donny=29?= Date: Tue, 18 Feb 2025 19:41:35 +0900 Subject: [PATCH 3/8] is global --- crates/hstr/src/dynamic.rs | 89 ++++++++++++++++++++++++++++++-------- 1 file changed, 70 insertions(+), 19 deletions(-) diff --git a/crates/hstr/src/dynamic.rs b/crates/hstr/src/dynamic.rs index a7f7acc528b5..2ac2611d804f 100644 --- a/crates/hstr/src/dynamic.rs +++ b/crates/hstr/src/dynamic.rs @@ -3,9 +3,9 @@ use std::{ cell::RefCell, ffi::c_void, hash::{BuildHasherDefault, Hash, Hasher}, - mem::ManuallyDrop, + mem::{forget, ManuallyDrop}, ops::Deref, - ptr::NonNull, + ptr::{self, NonNull}, }; use rustc_hash::FxHasher; @@ -16,13 +16,26 @@ use crate::{ Atom, INLINE_TAG_INIT, LEN_OFFSET, TAG_MASK, }; +#[derive(PartialEq, Eq)] pub(crate) struct Metadata { pub hash: u64, + pub is_global: bool, } -#[derive(Clone)] +#[derive(Clone, PartialEq, Eq)] pub(crate) struct Item(pub ThinArc, u8>); +impl Item { + /// https://users.rust-lang.org/t/what-is-the-ideal-way-to-destruct-a-guard-type/25974/8 + fn into_inner(self) -> ThinArc, u8> { + unsafe { + let inner = ptr::read(&self.0); + forget(self); + inner + } + } +} + impl Deref for Item { type Target = , u8> as Deref>::Target; @@ -31,9 +44,6 @@ impl Deref for Item { } } -/// TODO: Use real weak pointer -type WeakItem = Item; - impl Hash for Item { fn hash(&self, state: &mut H) { state.write_u64(self.0.header.header.header.hash); @@ -57,7 +67,7 @@ pub(crate) unsafe fn restore_arc(v: TaggedValue) -> Item { /// /// # Merging [AtomStore] pub struct AtomStore { - pub(crate) data: hashbrown::HashMap, + pub(crate) data: hashbrown::HashMap, } impl Default for AtomStore { @@ -71,25 +81,38 @@ impl Default for AtomStore { impl AtomStore { #[inline(always)] pub fn atom<'a>(&mut self, text: impl Into>) -> Atom { - atom_in(self, &text.into()) + atom_in(self, &text.into(), false) } } -pub(crate) fn global_atom(text: &str) -> Atom { - thread_local! { - static GLOBAL_DATA: RefCell = Default::default(); +impl Drop for Item { + fn drop(&mut self) { + // If we are going to drop the last reference, we need to remove the + // entry from the global store if it is a global atom + if self.0.header.header.header.is_global && ThinArc::strong_count(&self.0) == 2 { + GLOBAL_DATA.with(|global| { + let mut store = global.borrow_mut(); + store.data.remove(self); + }); + } } +} + +thread_local! { + static GLOBAL_DATA: RefCell = Default::default(); +} +pub(crate) fn global_atom(text: &str) -> Atom { GLOBAL_DATA.with(|global| { let mut store = global.borrow_mut(); - store.atom(text) + atom_in(&mut *store, text, true) }) } /// This can create any kind of [Atom], although this lives in the `dynamic` /// module. -pub(crate) fn atom_in(storage: S, text: &str) -> Atom +pub(crate) fn atom_in(storage: S, text: &str, is_global: bool) -> Atom where S: Storage, { @@ -106,8 +129,8 @@ where } let hash = calc_hash(text); - let entry = storage.insert_entry(text, hash); - let entry = ThinArc::into_raw(entry.0) as *mut c_void; + let entry = storage.insert_entry(text, hash, is_global); + let entry = ThinArc::into_raw(entry.into_inner()) as *mut c_void; let ptr: NonNull = unsafe { // Safety: Arc::into_raw returns a non-null pointer @@ -120,16 +143,16 @@ where } pub(crate) trait Storage { - fn insert_entry(self, text: &str, hash: u64) -> Item; + fn insert_entry(self, text: &str, hash: u64, is_global: bool) -> Item; } impl Storage for &'_ mut AtomStore { #[inline(never)] - fn insert_entry(self, text: &str, hash: u64) -> Item { + fn insert_entry(self, text: &str, hash: u64, is_global: bool) -> Item { // If the text is too long, interning is not worth it. if text.len() > 512 { return Item(ThinArc::from_header_and_slice( - HeaderWithLength::new(Metadata { hash }, text.len()), + HeaderWithLength::new(Metadata { hash, is_global }, text.len()), text.as_bytes(), )); } @@ -143,7 +166,7 @@ impl Storage for &'_ mut AtomStore { .or_insert_with(move || { ( Item(ThinArc::from_header_and_slice( - HeaderWithLength::new(Metadata { hash }, text.len()), + HeaderWithLength::new(Metadata { hash, is_global }, text.len()), text.as_bytes(), )), (), @@ -200,3 +223,31 @@ impl Hasher for EntryHasher { self.hash = val; } } + +#[cfg(test)] +mod tests { + use crate::{dynamic::GLOBAL_DATA, Atom}; + + #[test] + fn global_ref_count_dynamic() { + let atom1 = Atom::new("Hello, world!"); + + let atom2 = Atom::new("Hello, world!"); + + // 2 for the two atoms, 1 for the global store + assert_eq!(atom1.ref_count(), 3); + assert_eq!(atom2.ref_count(), 3); + + drop(atom1); + + // 1 for the atom2, 1 for the global store + assert_eq!(atom2.ref_count(), 2); + + drop(atom2); + + GLOBAL_DATA.with(|global| { + let store = global.borrow(); + assert_eq!(store.data.len(), 0); + }); + } +} From a0da9361a24c5efef2db26df2e94772c8e793cba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=EA=B0=95=EB=8F=99=EC=9C=A4=20=28Donny=29?= Date: Tue, 18 Feb 2025 19:51:08 +0900 Subject: [PATCH 4/8] fix --- crates/hstr/src/dynamic.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/hstr/src/dynamic.rs b/crates/hstr/src/dynamic.rs index 2ac2611d804f..65b473bc6d13 100644 --- a/crates/hstr/src/dynamic.rs +++ b/crates/hstr/src/dynamic.rs @@ -92,7 +92,7 @@ impl Drop for Item { if self.0.header.header.header.is_global && ThinArc::strong_count(&self.0) == 2 { GLOBAL_DATA.with(|global| { let mut store = global.borrow_mut(); - store.data.remove(self); + forget(store.data.remove_entry(self)); }); } } From 2a30e3fad185e441eb35c17c0efef4bfe22fffd2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=EA=B0=95=EB=8F=99=EC=9C=A4=20=28Donny=29?= Date: Wed, 19 Feb 2025 11:13:50 +0900 Subject: [PATCH 5/8] forget from outside --- crates/hstr/src/dynamic.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/crates/hstr/src/dynamic.rs b/crates/hstr/src/dynamic.rs index 65b473bc6d13..5e0d3f4bdd5e 100644 --- a/crates/hstr/src/dynamic.rs +++ b/crates/hstr/src/dynamic.rs @@ -90,10 +90,12 @@ impl Drop for Item { // If we are going to drop the last reference, we need to remove the // entry from the global store if it is a global atom if self.0.header.header.header.is_global && ThinArc::strong_count(&self.0) == 2 { - GLOBAL_DATA.with(|global| { + let v = GLOBAL_DATA.with(|global| { let mut store = global.borrow_mut(); - forget(store.data.remove_entry(self)); + store.data.remove_entry(self) }); + + forget(v); } } } From 0f4db0d17b858fe4e18685af15e8df5b83c69705 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=EA=B0=95=EB=8F=99=EC=9C=A4=20=28Donny=29?= Date: Wed, 19 Feb 2025 11:21:00 +0900 Subject: [PATCH 6/8] inner --- crates/hstr/src/dynamic.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/crates/hstr/src/dynamic.rs b/crates/hstr/src/dynamic.rs index 5e0d3f4bdd5e..d7b7af90d1e3 100644 --- a/crates/hstr/src/dynamic.rs +++ b/crates/hstr/src/dynamic.rs @@ -95,7 +95,9 @@ impl Drop for Item { store.data.remove_entry(self) }); - forget(v); + if let Some((v, _)) = v { + v.into_inner(); + } } } } From e2a7d347e7a453eefd2e0b06e7d88225f8189023 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=EA=B0=95=EB=8F=99=EC=9C=A4=20=28Donny=29?= Date: Wed, 19 Feb 2025 11:27:55 +0900 Subject: [PATCH 7/8] try_with --- crates/hstr/src/dynamic.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/hstr/src/dynamic.rs b/crates/hstr/src/dynamic.rs index d7b7af90d1e3..2b127101c15c 100644 --- a/crates/hstr/src/dynamic.rs +++ b/crates/hstr/src/dynamic.rs @@ -90,12 +90,12 @@ impl Drop for Item { // If we are going to drop the last reference, we need to remove the // entry from the global store if it is a global atom if self.0.header.header.header.is_global && ThinArc::strong_count(&self.0) == 2 { - let v = GLOBAL_DATA.with(|global| { + let v = GLOBAL_DATA.try_with(|global| { let mut store = global.borrow_mut(); store.data.remove_entry(self) }); - if let Some((v, _)) = v { + if let Ok(Some((v, _))) = v { v.into_inner(); } } From e247ebff8094b0a964fbd602fbc31a12de3d02b3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Donny/=EA=B0=95=EB=8F=99=EC=9C=A4?= Date: Wed, 19 Feb 2025 12:02:33 +0900 Subject: [PATCH 8/8] Create chatty-horses-invent.md --- .changeset/chatty-horses-invent.md | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 .changeset/chatty-horses-invent.md diff --git a/.changeset/chatty-horses-invent.md b/.changeset/chatty-horses-invent.md new file mode 100644 index 000000000000..818934ee85e8 --- /dev/null +++ b/.changeset/chatty-horses-invent.md @@ -0,0 +1,6 @@ +--- +hstr: patch +swc_core: patch +--- + +fix(hstr): Prevent memory leak for global stores