diff --git a/pageserver/src/tenant/timeline/handle.rs b/pageserver/src/tenant/timeline/handle.rs index b8bc2ba2aa..d57776a2fe 100644 --- a/pageserver/src/tenant/timeline/handle.rs +++ b/pageserver/src/tenant/timeline/handle.rs @@ -46,19 +46,21 @@ //! //! The `HandleInner` is allocated as a `Arc>` and //! referenced weakly and strongly from various places which we are now illustrating. -//! For bevity, we will omit the `Arc>` part in the following. +//! For brevity, we will omit the `Arc>` part in the following and instead +//! use `strong ref` and `weak ref` when referring to the `Arc>` +//! or `Weak>`, respectively. //! //! - The `Handle` is a strong ref. //! - The `WeakHandle` is a weak ref. -//! - The only long-lived strong reference is stored in the `PerTimelineState`. -//! - The `Cache` stores a weak ref. +//! - The `PerTimelineState` contains a `HashMap`. +//! - The `Cache` is a `HashMap`. //! //! Lifetimes: //! - `WeakHandle` and `Handle`: single pagestream request. //! - `Cache`: single page service connection. //! - `PerTimelineState`: lifetime of the Timeline object (i.e., i.e., till `Timeline::shutdown`). //! -//! ## Request Handling Flow +//! ## Request Handling Flow (= filling and using the `Cache``) //! //! To dispatch a request, the page service connection calls `Cache::get`. //! @@ -86,7 +88,7 @@ //! > method invocation. //! //! Why do we want to avoid that? -//! Because the gate is a shared location in memory and entering it invovles +//! Because the gate is a shared location in memory and entering it involves //! bumping refcounts, which leads to cache contention if done frequently //! from multiple cores in parallel. //! @@ -94,17 +96,19 @@ //! That `Arc` is private to the `HandleInner` and hence to the connection. //! (Review the "Data Structures" section if that is unclear to you.) //! -//! A `WeakHandle` is a weak ref to the HandleInner. -//! When upgrading a `WeakHandle`, we acquire a strong ref to the `Arc`. +//! A `WeakHandle` is a weak ref to the `HandleInner`. +//! When upgrading a `WeakHandle`, we upgrade to a strong ref to the `HandleInner` and +//! further acquire an additional strong ref to the `Arc` inside it. //! Again, this manipulation of ref counts is is cheap because `Arc` is private to the connection. //! //! When downgrading a `Handle` to a `WeakHandle`, we drop the `Arc`. //! Again, this is cheap because the `Arc` is private to the connection. //! -//! In addition to the GateGuard, we need to provide `impl Deref for Handle` and `impl Deref for WeakHandle`. +//! In addition to the GateGuard, we need to provide `Deref` impls +//! for `Handle` or `WeakHandle`. //! For this, both `Handle` and `WeakHandle` need access to the `Arc`. -//! Upgrading a `WeakHandle` does not consume the `Handle`, so we can't move the `Arc` out of -//! `Handle` into `WeakHandle`. +//! Upgrading a `WeakHandle` does not consume the `Handle`, so we can't move the +//! `Arc` out of `Handle` into `WeakHandle`. //! We could clone the `Arc` when upgrading a `WeakHandle`, but that would cause contention //! on the shared memory location that trakcs the refcount of the `Arc`. //! Instead, we wrap the `Arc` into another `Arc`. @@ -125,7 +129,8 @@ //! ``` //! //! The former cycle is a memory leak if not broken. -//! The latter cycle further prevent the Timeline from shutting down. +//! The latter cycle further prevents the Timeline from shutting down +//! because we certainly won't drop the Timeline while the GateGuard is alive. //! Preventing shutdown is the whole point of this handle/cache system, //! but when the Timeline needs to shut down, we need to break the cycle. //! @@ -137,19 +142,17 @@ //! [`HandleInner::ShutDown`], which drops the only long-lived strong ref to the //! `Arc`. //! -//! -//! //! `PerTimelineState::shutdown` drops all the `HandleInners` it contains, //! thereby breaking the cycle. -//! It also initiates draining of already existing `Handle`s, by -//! poisoning things so that no new `HandleInner`'s can be tracked -//! in the shut down `PerTimelineState`. +//! It also initiates draining of already existing `Handle`s by +//! poisoning things so that no new `HandleInner`'s can be added +//! to the `PerTimelineState`, which will make subsequent `Cache::get` fail. //! //! Concurrently existing / already upgraded `Handle`s will extend the -//! existence of the cycle. +//! lifetime of the `Arc>` and hence cycles. //! However, since `Handle`s are short-lived and new `Handle`s are not -//! handed out after either `PerTimelineState::shutdown` or `Cache` drop, -//! that extension of the cycle is bounded. +//! handed out from `Cache::get` or `WeakHandle::upgrade` after +//! `PerTimelineState::shutdown`, that extension of the cycle is bounded. //! //! Concurrently existing `WeakHandle`s will fail to `upgrade()`: //! while they will succeed in upgradingtheir weak arc ref to a strong ref,