feat: lazy initialization for log_memory_store to avoid unnecessary log-memory charges #7982
Conversation
maksymar
changed the title
| /// Clears the canister log records. | ||
| pub fn clear(&mut self) { | ||
| // Write an invalid ring buffer to the page map to avoid unnecessary log-memory charges. | ||
| self.page_map = RingBuffer::invalid(self.page_map.clone()).to_page_map(); |
There was a problem hiding this comment.
| self.page_map = RingBuffer::invalid(self.page_map.clone()).to_page_map(); | |
| self.page_map = PageMap::new(allocator) |
How about clearing it like that? That would reset the entire PageMap to zeroes. The way you do it the old log is still in the state/checkpoint, as you simply marked it as invalid. Replacing the PageMap will discard the old data. Your invalid marker would then simply be zero bytes.
rs/replicated_state/src/canister_state/system_state/log_memory_store/mod.rs
Outdated
Show resolved
Hide resolved
| let mut ring_buffer = self.load_ring_buffer(); | ||
| f(&mut ring_buffer); | ||
| let mut ring_buffer = self.load_ring_buffer().unwrap_or(RingBuffer::new( | ||
| self.page_map.clone(), |
There was a problem hiding this comment.
Why is this cloning the old page_map, instead of creating a new, empty one? I thought this is the case where the ring buffer does not exist.
There was a problem hiding this comment.
In append_delta_log when it's called the first time after creation we don't have access to the allocator, but we do have access to the empty page_map that was created in new_inner or in clear. So it's actually initializing this empty page map with the ring buffer for the first time.
| /// Clears the canister log records. | ||
| pub fn clear(&mut self, fd_factory: Arc<dyn PageAllocatorFileDescriptor>) { | ||
| // This creates a new empty page map with invalid ring buffer header. | ||
| self.page_map = PageMap::new(fd_factory); |
There was a problem hiding this comment.
More an interface question on future PRs really, but what is the plan regarding the log_memory_limit when you uninstall code?
When the code uninstalls because we are out of cycles, presumably we want to clear the log, and also want to set the limit back down to DATA_CAPACITY_MIN so that the memory is freed again.
On the other hand, if a user uninstalls code manually, do they even want the log to be cleared? And if so, should the log limit change in that case? Is this maybe something the user can choose with a new field for uninstall_code?
There was a problem hiding this comment.
when canister is uninstalls then log is deleted. but log_memory_limit is more like a user setting (similar to wasm memory limit, etc) and I don't think this should be dropped to default. when the user installs the wasm, then it'll start working with the previous log memory limit value.
| // | ||
| // Recreate a ring buffer with the new capacity and restore records. | ||
| let mut new = RingBuffer::new( | ||
| self.page_map.clone(), |
There was a problem hiding this comment.
Same question, should this be an empty PageMap instead of a clone?
There was a problem hiding this comment.
Since we don't store the allocator it was easier just to clone an existing page map. Especially considering that this would be the first candidate for future optimizations.
| self.page_map.clone(), | ||
| MemorySize::new(new_log_memory_limit as u64), | ||
| ); | ||
| new.append_log(old.all_records()); |
There was a problem hiding this comment.
Note for a future PR that calls this function: This is potentially expensive and needs to be reflected correctly in cycles cost/instruction counters/heap deltas.
There was a problem hiding this comment.
Yes, noted. that's the first candidate for optimization.
| // Only resize on an existing ring buffer. | ||
| if let Some(old) = self.load_ring_buffer() { | ||
| // Only resize when the capacity actually changes. | ||
| if old.byte_capacity() != new_log_memory_limit { |
There was a problem hiding this comment.
Do we need to do this expensive clone even if the limit increases? PageMaps can already grow today.
There was a problem hiding this comment.
Yes we do, because it's a ring buffer which is always almost full and 99% of a time is wrapped, so you can't just grow memory and continue saving records. you need to re-wrap the whole buffer at the new capacity. that's why I read all the records in memory, re-create ring buffer with a new capacity and write records back.
2 participants
This PR introduces a lazy initialization for
log_memory_storeto avoid charging for dirty page_map.The canister can be created without wasm module or be uninstalled (when all its memories are cleaned). In those states the canister should require no charge, therefore should use zero memory (especially in page_map's). Previously we were storing
log_memory_limitimplicitly in ring-buffer header in the first page of page_map, which dirtied the page and required memory usage charging even if canister had no wasm module or was uninstalled.In this PR we store
log_memory_limitoutside of page_map keeping it clean and only initizlizing it when appending logs. In the follow up PR this value will be preserved in canister state bits protobuf.