Reduce rtp port name search complexity #4272
Conversation
|
Build Passed! |
| @@ -79,7 +79,7 @@ graph_type(std::shared_ptr<xrt_core::device> dev, const uuid_t, const std::strin | |||
| rtp.selector_row += 1; | |||
| rtp.ping_row += 1; | |||
| rtp.pong_row += 1; | |||
| rtps.emplace_back(std::move(rtp)); | |||
| rtps.insert(std::make_pair(rtp.name, std::move(rtp))); | |||
There was a problem hiding this comment.
| rtps.insert(std::make_pair(rtp.name, std::move(rtp))); | |
| rtps.emplace(rtp.name, std::move(rtp)); |
There was a problem hiding this comment.
Same code fewer key strokes. But btw is either construct really safe? Is there any chance rtp.name could have been moved before it is copied, for sure order of evaluation is not defined here. There is enough doubt in my mind that I think a copy of the string should have been done outside insert or emplace, then another std::move on the copied string.
Better, a std::set should have been used with proper compare defined.
There was a problem hiding this comment.
Same code fewer key strokes. But btw is either construct really safe? Is there any chance rtp.name could have been moved before it is copied, for sure order of evaluation is not defined here. There is enough doubt in my mind that I think a copy of the string should have been done outside insert or emplace, then another std::move on the copied string.
Ouch, you are right!
Better, a std::set should have been used with proper compare defined.
Brilliant.
| throw xrt_core::error(-EINVAL, "Can't update graph '" + name + "': RTP port '" + port + "' not found"); | ||
| auto& rtp = it->second; |
| @@ -25,6 +25,7 @@ | |||
| #include "core/common/device.h" | |||
| #include <string> | |||
| #include <vector> | |||
| #include <unordered_map> | |||
There was a problem hiding this comment.
Are you using an unordered_sort on the headers here? ;-)
There was a problem hiding this comment.
It would have been nicer with #include <unordered_map> before #include <vector>
There was a problem hiding this comment.
Point taken. Will sort it in my next PR.
There was a problem hiding this comment.
Never thought about sorting #includes but actually that's a good idea. I will follow that.
Do make sure all XRT local includes are grouped before system includes though.
@keryell Took your second comment before I understood your first :-)
There was a problem hiding this comment.
Sorry for using too much convoluted humor too often...
Sorting headers is common in a lot of projects and coding styles to have a quicker idea about what are the headers in use.
|
Build Passed! |
This PR addressed the following issues
1.
We are currently using vector to store RTP port and when a port is updated, we use
auto rtp = std::find_if(rtps.begin(), rtps.end(), auto it = rtps.find(port);
[port](rtp_type it) { return it.name.compare(port) == 0; });
to match a given port name.
This is time consuming when there are as much as 400 rtp ports used. In this PR, we changed vector to unordered_map to store rtp ports so that we can theoretically reduce the search complexity to O(1)
For interface xrtGraphWait and xrtGraphWaitDone, we return successfully if the graph is already in suspend state or stop state.