Merge pull request #78 from maciejhirsz/0.9.1

0.9.1

Author: maciejhirsz

Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "json"
-version = "0.9.0"
+version = "0.9.1"
 authors = ["Maciej Hirsz <maciej.hirsz@gmail.com>"]
 description = "JSON implementation in Rust"
 repository = "https://github.com/maciejhirsz/json-rust"

src/object.rs

@@ -115,13 +115,13 @@ impl Node {
     }
 
     #[inline(always)]
-    fn new(value: JsonValue) -> Node {
+    fn new(value: JsonValue, hash: u64, len: usize) -> Node {
         unsafe {
             Node {
                 key_buf: mem::uninitialized(),
-                key_len: 0,
+                key_len: len,
                 key_ptr: mem::uninitialized(),
-                key_hash: mem::uninitialized(),
+                key_hash: hash,
                 value: value,
                 left: 0,
                 right: 0,
@@ -134,20 +134,18 @@ impl Node {
     // `Node`, only once the `Node` is somewhere on the heap, a persisting
     // pointer to the key can be obtained.
     #[inline(always)]
-    fn attach_key(&mut self, key: &[u8], hash: u64) {
-        self.key_len = key.len();
-        self.key_hash = hash;
-        if key.len() <= KEY_BUF_LEN {
+    fn attach_key(&mut self, key: &[u8]) {
+        if self.key_len <= KEY_BUF_LEN {
             unsafe {
                 ptr::copy_nonoverlapping(
                     key.as_ptr(),
                     self.key_buf.as_mut_ptr(),
-                    key.len()
+                    self.key_len
                 );
             }
             self.key_ptr = self.key_buf.as_mut_ptr();
         } else {
-            let mut heap: Vec<u8> = key.to_vec();
+            let mut heap = key.to_vec();
             self.key_ptr = heap.as_mut_ptr();
             mem::forget(heap);
         }
@@ -230,6 +228,7 @@ pub struct Object {
 impl Object {
     /// Create a new, empty instance of `Object`. Empty `Object` performs no
     /// allocation until a value is inserted into it.
+    #[inline(always)]
     pub fn new() -> Self {
         Object {
             store: Vec::new()
@@ -238,23 +237,24 @@ impl Object {
 
     /// Create a new `Object` with memory preallocated for `capacity` number
     /// of entries.
+    #[inline(always)]
     pub fn with_capacity(capacity: usize) -> Self {
         Object {
             store: Vec::with_capacity(capacity)
         }
     }
 
+    #[inline(always)]
     fn node_at_index<'a>(&self, index: usize) -> &'a Node {
-        let store_ptr = self.store.as_ptr();
         unsafe {
-            &*store_ptr.offset(index as isize)
+            &*self.store.as_ptr().offset(index as isize)
         }
     }
 
+    #[inline(always)]
     fn node_at_index_mut<'a>(&mut self, index: usize) -> &'a mut Node {
-        let store_ptr = self.store.as_mut_ptr();
         unsafe {
-            &mut *store_ptr.offset(index as isize)
+            &mut *self.store.as_mut_ptr().offset(index as isize)
         }
     }
 
@@ -263,15 +263,35 @@ impl Object {
         let index = self.store.len();
 
         if index < self.store.capacity() {
-            self.store.push(Node::new(value));
-            self.store[index].attach_key(key, hash);
+            // Because we've just checked the capacity, we can avoid
+            // using `push`, and instead do unsafe magic to memcpy
+            // the new node at the correct index without additional
+            // capacity or bound checks.
+            unsafe {
+                let node = Node::new(value, hash, key.len());
+                self.store.set_len(index + 1);
+
+                // To whomever gets concerned: I got better results with
+                // copy than write. Difference in benchmarks wasn't big though.
+                ptr::copy_nonoverlapping(
+                    &node as *const Node,
+                    self.store.as_mut_ptr().offset(index as isize),
+                    1,
+                );
+
+                // Since the Node has been copied, we need to forget about
+                // the owned value, else we may run into use after free.
+                mem::forget(node);
+            }
+            self.node_at_index_mut(index).attach_key(key);
         } else {
-            self.store.push(Node::new(value));
-            self.store[index].attach_key(key, hash);
+            self.store.push(Node::new(value, hash, key.len()));
+            self.node_at_index_mut(index).attach_key(key);
 
-            // FIXME: don't fix the last element again
-            for node in self.store.iter_mut() {
-                node.fix_key_ptr();
+            // Index up to the index (old length), we don't need to fix
+            // anything on the Node that just got pushed.
+            for i in 0 .. index {
+                self.node_at_index_mut(i).fix_key_ptr();
             }
         }
 
@@ -287,8 +307,8 @@ impl Object {
         let hash = hash_key(key);
 
         if self.store.len() == 0 {
-            self.store.push(Node::new(value));
-            self.store[0].attach_key(key, hash);
+            self.store.push(Node::new(value, hash, key.len()));
+            self.store[0].attach_key(key);
             return;
         }
 
@@ -430,10 +450,12 @@ impl Object {
         Some(removed)
     }
 
+    #[inline(always)]
     pub fn len(&self) -> usize {
         self.store.len()
     }
 
+    #[inline(always)]
     pub fn is_empty(&self) -> bool {
         self.store.is_empty()
     }