Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat: replace worker connection's mutex with
tbb::concurrent_hash_map
#2461base: unstable
Are you sure you want to change the base?
feat: replace worker connection's mutex with
tbb::concurrent_hash_map
#2461Changes from 9 commits
3d495ef
f6192d4
4416da7
fe89194
1104241
effbcf9
c0d9efb
3c4205a
c17ae9a
9f1b707
8385b41
14a0deb
f4bbded
797c9d9
4eff1e8
2027b03
e3d992d
49d6792
8011d59
20678a5
b17dcec
0f3f8bd
afccc88
9714558
1665dab
65648bf
261812d
0b64679
ee88850
1484808
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Is
parallel_reduce
necessary here?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.
Hi @PragmaTwice ,
I use a reduce action before iteration because the
parallel_for
will acquire a range rather than one item.So, I want to move the time costly action
Close()
outside of theparallel_for
.But since this function is inside the dtor, if the ownership is managed correctly, it should be the only function uses internal state of the
Worker
object.I think the
parallel_reduce
can be replaced byparallel_for
or even removed.I will refactor it and run tests before pushing to current PR.
Best Regards,
Edward
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 mean maybe a single-thread for is enough here? didn't have a deep look yet cc @mapleFU
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.
Hi @PragmaTwice ,
I have removed the
parallel_reduce
from dtor and all tests are passed in my repo's GitHub Actions.I think in dtor the concurrency protection of internal state may be redundant since all references should be released except the dtor itself.
Best Regards,
Edward
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.
Yeah parallel task also has a dop (degree of parallel), and I guess default of
parallel_for
would be high and be good at writing cpu-bound tasks like OpenMPI think maybe lock is prefered here
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.
Hi @mapleFU ,
Sorry for delay reply.
I'd like to use this way on all accessors to mitigate potential inconsistency in
monitor_conns_
andconns_
.Best Regards,
Edward
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.
IMO parallel_for looks like some cpu-bound operations, and would it occupies more threads than expected here?
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.
Hi @mapleFU ,
Thanks for your review suggestion.
I read the tbb's official document, and find the schedule uses all cores on default.
Maybe we should set a limitation for tbb schedule.
In fact, I'm not sure do we really need tbb hashmap to replace current mutex now. 🤔
Best Regards,
Edward
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 it's worth doing if we don't traverse all
conns_
frequently.