Addressing reviewer's comments

alexcrichton · Nov 5, 2020 · b99a307 · b99a307
1 parent 0272ed2
commit b99a307
Show file tree

Hide file tree

Showing 8 changed files with 240 additions and 177 deletions.
diff --git a/src/dlmalloc.rs b/src/dlmalloc.rs
@@ -4,13 +4,12 @@
 // The original source was written by Doug Lea and released to the public domain
 
 use core::cmp;
-use core::marker::PhantomData;
 use core::mem;
 use core::ptr;
 
-use System;
+use Allocator;
 
-pub struct Dlmalloc<S> {
+pub struct Dlmalloc<A> {
     smallmap: u32,
     treemap: u32,
     smallbins: [*mut Chunk; (NSMALLBINS + 1) * 2],
@@ -25,9 +24,9 @@ pub struct Dlmalloc<S> {
     trim_check: usize,
     least_addr: *mut u8,
     release_checks: usize,
-    _system_allocator: PhantomData<S>,
+    system_allocator: A,
 }
-unsafe impl<S> Send for Dlmalloc<S> {}
+unsafe impl<A: Send> Send for Dlmalloc<A> {}
 
 // TODO: document this
 const NSMALLBINS: usize = 32;
@@ -87,8 +86,8 @@ fn leftshift_for_tree_index(x: u32) -> u32 {
     }
 }
 
-impl<Sys> Dlmalloc<Sys> {
-    pub const fn init() -> Dlmalloc<Sys> {
+impl<A> Dlmalloc<A> {
+    pub const fn new(system_allocator: A) -> Dlmalloc<A> {
         Dlmalloc {
             smallmap: 0,
             treemap: 0,
@@ -109,12 +108,12 @@ impl<Sys> Dlmalloc<Sys> {
             trim_check: 0,
             least_addr: 0 as *mut _,
             release_checks: 0,
-            _system_allocator: PhantomData,
+            system_allocator,
         }
     }
 }
 
-impl<Sys: System> Dlmalloc<Sys> {
+impl<A: Allocator> Dlmalloc<A> {
     // TODO: can we get rid of this?
     pub fn malloc_alignment(&self) -> usize {
         mem::size_of::<usize>() * 2
@@ -231,7 +230,7 @@ impl<Sys: System> Dlmalloc<Sys> {
     }
 
     pub unsafe fn calloc_must_clear(&self, ptr: *mut u8) -> bool {
-        !Sys::allocates_zeros() || !Chunk::mmapped(Chunk::from_mem(ptr))
+        !self.system_allocator.allocates_zeros() || !Chunk::mmapped(Chunk::from_mem(ptr))
     }
 
     pub unsafe fn malloc(&mut self, size: usize) -> *mut u8 {
@@ -359,7 +358,7 @@ impl<Sys: System> Dlmalloc<Sys> {
             DEFAULT_GRANULARITY,
         );
 
-        let (tbase, tsize, flags) = Sys::alloc(asize);
+        let (tbase, tsize, flags) = self.system_allocator.alloc(asize);
         if tbase.is_null() {
             return tbase;
         }
@@ -540,7 +539,7 @@ impl<Sys: System> Dlmalloc<Sys> {
         let oldmmsize = oldsize + offset + self.mmap_foot_pad();
         let newmmsize =
             self.mmap_align(nb + 6 * mem::size_of::<usize>() + self.malloc_alignment() - 1);
-        let ptr = Sys::remap(
+        let ptr = self.system_allocator.remap(
             (oldp as *mut u8).offset(-(offset as isize)),
             oldmmsize,
             newmmsize,
@@ -562,7 +561,7 @@ impl<Sys: System> Dlmalloc<Sys> {
     }
 
     fn mmap_align(&self, a: usize) -> usize {
-        align_up(a, Sys::page_size())
+        align_up(a, self.system_allocator.page_size())
     }
 
     // Only call this with power-of-two alignment and alignment >
@@ -638,7 +637,10 @@ impl<Sys: System> Dlmalloc<Sys> {
             let prevsize = (*p).prev_foot;
             if Chunk::mmapped(p) {
                 psize += prevsize + self.mmap_foot_pad();
-                if Sys::free((p as *mut u8).offset(-(prevsize as isize)), psize) {
+                if self
+                    .system_allocator
+                    .free((p as *mut u8).offset(-(prevsize as isize)), psize)
+                {
                     self.footprint -= psize;
                 }
                 return;
@@ -1168,7 +1170,10 @@ impl<Sys: System> Dlmalloc<Sys> {
 
             if Chunk::mmapped(p) {
                 psize += prevsize + self.mmap_foot_pad();
-                if Sys::free((p as *mut u8).offset(-(prevsize as isize)), psize) {
+                if self
+                    .system_allocator
+                    .free((p as *mut u8).offset(-(prevsize as isize)), psize)
+                {
                     self.footprint -= psize;
                 }
                 return;
@@ -1249,10 +1254,13 @@ impl<Sys: System> Dlmalloc<Sys> {
                 debug_assert!(!sp.is_null());
 
                 if !Segment::is_extern(sp) {
-                    if Segment::can_release_part::<Sys>(sp) {
+                    if Segment::can_release_part(&self.system_allocator, sp) {
                         if (*sp).size >= extra && !self.has_segment_link(sp) {
                             let newsize = (*sp).size - extra;
-                            if Sys::free_part((*sp).base, (*sp).size, newsize) {
+                            if self
+                                .system_allocator
+                                .free_part((*sp).base, (*sp).size, newsize)
+                            {
                                 released = extra;
                             }
                         }
@@ -1302,7 +1310,7 @@ impl<Sys: System> Dlmalloc<Sys> {
             let next = (*sp).next;
             nsegs += 1;
 
-            if Segment::can_release_part::<Sys>(sp) && !Segment::is_extern(sp) {
+            if Segment::can_release_part(&self.system_allocator, sp) && !Segment::is_extern(sp) {
                 let p = self.align_as_chunk(base);
                 let psize = Chunk::size(p);
                 // We can unmap if the first chunk holds the entire segment and
@@ -1318,7 +1326,7 @@ impl<Sys: System> Dlmalloc<Sys> {
                     } else {
                         self.unlink_large_chunk(tp);
                     }
-                    if Sys::free(base, size) {
+                    if self.system_allocator.free(base, size) {
                         released += size;
                         self.footprint -= size;
                         // unlink our obsolete record
@@ -1412,7 +1420,7 @@ impl<Sys: System> Dlmalloc<Sys> {
         );
         debug_assert!(p as *mut u8 >= self.least_addr);
         debug_assert!(!self.is_small(sz));
-        debug_assert_eq!(align_up(len, Sys::page_size()), len);
+        debug_assert_eq!(align_up(len, self.system_allocator.page_size()), len);
         debug_assert_eq!((*Chunk::plus_offset(p, sz)).head, Chunk::fencepost_head());
         debug_assert_eq!(
             (*Chunk::plus_offset(p, sz + mem::size_of::<usize>())).head,
@@ -1753,8 +1761,8 @@ impl Segment {
         (*seg).flags & EXTERN != 0
     }
 
-    unsafe fn can_release_part<S: System>(seg: *mut Segment) -> bool {
-        S::can_release_part((*seg).flags >> 1)
+    unsafe fn can_release_part<A: Allocator>(system_allocator: &A, seg: *mut Segment) -> bool {
+        system_allocator.can_release_part((*seg).flags >> 1)
     }
 
     unsafe fn sys_flags(seg: *mut Segment) -> u32 {
@@ -1773,12 +1781,11 @@ impl Segment {
 #[cfg(test)]
 mod tests {
     use super::*;
-    #[cfg(any(target_os = "linux", target_arch = "wasm32", target_os = "macos"))]
-    use Platform;
+    use System;
 
     // Prime the allocator with some allocations such that there will be free
     // chunks in the treemap
-    unsafe fn setup_treemap<S: System>(a: &mut Dlmalloc<S>) {
+    unsafe fn setup_treemap<A: Allocator>(a: &mut Dlmalloc<A>) {
         let large_request_size = NSMALLBINS * (1 << SMALLBIN_SHIFT);
         assert!(!a.is_small(large_request_size));
         let large_request1 = a.malloc(large_request_size);
@@ -1792,9 +1799,8 @@ mod tests {
     #[test]
     // Test allocating, with a non-empty treemap, a specific size that used to
     // trigger an integer overflow bug
-    #[cfg(any(target_os = "linux", target_arch = "wasm32", target_os = "macos"))]
     fn treemap_alloc_overflow_minimal() {
-        let mut a: Dlmalloc<Platform> = Dlmalloc::init();
+        let mut a = Dlmalloc::new(System::new());
         unsafe {
             setup_treemap(&mut a);
             let min_idx31_size = (0xc000 << TREEBIN_SHIFT) - a.chunk_overhead() + 1;
@@ -1803,10 +1809,9 @@ mod tests {
     }
 
     #[test]
-    #[cfg(any(target_os = "linux", target_arch = "wasm32", target_os = "macos"))]
     // Test allocating the maximum request size with a non-empty treemap
     fn treemap_alloc_max() {
-        let mut a: Dlmalloc<Platform> = Dlmalloc::init();
+        let mut a = Dlmalloc::new(System::new());
         unsafe {
             setup_treemap(&mut a);
             let max_request_size = a.max_request() - 1;

diff --git a/src/dummy.rs b/src/dummy.rs
@@ -0,0 +1,42 @@
+use core::ptr;
+use Allocator;
+
+pub struct System {
+    _priv: (),
+}
+
+impl System {
+    const fn new() -> System {
+        System { _priv: () }
+    }
+}
+
+unsafe impl Allocator for System {
+    fn alloc(&self, size: usize) -> (*mut u8, usize, u32) {
+        (ptr::null_mut(), 0, 0)
+    }
+
+    fn remap(&self, ptr: *mut u8, oldsize: usize, newsize: usize, can_move: bool) -> *mut u8 {
+        ptr::null_mut()
+    }
+
+    fn free_part(&self, ptr: *mut u8, oldsize: usize, newsize: usize) -> bool {
+        false
+    }
+
+    fn free(&self, ptr: *mut u8, size: usize) -> bool {
+        false
+    }
+
+    fn can_release_part(&self, flags: u32) -> bool {
+        false
+    }
+
+    fn allocates_zeros(&self) -> bool {
+        false
+    }
+
+    fn page_size(&self) -> usize {
+        1
+    }
+}
diff --git a/src/global.rs b/src/global.rs
@@ -1,11 +1,9 @@
-use crate::{GlobalSystem, Platform};
 #[cfg(feature = "allocator-api")]
 use core::alloc::{AllocErr, AllocRef};
 use core::alloc::{GlobalAlloc, Layout};
 use core::ops::{Deref, DerefMut};
 #[cfg(feature = "allocator-api")]
 use core::ptr::NonNull;
-use DLMALLOC_INIT;
 
 use Dlmalloc;
 
@@ -55,12 +53,12 @@ unsafe impl AllocRef for GlobalDlmalloc {
     }
 }
 
-static mut DLMALLOC: Dlmalloc = DLMALLOC_INIT;
+static mut DLMALLOC: Dlmalloc = Dlmalloc::new();
 
 struct Instance;
 
 unsafe fn get() -> Instance {
-    Platform::acquire_global_lock();
+    ::sys::acquire_global_lock();
     Instance
 }
 
@@ -79,6 +77,6 @@ impl DerefMut for Instance {
 
 impl Drop for Instance {
     fn drop(&mut self) {
-        Platform::release_global_lock()
+        ::sys::release_global_lock()
     }
 }