Address allocator documentation #12
Conversation
AlexandruCihodaru
force-pushed
the
address_allocator_documentation
branch
from
e7f8d7d to
5933223
Compare
|
I have pushed a reference implementation here. Please take a look. |
Signed-off-by: AlexandruCihodaru <cihodar@amazon.com> Suggested-by: Liu Jiang <gerry@linux.alibaba.com> Suggested-by: Chao Wu <chaowu@linux.alibaba.com>
Signed-off-by: AlexandruCihodaru <cihodar@amazon.com> Suggested-by: Liu Jiang <gerry@linux.alibaba.com> Suggested-by: Chao Wu <chaowu@linux.alibaba.com>
AlexandruCihodaru
force-pushed
the
address_allocator_documentation
branch
from
5933223 to
a666a42
Compare
| ```rust | ||
| impl AddressAllocator { | ||
| /// Creates a new AddressAllocator object with an empty IntervalTree | ||
| pub fn new(base: u64, size: u64) -> std::result::Result<Self, Error> { } |
There was a problem hiding this comment.
The struct has the end field, while the constructor has size. Is this by design, i.e. does base and size in the constructor get translated to base and end in the struct?
There was a problem hiding this comment.
Also, looking at the allocate() method below, the return type should probably be Result<Self>.
There was a problem hiding this comment.
The allocate method return an interval [a, b] that correspond to the constraint, I do not believe it should return Result<Self>
There was a problem hiding this comment.
I see how my comment was confusing, sorry :( What I meant is that new() should return Result<Self> similar to how allocate() returns Result<Range>. So, Result<Self> instead of std::result::Result<Self, Error>.
| base: u64, | ||
| // End of the address space that we want to manage. | ||
| end: u64, | ||
| // Allocation engine, the allocation logic that is interfaced |
There was a problem hiding this comment.
"the ... logic that is interfaced" sounds a bit weird. Maybe just "The Interval Tree allocation engine"?
| _address: Option<u64>, | ||
| size: u64, | ||
| align_size: Option<u64>, | ||
| alloc_policy: AllocPolicy, |
There was a problem hiding this comment.
policy: AllocPolicy is probably sufficient, from the method's name it's pretty clear that it's about allocation.
| pub fn min(mut self, min: u64) -> Self { } | ||
|
|
||
| /// Set the max constraint. | ||
| pub fn max(mut self, max: u64) -> Self { } |
There was a problem hiding this comment.
Shouldn't min and max be returning Result<Self>, for example when max is already set, and then mix is set with a larger value?
Also, is it possible for align() to fail?
| pub(crate) struct InnerNode { | ||
| /// Interval handled by this node. | ||
| pub(crate) key: Range, | ||
| /// Optional contained data, None if the node is free. |
There was a problem hiding this comment.
Probably a leftover from the original implementation when it was Option. Now it's NodeState enum, so no "Optional" and "None".
|
|
||
| ```rust | ||
| pub struct IntervalTree { | ||
| pub(crate) root: Option<InnerNode>, |
There was a problem hiding this comment.
Do we need to have the pub(crate) visibility here? Is it possible to completely hide away this implementation detail?
| Allocated, | ||
| } | ||
|
|
||
| /// Insert the ket into the interval tree, returns Error if intersects with existing nodes. |
There was a problem hiding this comment.
The methods should probably be wrapped into the impl IntervalTree block.
| pub fn insert(&mut self, key: Range, node_state: NodeState) -> Result<()> { } | ||
|
|
||
| /// Update an existing entry and return the old value. | ||
| pub(crate) fn update(&mut self, key: &Range, node_state: NodeState) -> Result<()> { } |
There was a problem hiding this comment.
Why pub(crate) instead of pub like the rest of the methods?
| pub fn is_empty(&self) -> bool { } | ||
|
|
||
| /// Get the data item associated with the key, or return None if no match found. | ||
| pub fn get(&self, key: &Range) -> Option<NodeState> { } |
There was a problem hiding this comment.
Just to clarify, NodeState is the actual data item, right?
| let interval_tree = IntervalTree::new(); | ||
| let mut address_allocator = AddressAllocator { | ||
| base: base, | ||
| end: end.unwrap(), |
There was a problem hiding this comment.
Where does end come from and why .unwrap()? Same question for the allocate() below.
| let constraint = Constraint::new(size).align(alignment).policy(alloc_policy); | ||
| // some other code here | ||
| if logical_condition { | ||
| let key = self.interval_tree.find_candidate(&constaint); |
There was a problem hiding this comment.
The find_candidate() method was not in the list of the methods above.
|
Maybe it's just me, but I do have a strong feeling that we are not talking documentation here, but discussing the interfaces instead. It's also kinda awkward to review when the interfaces are part of the documentation. Maybe it would be better to have a very short docs with high-level concepts expressed with words, and the interfaces be part of the actual code. |
The implementation of AddressAllocator with IntervalTree backend will be added in multiple subsequent PRs.