-
Notifications
You must be signed in to change notification settings - Fork 797
[XPTI][INFRA] Fixes race issues with Emhash under contention #19600
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
Conversation
tovinkere
commented
Jul 25, 2025
- Under severe concurrent access, rehash in Emhash was being called during insert and this is documented as a problem in Emhash Readme. Rehash for the Emhash containers will not have to be called now as enough space for handling streams is reserved.
- xptiCheckTraceEnabled() is now thread-safe
- Emhash containers now have sizes reserved to accommodate the entries and all second level hash maps that were Emhash in the past have been replaced with parallel hashmap.
- xptiCheckTraceEnabled() is now thread-safe - Emash can have issues when rehash is called during insertion. So, all Emhash containers now have sizes reserved to accommodate the entries and all second level hash maps use parallel hashmap. - Under severe concurrent access, rehash in Emhash was being called during insert. Rehash for the Emhash containers will not have to be called now. Signed-off-by: Vasanth Tovinkere <[email protected]>
Signed-off-by: Vasanth Tovinkere <[email protected]>
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.
Looks reasonable, but I'm not familiar with the code. @KseniyaTikhomirova , FYI, maybe for a post-commit review?
xptifw/src/xpti_trace_framework.cpp
Outdated
@@ -1562,12 +1565,14 @@ class Notifications { | |||
} | |||
} | |||
#endif | |||
{ | |||
std::unique_lock<std::shared_mutex> Lock(MFlagsLock); | |||
auto &TraceFlags = MStreamFlags[StreamID]; // Get the trace flags for the |
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.
The comment here ends abruptly.
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.
Updated it!
Signed-off-by: Vasanth Tovinkere <[email protected]>
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.
lgtm from dependency pov, parallel hashmap is already a dep so this isnt changing anything. not qualified to review for correctness
Signed-off-by: Vasanth Tovinkere <[email protected]>
Fail seems to be covered by #15207. |
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.
LGTM