Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[http_server] Simplify DRT node and table #132

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

cong1920
Copy link
Contributor

@cong1920 cong1920 commented Dec 31, 2024

When I see the std::vecotr<std::shared_ptr<drt_node>> and std::vector<std::shared_ptr<std::string>> as the drt_node and dynamic_routing_table's members respectively, I cannot help wondering why the shared_ptr<T> are needed here. Later I realized the instance of the DRT table is allowed to copy therefore we need to keep its objects being shared across all instances. Somehow I still think it is a waste to have shared_ptr<T> manage each small object.

This PR includes

  • unordered_set<string> to keep the strings other than std::vector<std::shared_ptr<std::string>>. unordered_set<string> is more straightforward and able to get rid of redundant duplicates automatically.
  • drt_node_pool wrapping the vector<unique_ptr<drt_node>> and its instance is shared across all instances of drt_node to manage the lifetime of new drt_node instances.
  • dynamic_routing_table is to hold one shared_ptr<dynamic_routing_table_data> to let one shared_ptr manage one big object other than multiple shared_ptrs manage multple small objects.

This PR is posted as draft to let @matt-42 have a review first. Will squash to one single commit before final merge.

@cong1920 cong1920 marked this pull request as ready for review December 31, 2024 08:06
@cong1920 cong1920 marked this pull request as draft December 31, 2024 08:07
@cong1920 cong1920 force-pushed the simplify_drt_node branch from 1966842 to 7a5aa97 Compare March 16, 2025 07:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant