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

Replace boost::container::flat_map with absl::btree_map in P4Tools. #4768

Merged
merged 4 commits into from
Aug 1, 2024

Conversation

fruffy
Copy link
Collaborator

@fruffy fruffy commented Jul 1, 2024

Yet another variant of #4667 and #4713 but a bit more conservative.

btree_map is a memory-efficient variant of std::map which does not require hashing. We can use it to replace boost::container::flat_map in P4Tools.

Reason why I am splitting this PR out:

#4667 introduces some hash functions which are still too inefficient and needs improvement.

#4713 introduces a new data structure (flat_map) which should be reviewed separately.

The other two PRs will be rebased after this one is in.

@fruffy fruffy added the p4tools Topics related to the P4Tools back end label Jul 1, 2024
@fruffy fruffy force-pushed the fruffy/absl_map branch 2 times, most recently from 013c70f to 406a52b Compare July 1, 2024 16:24
@asl
Copy link
Contributor

asl commented Jul 1, 2024

Note that btree_map has different iterator invalidation rules as compared to std::map. Not sure if it matters, but worth double checking.

@fruffy
Copy link
Collaborator Author

fruffy commented Jul 1, 2024

Note that btree_map has different iterator invalidation rules as compared to std::map. Not sure if it matters, but worth double checking.

We do not remove elements from these maps (although we should...) and I believe we only iterate for debugging. Pointer stability could be a problem but can't currently think of a case where.

@fruffy fruffy force-pushed the fruffy/absl_map branch 2 times, most recently from 21bf9fa to 9bfab6d Compare July 1, 2024 21:15
@fruffy fruffy requested a review from vlstill July 1, 2024 22:44
@fruffy fruffy requested review from vlstill and removed request for vlstill July 25, 2024 17:42
Copy link
Contributor

@vlstill vlstill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Conceptually looks good to me, just some code quality suggestions.

backends/p4tools/p4tools.def Outdated Show resolved Hide resolved
backends/p4tools/p4tools.def Outdated Show resolved Hide resolved
backends/p4tools/p4tools.def Outdated Show resolved Hide resolved
backends/p4tools/p4tools.def Outdated Show resolved Hide resolved
backends/p4tools/p4tools.def Outdated Show resolved Hide resolved
@fruffy
Copy link
Collaborator Author

fruffy commented Jul 30, 2024

I noticed solver.h depends on IR::SymbolicVariable, which is only defined in the p4tools back end. Either we move the definitions of these files to the top-level def files, or move the solver class back to the p4tools code.

@fruffy fruffy enabled auto-merge August 1, 2024 10:35
@fruffy fruffy added this pull request to the merge queue Aug 1, 2024
Merged via the queue into main with commit a7d6e35 Aug 1, 2024
17 checks passed
@fruffy fruffy deleted the fruffy/absl_map branch August 1, 2024 11:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p4tools Topics related to the P4Tools back end
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants