Conversation
Signed-off-by: wbpcode/wangbaiping <wbphub@gmail.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a well-implemented intrusive linked list (IntrusiveList and IntrusiveListNode) as a more efficient alternative to std::list with LinkedObject. The implementation is solid, with clear ownership semantics and good test coverage. My feedback focuses on refining the API to be more idiomatic and robust.
There was a problem hiding this comment.
Pull request overview
Adds a new intrusive, owning doubly-linked list implementation alongside the existing LinkedObject helpers, aiming to avoid per-element std::list node allocations and improve locality for linked heap objects.
Changes:
- Introduces
IntrusiveListNode<T>(CRTP) andIntrusiveList<T>intosource/common/common/linked_object.h. - Adds unit tests covering insert/remove and cross-list moves for the new intrusive list.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| source/common/common/linked_object.h | Adds the new intrusive list/node container implementation. |
| test/common/common/linked_object_test.cc | Adds unit tests for intrusive list insertion/removal and moving between lists. |
You can also share your feedback on Copilot code review. Take the survey.
Signed-off-by: wbpcode/wangbaiping <wbphub@gmail.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a well-designed and thoroughly documented intrusive list implementation, IntrusiveList and IntrusiveListNode. This is a valuable addition that promises performance benefits by eliminating extra memory allocations compared to std::list<unique_ptr<T>>. The implementation is robust, featuring clear ownership semantics and compile-time safety checks. The unit tests are comprehensive, covering a wide range of scenarios. My feedback includes a couple of minor suggestions to improve the clarity of compile-time error messages.
There was a problem hiding this comment.
Pull request overview
Adds a new owning intrusive doubly-linked list implementation to source/common/common/linked_object.h, intended to avoid the per-element std::list node allocation currently incurred by std::list<std::unique_ptr<T>> patterns, and extends the existing unit tests to cover the new container behavior.
Changes:
- Introduce
IntrusiveListNode<T>(CRTP) andIntrusiveList<T>(owning container) inlinked_object.h. - Add unit tests validating push front/back, removal in different positions, and moving elements between lists.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
source/common/common/linked_object.h |
Adds IntrusiveListNode + IntrusiveList implementation and documentation. |
test/common/common/linked_object_test.cc |
Adds unit tests covering the new intrusive list APIs. |
You can also share your feedback on Copilot code review. Take the survey.
Signed-off-by: wbpcode/wangbaiping <wbphub@gmail.com>
Signed-off-by: wbpcode/wangbaiping <wbphub@gmail.com>
|
retest |
|
/retest |
|
Do you by any chance have benchmarks for this? |
|
yavlasov cannot be assigned to this issue. |
|
🙀 Error while processing event: |
|
/assign @yanavlasov |
Sure, Paul. Here is a result of the benchmark: From the result, we can found the new intrusive list have similar iteration performance. And have ~50% improvement for insertion and removal. Sure this is before we used very simple heap object for test. If more complex structure are used, I guess there won't be such huge improvement. |
Signed-off-by: wbpcode/wangbaiping <wbphub@gmail.com>
|
/retest |
Commit Message: common: add intrusive list
Additional Description:
In the current implementation, the
LinkedObjectand std::list is widely used to store the linked heap object. But we will find although we use theLinkedObjectto invade the heap object, but because std::list is used, we still always need to allocated a heap list node for every stored heap object.This PR added an implementation of intrusive linked list which could be used to replace the std::list +
LinkedObjectand get better efficiency and eliminate unnecessary heap allocations.Risk Level: low. Only new container implementation and does not change any existing feature.
Testing: unit.
Docs Changes: n/a.
Release Notes: n/a.
Platform Specific Features: n/a.