-
Notifications
You must be signed in to change notification settings - Fork 444
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
Use abseil maps even more #4473
Conversation
I think some other benchmarks are also required. Unfortunately, we do not have much that could be ported from downstream to "stock" p4c. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went over the C++ changes. This definitely goes in the right direction, thank you. But I have some points that I believe needs to be addressed. You seem to be using Util::Hash
only sometimes, and it is not clear to me if this is deliberate, or if it is an omission. Also, I suggest we make the get
availble for the abseil maps to simplify both migration and the code itself.
@@ -17,9 +17,9 @@ limitations under the License. | |||
#ifndef COMMON_RESOLVEREFERENCES_RESOLVEREFERENCES_H_ | |||
#define COMMON_RESOLVEREFERENCES_RESOLVEREFERENCES_H_ | |||
|
|||
#include "absl/container/flat_hash_map.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, can we make absl a bracketed system include to be consistent?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think someone asked somewhere exactly the opposite for fetched dependencies :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really? I believe all our fetched dependencies (protobuf, gtest, inja, z3) are included using bracketed includes. Can you point me to the discussion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to find. But I'm happy to change to angled brackets. This makes much more sense to me :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, apparently abseil expects to use quoted includes. And bazel build even enforces this (the path is exported as -iquoted
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ugh, seems to be an opinionated maintainer. This might cause issues with Werror and clang-tidy. Not a big deal but annoying.
We can just patch this, I can get this done in a separate PR.
5c38f07
to
83fff4d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving from my side.
This is PR on top of #4459 to show how we can improve the things even more with better data structures.
Essentially, this uses more efficient data structures on some hot code paths:
Here are results:
gtestp4c-abseil --gtest_filter=P4CParserUnroll.switch_20160512
gtestp4c --gtest_filter=P4CParserUnroll.switch_20160512
gtestp4c-abseil
is #4459 (so yes 78% faster a total compared tomain
).