Minor: Improve memory helper trait documentation #9025
Conversation
| /// | ||
| /// Note this calculation is not recursive, and does not include any heap | ||
| /// allocations contained within the Vec's elements. Does not include the | ||
| /// size of `self` |
There was a problem hiding this comment.
when reviewing this code, I note that datafusion has a custom growth strategy (2x) which is wasteful with large allocations -- I think we should perhaps go back to using the built in rust growth strategy. I'll file a PR / ticket with this idea later
| // under the License. | ||
|
|
||
| //! Manages all available memory during query execution | ||
| //! [`MemoryPool`] for memory management during query execution, [`proxy]` for |
There was a problem hiding this comment.
what is proxy? I found the proxy.rs, is it correct?
| /// | ||
| /// For help with allocation accounting, see the [proxy] module. | ||
| /// | ||
| /// [proxy]: crate::memory_pool::proxy |
There was a problem hiding this comment.
oh I see what is proxy now
There was a problem hiding this comment.
Yeah, I also find proxy non intuitive -- the extension traits aren't really proxies for anything. Maybe we should move them to crate::memory_pool::extensions or crate::memory_pool::utils 🤔
I suppose at least this PR documents them which is an improvement
There was a problem hiding this comment.
IIRC we once had an actual proxy object that could be used to represent the memory for another more complex data structure, but this design was since changed. So only the helper traits remain, which I think should be under utils or extensions. So feel free to move them.
| /// | ||
| /// For help with allocation accounting, see the [proxy] module. | ||
| /// | ||
| /// [proxy]: crate::memory_pool::proxy |
There was a problem hiding this comment.
IIRC we once had an actual proxy object that could be used to represent the memory for another more complex data structure, but this design was since changed. So only the helper traits remain, which I think should be under utils or extensions. So feel free to move them.
| /// [Insert](RawTable::insert) new element into table and increase | ||
| /// `accounting` by any newly allocated bytes. | ||
| /// | ||
| /// Returns the bucket where the element was inserted. |
There was a problem hiding this comment.
TBH this design is "good enough", but we should probably add some examples at some point (doesn't have to be in this PR though). Same is actually true for the other trait.
alamb
force-pushed
the
alamb/memory_docs
branch
from
d633ff5 to
c19e078
Compare
3 participants
Which issue does this PR close?
Part of #7013
Rationale for this change
I forgot about these traits and while rediscovering them I felt I could try and make them more discoverable via some documentation.
What changes are included in this PR?
Add / improve documentation
Are these changes tested?
Doc tests (no code changes)
Are there any user-facing changes?