LruCache clean ups and optimizations #13618
Conversation
Instead of allocating both head and tail nodes for the ends of the node list, a single node can be allocated and linked circularly instead, making it act as both the head and the tail of the list at the same time.
| key: None, | ||
| value: None, | ||
| key: unsafe { mem::init() }, | ||
| value: unsafe { mem::init() }, | ||
| next: ptr::mut_null(), | ||
| prev: ptr::mut_null(), | ||
| } |
There was a problem hiding this comment.
Could this whole function be replaced by a call to mem::uninit? It seems like it should work, as you never inspect the key and value and you immediately overwrite the pointers. I'm not sure if there are any downsides, but it would make this simpler.
There was a problem hiding this comment.
i think that works, yes. Is there any particular preference between mem::init and mem::uninit? I had uninit in there before for the fields but shied away from it to reduce any non-determinism. Assuming this has no bugs then it indeed shouldn't make a difference.
There was a problem hiding this comment.
The init function zeroes the memory. It's easier to find bugs with uninit because valgrind errors will be triggered. Either way is just as unsafe here.
|
I've updated the last commit with the changes proposed by @gereeter |
| @@ -102,41 +93,42 @@ impl<K: Hash + TotalEq, V> LruCache<K, V> { | |||
| let cache = LruCache { | |||
| map: HashMap::new(), | |||
| max_size: capacity, | |||
| head: unsafe{ cast::transmute(~LruEntry::<K, V>::new()) }, | |||
| tail: unsafe{ cast::transmute(~LruEntry::<K, V>::new()) }, | |||
| head: unsafe{ cast::transmute(~mem::uninit::<LruCache<K, V>>()) }, | |||
There was a problem hiding this comment.
Should this be LruEntry instead of LruCache?
LruEntry nodes previously used Option to encapsulate the key and value fields. This was used merely as a way avoid having values for the sigil node. Apart from wasting a few bytes for the discriminant, this cluttered the rest of the code, since these fields always contained Some on regular nodes as a class invariant. The Option wrapping was removed, and the values in the sigil field are initialized using mem::init, so that they don't contain any real data.
Just a few space saving optimizations that end up making the code less cluttered too. I'd like to someone to review the last commit closely, I don't have much experience with writing unsafe code, I had someone walk me through how to use cast::forget in IRC.
Just a few space saving optimizations that end up making the code less cluttered too. I'd like to someone to review the last commit closely, I don't have much experience with writing unsafe code, I had someone walk me through how to use cast::forget in IRC.