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 4 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 is to have:

  • 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 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 the only place to hold one shared_ptr<dynamic_routing_table_impl> and the dynamic_routing_table_impl is the previous implementation. therefore we only have one shared_ptr to manage one big object.

Will post this PR 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
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