From dfa7e17960f47a38bc41075fffab1ad9cba89f81 Mon Sep 17 00:00:00 2001 From: Sean McArthur Date: Fri, 4 May 2018 15:36:20 -0700 Subject: [PATCH] refactor(client): change last Weak::new to an Option --- src/client/pool.rs | 65 +++++++++++++++++++++++++++------------------- 1 file changed, 38 insertions(+), 27 deletions(-) diff --git a/src/client/pool.rs b/src/client/pool.rs index 658b1c29..c86577c6 100644 --- a/src/client/pool.rs +++ b/src/client/pool.rs @@ -50,11 +50,6 @@ type Key = (Arc, Ver); struct PoolInner { connections: Mutex>, enabled: bool, - /// A single Weak pointer used every time a proper weak reference - /// is not needed. This prevents allocating space in the heap to hold - /// a PoolInner *every single time*, and instead we just allocate - /// this one extra per pool. - weak: Weak>, } struct Connections { @@ -84,6 +79,10 @@ struct Connections { timeout: Option, } +// This is because `Weak::new()` *allocates* space for `T`, even if it +// doesn't need it! +struct WeakOpt(Option>); + impl Pool { pub fn new(enabled: bool, timeout: Option, __exec: &Exec) -> Pool { Pool { @@ -99,7 +98,6 @@ impl Pool { timeout, }), enabled, - weak: Weak::new(), }), } } @@ -136,7 +134,7 @@ impl Pool { if inner.connecting.insert(key.clone()) { let connecting = Connecting { key: key.clone(), - pool: Arc::downgrade(&self.inner), + pool: WeakOpt::downgrade(&self.inner), }; Some(connecting) } else { @@ -148,7 +146,7 @@ impl Pool { key: key.clone(), // in HTTP/1's case, there is never a lock, so we don't // need to do anything in Drop. - pool: self.inner.weak.clone(), + pool: WeakOpt::none(), }) } } @@ -189,7 +187,7 @@ impl Pool { } pub(super) fn pooled(&self, mut connecting: Connecting, value: T) -> Pooled { - let (value, pool_ref, has_pool) = if self.inner.enabled { + let (value, pool_ref) = if self.inner.enabled { match value.reserve() { Reservation::Shared(to_insert, to_return) => { debug_assert_eq!( @@ -203,17 +201,17 @@ impl Pool { // already have a lock, no need to lock the mutex twice. inner.connected(&connecting.key); // prevent the Drop of Connecting from repeating inner.connected() - connecting.pool = self.inner.weak.clone(); + connecting.pool = WeakOpt::none(); // Shared reservations don't need a reference to the pool, // since the pool always keeps a copy. - (to_return, self.inner.weak.clone(), false) + (to_return, WeakOpt::none()) }, Reservation::Unique(value) => { // Unique reservations must take a reference to the pool // since they hope to reinsert once the reservation is // completed - (value, Arc::downgrade(&self.inner), true) + (value, WeakOpt::downgrade(&self.inner)) }, } } else { @@ -222,11 +220,10 @@ impl Pool { // The Connecting should have had no pool ref debug_assert!(connecting.pool.upgrade().is_none()); - (value, self.inner.weak.clone(), false) + (value, WeakOpt::none()) }; Pooled { key: connecting.key.clone(), - has_pool, is_reused: false, pool: pool_ref, value: Some(value) @@ -243,14 +240,13 @@ impl Pool { // we just have the final value, without knowledge of if this is // unique or shared. So, the hack is to just assume Ver::Http2 means // shared... :( - let (pool_ref, has_pool) = if key.1 == Ver::Http2 { - (self.inner.weak.clone(), false) + let pool_ref = if key.1 == Ver::Http2 { + WeakOpt::none() } else { - (Arc::downgrade(&self.inner), true) + WeakOpt::downgrade(&self.inner) }; Pooled { - has_pool, is_reused: true, key: key.clone(), pool: pool_ref, @@ -414,7 +410,7 @@ impl Connections { let interval = Interval::new(start, dur); self.exec.execute(IdleInterval { interval: interval, - pool: Arc::downgrade(pool_ref), + pool: WeakOpt::downgrade(pool_ref), pool_drop_notifier: rx, }); } @@ -481,10 +477,9 @@ impl Clone for Pool { // Note: The bounds `T: Poolable` is needed for the Drop impl. pub(super) struct Pooled { value: Option, - has_pool: bool, is_reused: bool, key: Key, - pool: Weak>, + pool: WeakOpt>, } impl Pooled { @@ -493,7 +488,7 @@ impl Pooled { } pub fn is_pool_enabled(&self) -> bool { - self.has_pool + self.pool.0.is_some() } fn as_ref(&self) -> &T { @@ -628,7 +623,7 @@ impl Drop for Checkout { pub(super) struct Connecting { key: Key, - pool: Weak>, + pool: WeakOpt>, } impl Drop for Connecting { @@ -665,7 +660,7 @@ impl Expiration { #[cfg(feature = "runtime")] struct IdleInterval { interval: Interval, - pool: Weak>, + pool: WeakOpt>, // This allows the IdleInterval to be notified as soon as the entire // Pool is fully dropped, and shutdown. This channel is never sent on, // but Err(Canceled) will be received when the Pool is dropped. @@ -707,14 +702,30 @@ impl Future for IdleInterval { } } +impl WeakOpt { + fn none() -> Self { + WeakOpt(None) + } + + fn downgrade(arc: &Arc) -> Self { + WeakOpt(Some(Arc::downgrade(arc))) + } + + fn upgrade(&self) -> Option> { + self.0 + .as_ref() + .and_then(Weak::upgrade) + } +} + #[cfg(test)] mod tests { - use std::sync::{Arc, Weak}; + use std::sync::Arc; use std::time::Duration; use futures::{Async, Future}; use futures::future; use common::Exec; - use super::{Connecting, Key, Poolable, Pool, Reservation, Ver}; + use super::{Connecting, Key, Poolable, Pool, Reservation, Ver, WeakOpt}; /// Test unique reservations. #[derive(Debug, PartialEq, Eq)] @@ -733,7 +744,7 @@ mod tests { fn c(key: Key) -> Connecting { Connecting { key, - pool: Weak::new(), + pool: WeakOpt::none(), } }