Stop keeping all DataStore entries in-memory, add pagination - #1024
Stop keeping all DataStore entries in-memory, add pagination#1024tnull wants to merge 9 commits into
DataStore entries in-memory, add pagination#1024Conversation
The `WalletEvent::TxReplaced` handler read the payment from the store twice, once inside a `debug_assert!` and once for real, so the assertion and the value actually used could in principle disagree. Reuse a single lookup instead. Co-Authored-By: HAL 9000
`DataStore::get`, `contains_key` and `list_filter` were synchronous and infallible because every entry of a namespace is held in memory, so a lookup could never fail or block. That assumption goes away once a store may keep only a subset of its entries in memory and has to read through to the `KVStore` on a miss. Turn the readers into async methods and let `get` and `contains_key` report an error, so that a failed store read is never mistaken for "no such object". Behavior is unchanged: every reader still answers from memory and always returns `Ok`. `Node::payment` consequently returns a `Result`. Async and fallible are introduced together on purpose, so that adding the read-through paths later does not have to churn the same call sites twice. Co-Authored-By: HAL 9000
Mutations persist to the `KVStore` first and only then update the in-memory state, so that a failed write leaves memory untouched. The readers, however, did not wait on the mutation lock, so during that window they could hand out an object the store had already moved past. The in-code comments documented this as a known caveat. Now that the readers are async they can wait, so turn the mutation lock into a read-write lock: mutations take the write guard across both steps, readers take the read guard and therefore never observe the intermediate state. This also becomes load-bearing once entries may be read back from the store on a cache miss, because a reader that repopulates memory from a value it read before a concurrent write would otherwise leave memory durably disagreeing with the store. Readers now block for the duration of an in-flight write, which for a remote backend is one network round trip. Co-Authored-By: HAL 9000
`DataStore` held every object of its namespace in memory for the lifetime of the node. That is fine for the pending-payment store, which drops entries as payments resolve, but the payment store grows without bound, so memory use and startup time grow with a node's history. Give each store a caching policy, either keeping all entries as before, or keeping only a bounded number of least recently used ones and reading the rest back from the store on demand. Both existing stores keep all their entries, so nothing changes yet. The policy is a type parameter rather than a plain value so that `list_filter`, which can only answer correctly while everything is in memory, is unavailable on a bounded store. Reaching for a full scan where it would silently return a subset is a compile error. A bounded store also has to read through on its write paths, not just on reads: merging, updating or removing against a cache miss would otherwise overwrite an evicted object with a partial one, drop an update as if the object were unknown, or leave a removed object in the store forever. A miss is only evidence of absence when the cache holds everything. Co-Authored-By: HAL 9000
Paginated listing hands the storage backend a token supplied by the caller, which the backend rejects if it is malformed. Reporting that as `PersistenceFailed` would be misleading, as nothing failed to persist, and would give a bindings user who round-trips a token through their own storage no way to tell a bad token from a broken store. Co-Authored-By: HAL 9000
Tests reach for the payment history in a great many places, all of them spelling out how it is retrieved. Route them through a helper trait instead, so that they state what they want and the retrieval lives in one place. Pure refactor: the helper currently just forwards to the existing listing API. Co-Authored-By: HAL 9000
Returning the entire payment history in one call requires holding it all in memory, which is exactly what a node with a long history cannot afford, and it gives an app no way to show recent payments without loading every old one. Return one page at a time instead, ordered from most recently created to least recently created, and drop the unpaginated variants. The ordering and the page tokens are the storage backend's own: we hand its opaque token straight back to it and never derive an order of ours. That keeps tokens valid across restarts and independent of what we happen to hold in memory, and it means a store that caches only a subset of its namespace can still list all of it, reading back whatever it does not hold. Listing deliberately neither waits for in-flight writes across its reads nor disturbs the cache. Blocking every writer for the duration of a round trip to a remote backend because something asked for a page would be a poor trade, and letting a sweep of the whole namespace count as use would evict the very entries a node works with most. Co-Authored-By: HAL 9000
The payment history grows for the lifetime of a node, and holding all of it in memory was the reason `Node::list_payments` had to hand back everything at once. Now that the store can read entries back on demand and listing goes through the storage backend, the payment store no longer has to. Keep the most recently used payments in memory and read the rest back as they are needed. At roughly 400 to 500 bytes per cached payment, 1000 of them bound this at well under a megabyte, regardless of how long a node has been running. Also stop reading the payment history at startup, which would otherwise mean fetching a node's entire history from the storage backend only to immediately drop all but the newest entries. The cache now starts empty and fills as payments are used. One consequence worth noting: a payment that fails to deserialize no longer fails the build, as we no longer read them all up front. It surfaces when that payment is accessed instead. Co-Authored-By: HAL 9000
Seeding a store meant reading its entire namespace, which for a bounded cache means fetching a node's whole payment history at startup only to drop all but the newest entries. The previous commit sidestepped that by not seeding the payment store at all, leaving it cold and no longer catching unreadable payment data at build time. Give the reader a bound instead, and seed the payment store with the newest 50 payments. That matches the storage backends' page size, so warming the cache costs a single page listing and one batch of reads, and the first page of `Node::list_payments` is answered without going to the store. Take the keys from the paginated listing rather than `KVStore::list`, which is documented to return them in arbitrary order and would therefore make "the newest 50" meaningless. Objects now come back in the store's own creation order, newest first, where before they came back in whatever order the reads happened to finish. Note the cache treats the objects it is seeded with as increasingly recently used, so a newest-first read has to be reversed before seeding, or the newest entries would be the first ones evicted. Co-Authored-By: HAL 9000
|
👋 Hi! This PR is now in draft status. |
benthecarman
left a comment
benthecarman
left a comment
There was a problem hiding this comment.
concept ack, this approach looks good to me
| from the configured storage backend, so a token stays valid across restarts. This replaces | ||
| the previous unpaginated `Node::list_payments`, and `Node::list_payments_with_filter` has | ||
| been removed; filter the returned pages instead. | ||
| - `Node::payment` now returns a `Result`, as retrieving a payment may fail. |
| /// Note also that a page may hold fewer objects than the backend's page size, because objects | ||
| /// removed between listing the keys and reading them are skipped. Iterate until | ||
| /// `next_page_token` is `None` rather than until a short page. | ||
| pub(crate) async fn list_page( |
There was a problem hiding this comment.
claude:
A single page can mix stale cache hits with fresh store reads — src/data_store.rs list_page
The read guard is dropped after the cache peek, before read_missing runs. Objects served from the cache reflect the state at peek time; objects read from the store reflect a possibly-later state. The doc comment covers concurrent creation and removal but not this. One sentence would close it.
joostjager
left a comment
joostjager
left a comment
There was a problem hiding this comment.
Concept ack, but I'd split this PR in two parts. First do the disk read, and only in a follow up add caching if we are absolutely sure we need it.
| /// [`DataStorePage::next_page_token`] to continue from where the previous call left off. | ||
| /// | ||
| /// The ordering and the tokens are the storage backend's own: we hand its opaque token back to | ||
| /// it unchanged and never derive an order of our own. That keeps tokens valid across restarts |
There was a problem hiding this comment.
Delegating ordering to the backend sounds risky. I also believe we already had an issue with list_all where the order wasn't maintained across migrations. Wondering what the reason is we don't have an ldk-node level creation id that is used for ordering?
| /// | ||
| /// Note this deliberately does not hold the mutation lock across its reads: a listing must not | ||
| /// block every writer for the duration of a round trip to a remote backend. Objects created or | ||
| /// removed while paginating may or may not be observed. |
There was a problem hiding this comment.
Is this a premature optimization? Especially with pagination and the upcoming more efficient postgres list with values, this may not be necessary. If we can avoid "may or may not be observed" I think that would be good.
| // to immediately drop all but the most recent entries. | ||
| let payment_store = Arc::new(PaymentStore::new( | ||
| Vec::new(), | ||
| KeepLeastRecentlyUsed::new(PAYMENT_CACHE_CAPACITY), |
There was a problem hiding this comment.
Does this PR already need to include caching of payments that are no longer pending, or is it something to consider later when we see that disk reads are unacceptable?
| pub(crate) async fn list_page( | ||
| &self, page_token: Option<PageToken>, | ||
| ) -> Result<DataStorePage<SO>, Error> { | ||
| let response = PaginatedKVStore::list_paginated( |
There was a problem hiding this comment.
AI: The bounded cache does not make pagination scalable for the default filesystem backend. At the pinned rust-lightning revision, FilesystemStoreV2::list_paginated_impl collects every key in the namespace and sorts the complete vector on every page request: https://github.com/lightningdevkit/rust-lightning/blob/9174965af9437196c527a9aa0df36bbcf050c8bb/lightning-persister/src/fs_store/v2.rs#L147-L216
Consequently, one page still requires O(N) temporary key memory and O(N log N) work, while traversing the complete history repeats that work for every page. Could this be fixed upstream, or should the scalability claim be explicitly limited for the filesystem backend?
There was a problem hiding this comment.
Me says: drop filesystem store in ldk-node
| /// Pass `None` to start at the most recently created payment, and the | ||
| /// [`PaymentDetailsPage::next_page_token`] returned by the previous call to continue from | ||
| /// where it left off. Ordering and pagination are backed by the configured storage backend, so | ||
| /// a token stays valid across restarts of the node. |
There was a problem hiding this comment.
AI: This guarantee is stronger than the PaginatedKVStore contract. That trait only requires tokens to remain valid across calls for a reasonable timeframe; it does not require them to survive backend reconstruction or restart. build_with_store accepts arbitrary implementations, so forwarding an opaque token unchanged is not enough to guarantee this.
Could we either make restart-stable tokens an explicit custom-store requirement or qualify this statement for backends that provide that guarantee?
| e | ||
| ); | ||
| set.abort_all(); | ||
| return Err(Error::PersistenceFailed); |
There was a problem hiding this comment.
AI: Because startup now validates only the newest 50 payments, an unreadable older payment can first surface here during pagination. Returning an error discards response.next_page_token, leaving the caller unable to identify or skip the bad entry and therefore unable to reach any older pages.
Should pagination expose the continuation token alongside entry-level failures, provide another recovery mechanism, or deliberately retain fail-at-startup behavior?
Thanks, but I think the caching is an important part here, e.g. for VSS-based mobile wallets (but also server) not immediately hitting huge latency spikes when listing recent payments. I however did refrain from further optimizations (e.g. lazy look-ahead caching etc). |
For server deployments, it seems unlikely to me that reading one page from disk is expensive enough to warrant caching. For VSS, the individual object reads are already performed concurrently. Does that still cause unacceptable latency in practice? Either way, I think this is easier to review and reason about as two changes: first land disk-backed pagination, then add caching separately. |
I mean, if you add caching later, you'll have to re-review the same code twice as the whole design is now rearranged to make that fit? |
You'd have to re-arrange the commit stack, but still I think there is enough that doesn't need to be reviewed twice. If adding caching later requires a whole rewrite, I'd think there is something not quite right in the design. Generally caching should be something that can be added incrementally. If reviewers are fine with a PR of this size, then it matters less. Although even then I still think it is safer to take small steps. |
4 participants
Alternative/prefactor to #959.
Fixes #998.
Draft for now until concept ack.