Extended Utility Codel#475
Conversation
Codecov Report
@@ Coverage Diff @@
## master #475 +/- ##
==========================================
- Coverage 65.33% 65.26% -0.08%
==========================================
Files 198 198
Lines 12261 12311 +50
==========================================
+ Hits 8011 8035 +24
- Misses 4250 4276 +26
Continue to review full report at Codecov.
|
apanda
left a comment
apanda
left a comment
There was a problem hiding this comment.
I left some code comments, but I must admit I am entirely unsure about the context of this change. Do you mind adding something to the PR or to one of the commit messages describing what you are trying to achieve with this change?
| } | ||
|
|
||
| size_t Capacity() override { return queue_.max_size(); } | ||
| size_t Capacity() override { |
There was a problem hiding this comment.
Why is Codel's capacity different from the underlying queue capacity. We have already allocated memory (based on the Resize method below). It would be good to at least add a comment explaining this choice, since the current code is surprising.
There was a problem hiding this comment.
I switched the underlying queue to deque which has a max size of the memory available to it according to the spec and I didn't see any constructor parameter to limit the deque size. In the use case of having many codels in drr handling a burst, it would be better to restrict queue size rather than have issues with memory although the max_entries can be specified as 0 to default to the memory size. I will add a comment but is allowing restriction not a good idea?
There was a problem hiding this comment.
That is OK, just add a comment with the reason you gave above.
There was a problem hiding this comment.
The restriction is fine -- I think it needs to be documented. I am not super sure how one picks max_size_ -- it seems to depend on the workload, but c'est la vie.
| if (new_capacity <= Size()) { | ||
| return -1; | ||
| } else if (new_capacity >= queue_.max_size()) { | ||
| return -1; |
There was a problem hiding this comment.
Why does this method fail here, rather than calling queue_.resize?
There was a problem hiding this comment.
I changed the underlying queue to be deque so the max_size is the max number of entries that the queue can have given system limitations.
There was a problem hiding this comment.
Just document it -- it looks strange to have a resize method which mostly does nothing.
|
Just wanted to follow up -- Josh, do you think you're pretty much done making the changes Panda asked about? If so, we can merge this in the next day or two. |
|
ya, I can finish it up tonight and push. I will ping you when I do. |
|
Merged, thanks! |
3 participants
Adds Factory, Generator and Resize methods(as well as a few minor fixups) to Utility Codel in order to allow integration into DRR.