docs: Expand MemoryPool docs with related structs
#16289
Conversation
| /// ╚═══════════════════════════════════════════════════╝ | ||
| /// | ||
| /// For example, there are two parallel partitions of an operator X: each partition | ||
| /// corresponds to a `MemoryConsumer` in the above diagram. Inside operator X there are |
There was a problem hiding this comment.
Thanks @2010YOUY01
Can we clarify slightly more on Inside operator X there are several MemoryReservations for different internal data structures. I read it twice, not sure what does that mean 🤔 is it MemoryReservation per data structure?
There was a problem hiding this comment.
Thanks.
Yes, and I updated it to be more explicit. I suspect now most MemoryConsumers only use 1 reservation for aggregated memory accounting, but by design a consumer can use multiple reservations for finer-grained stat report and also make implementation cleaner.
|
@ding-young Could you take a look and point out anything that doesn't make sense to help refine the doc? |
| /// - `MemoryReservation` follows RAII principles - to release a reservation, | ||
| /// simply drop the corresponding `MemoryReservation` object |
There was a problem hiding this comment.
This comment is good for explaining that MemoryReservation uses RAII-style dropping, but it might be clearer if we also mention that it is automatically unregistered from the memory pool via SharedRegistration. That way, it’s easier to understand the role of SharedRegistration.
There was a problem hiding this comment.
This is a really nice improvement - thank you @2010YOUY01 @ding-young and @comphead
4 participants
Which issue does this PR close?
Rationale for this change
Add more docs to the structures related to
MemoryPoolWhat changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?