From 41a11afe7c540f072450b3b6d02a5396f16d1164 Mon Sep 17 00:00:00 2001 From: Ihor Andrianov Date: Sat, 28 Jun 2025 14:24:59 +0300 Subject: [PATCH 1/6] leaking box memory --- core/Cargo.toml | 1 + core/storage/page_cache.rs | 35 +++++++++++++++++++++++++++++++++-- 2 files changed, 34 insertions(+), 2 deletions(-) diff --git a/core/Cargo.toml b/core/Cargo.toml index 484d8b45d..e77b5dc37 100644 --- a/core/Cargo.toml +++ b/core/Cargo.toml @@ -91,6 +91,7 @@ built = { version = "0.7.5", features = ["git2", "chrono"] } pprof = { version = "0.14.0", features = ["criterion", "flamegraph"] } [dev-dependencies] +memory-stats = "1.2.0" criterion = { version = "0.5", features = [ "html_reports", "async", diff --git a/core/storage/page_cache.rs b/core/storage/page_cache.rs index ba4e10de2..664d03119 100644 --- a/core/storage/page_cache.rs +++ b/core/storage/page_cache.rs @@ -1,3 +1,4 @@ +use std::hash::{DefaultHasher, Hash, Hasher}; use std::{cell::RefCell, ptr::NonNull}; use std::sync::Arc; @@ -138,7 +139,9 @@ impl DumbLruPageCache { // Try to detach from LRU list first, can fail self.detach(ptr, clean_page)?; let ptr = self.map.borrow_mut().remove(&key).unwrap(); - unsafe { std::ptr::drop_in_place(ptr.as_ptr()) }; + unsafe { + let _ = Box::from_raw(ptr.as_ptr()); + }; Ok(()) } @@ -570,7 +573,11 @@ impl PageHashMap { } fn hash(&self, key: &PageCacheKey) -> usize { - key.pgno % self.capacity + if self.capacity.is_power_of_two() { + key.pgno & (self.capacity - 1) + } else { + key.pgno % self.capacity + } } pub fn rehash(&self, new_capacity: usize) -> PageHashMap { @@ -1184,4 +1191,28 @@ mod tests { cache.verify_list_integrity(); assert!(cache.insert(create_key(4), page_with_content(4)).is_ok()); } + + #[test] + fn test_memory_leak_fix() { + let mut cache = DumbLruPageCache::new(10); + + let initial_memory = memory_stats::memory_stats().unwrap(); + let initial_memory_virtual = initial_memory.virtual_mem; + let intial_memory_resident = initial_memory.physical_mem; + + for i in 0..10000 { + let key = create_key(i); + let page = page_with_content(i); + let _ = cache.insert(key, page); + } + + drop(cache); + + let final_memory = memory_stats::memory_stats().unwrap(); + let final_memory_virtual = final_memory.virtual_mem; + let final_memory_resident = final_memory.physical_mem; + + assert!(final_memory_virtual - initial_memory_virtual < 1_000_000); + assert!(final_memory_resident - intial_memory_resident < 1_000_000); + } } From 56b1fcf3b33ce6904226623526d31466e8509b7a Mon Sep 17 00:00:00 2001 From: Ihor Andrianov Date: Sat, 28 Jun 2025 14:32:23 +0300 Subject: [PATCH 2/6] remove unused imports --- core/storage/page_cache.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/core/storage/page_cache.rs b/core/storage/page_cache.rs index 664d03119..ed0d8cafb 100644 --- a/core/storage/page_cache.rs +++ b/core/storage/page_cache.rs @@ -1,4 +1,3 @@ -use std::hash::{DefaultHasher, Hash, Hasher}; use std::{cell::RefCell, ptr::NonNull}; use std::sync::Arc; From 647183938f29458331d656061081f113ff0622bd Mon Sep 17 00:00:00 2001 From: Ihor Andrianov Date: Sat, 28 Jun 2025 14:41:29 +0300 Subject: [PATCH 3/6] fix sub with below 0 in tests --- core/storage/page_cache.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/storage/page_cache.rs b/core/storage/page_cache.rs index ed0d8cafb..d4519625d 100644 --- a/core/storage/page_cache.rs +++ b/core/storage/page_cache.rs @@ -1211,7 +1211,7 @@ mod tests { let final_memory_virtual = final_memory.virtual_mem; let final_memory_resident = final_memory.physical_mem; - assert!(final_memory_virtual - initial_memory_virtual < 1_000_000); - assert!(final_memory_resident - intial_memory_resident < 1_000_000); + assert!(final_memory_virtual.saturating_sub(initial_memory_virtual) < 1_000_000); + assert!(final_memory_resident.saturating_sub(intial_memory_resident) < 1_000_000); } } From 68e638e955b2ce7883d0752c237b6c2e2ee26d7d Mon Sep 17 00:00:00 2001 From: Ihor Andrianov Date: Sat, 28 Jun 2025 15:06:23 +0300 Subject: [PATCH 4/6] fix second occurance --- core/storage/page_cache.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/core/storage/page_cache.rs b/core/storage/page_cache.rs index d4519625d..4a74456c1 100644 --- a/core/storage/page_cache.rs +++ b/core/storage/page_cache.rs @@ -301,7 +301,9 @@ impl DumbLruPageCache { unsafe { assert!(!current_entry.as_ref().page.is_dirty()); } - unsafe { std::ptr::drop_in_place(current_entry.as_ptr()) }; + unsafe { + let _ = Box::from_raw(current_entry.as_ptr()); + }; current = next; } let _ = self.head.take(); @@ -1197,7 +1199,7 @@ mod tests { let initial_memory = memory_stats::memory_stats().unwrap(); let initial_memory_virtual = initial_memory.virtual_mem; - let intial_memory_resident = initial_memory.physical_mem; + let initial_memory_resident = initial_memory.physical_mem; for i in 0..10000 { let key = create_key(i); @@ -1212,6 +1214,6 @@ mod tests { let final_memory_resident = final_memory.physical_mem; assert!(final_memory_virtual.saturating_sub(initial_memory_virtual) < 1_000_000); - assert!(final_memory_resident.saturating_sub(intial_memory_resident) < 1_000_000); + assert!(final_memory_resident.saturating_sub(initial_memory_resident) < 1_000_000); } } From 564bb28dea15ec3706ed93fb2cdc167779bb49df Mon Sep 17 00:00:00 2001 From: Ihor Andrianov Date: Sat, 28 Jun 2025 15:38:10 +0300 Subject: [PATCH 5/6] rewrite test to make fix verifiable --- core/storage/page_cache.rs | 37 +++++++++++++++++++++---------------- 1 file changed, 21 insertions(+), 16 deletions(-) diff --git a/core/storage/page_cache.rs b/core/storage/page_cache.rs index 4a74456c1..f5aa9cfea 100644 --- a/core/storage/page_cache.rs +++ b/core/storage/page_cache.rs @@ -1194,26 +1194,31 @@ mod tests { } #[test] - fn test_memory_leak_fix() { - let mut cache = DumbLruPageCache::new(10); + #[ignore = "long running test, remove to verify"] + fn test_clear_memory_stability() { + let initial_memory = memory_stats::memory_stats().unwrap().physical_mem; - let initial_memory = memory_stats::memory_stats().unwrap(); - let initial_memory_virtual = initial_memory.virtual_mem; - let initial_memory_resident = initial_memory.physical_mem; + for _ in 0..100000 { + let mut cache = DumbLruPageCache::new(1000); - for i in 0..10000 { - let key = create_key(i); - let page = page_with_content(i); - let _ = cache.insert(key, page); + for i in 0..1000 { + let key = create_key(i); + let page = page_with_content(i); + cache.insert(key, page).unwrap(); + } + + cache.clear().unwrap(); + drop(cache); } - drop(cache); + let final_memory = memory_stats::memory_stats().unwrap().physical_mem; - let final_memory = memory_stats::memory_stats().unwrap(); - let final_memory_virtual = final_memory.virtual_mem; - let final_memory_resident = final_memory.physical_mem; - - assert!(final_memory_virtual.saturating_sub(initial_memory_virtual) < 1_000_000); - assert!(final_memory_resident.saturating_sub(initial_memory_resident) < 1_000_000); + let growth = final_memory.saturating_sub(initial_memory); + println!("Growth: {}", growth); + assert!( + growth < 10_000_000, + "Memory grew by {} bytes over 10 cycles", + growth + ); } } From c0aa67dccb8fc6d5ce0c0df8fc9eb5a2810ee894 Mon Sep 17 00:00:00 2001 From: Ihor Andrianov Date: Tue, 1 Jul 2025 17:28:34 +0300 Subject: [PATCH 6/6] rebase --- Cargo.lock | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/Cargo.lock b/Cargo.lock index 6d13648a9..81fd2b95c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2007,6 +2007,16 @@ dependencies = [ "autocfg", ] +[[package]] +name = "memory-stats" +version = "1.2.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "c73f5c649995a115e1a0220b35e4df0a1294500477f97a91d0660fb5abeb574a" +dependencies = [ + "libc", + "windows-sys 0.52.0", +] + [[package]] name = "miette" version = "7.6.0" @@ -3729,6 +3739,7 @@ dependencies = [ "limbo_time", "limbo_uuid", "lru", + "memory-stats", "miette", "mimalloc", "parking_lot",