-
Notifications
You must be signed in to change notification settings - Fork 697
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
Add boolean parameter to clear memory for edge labels from thor #2789
Add boolean parameter to clear memory for edge labels from thor #2789
Conversation
a0cd449
to
7390d53
Compare
c49d5a0
to
45e0f0a
Compare
45e0f0a
to
8c619dd
Compare
looks like the tests are running into what i expected: #2808 I would suggest we reserve max_reach in the reach constructor by passing it to the dijkstra constructor. |
@keygang i cleaned up that issue with the resevering too much by default in the reach check on this branch. with any luck the performacne will be unimpacted and we can merge after checking that |
bad news! performance is absolutely aweful compared to master... im not quite sure where the hell its going though... @genadz any thoughts? |
src/thor/astar_bss.cc
Outdated
if (edgelabels_.size() > max_reserved_labels_count_) { | ||
edgelabels_.resize(max_reserved_labels_count_); | ||
edgelabels_.shrink_to_fit(); | ||
} | ||
edgelabels_.resize(clear_reserved_memory_ ? 0 : max_reserved_labels_count_); | ||
edgelabels_.shrink_to_fit(); |
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.
my suggestion was wrong here. when you dont have clear_reserved_memory_ set to true and the edges labels container is smaller than max_reserved_labels_count_ then you dont actually have to resize and shrink_to_fit.\
auto reservation = clear_reserved_memory_ ? 0 : max_reserved_labels_count_; // do this in the constructor and save it as a member variable?
if (edgelabels_forward_.size() > reservation) {
edgelabels_forward_.resize(reservation);
edgelabels_forward_.shrink_to_fit();
}
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 tried fixing this in bidirectional a* and dijkstras but its still slow... where else are we running into this, it clearly has to be this bit of the code that is causign the performane bottleneck. i'll look at it with fresh eyes in a day or so
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.
@kevinkreiser
you haven't watched this PR again?
@keygang id love to merge this but there is something wrong with the performance. its incredibly slow. we need to concentrate on what these changes are doing to make the code slower. maybe start with master and add one change at a time to see which one is doing it. i had some th oughts about where it could be comign from but wasnt able to figure out why. maybe @genadz could also take a quick look since hes also been in there recently? |
@keygang @kevinkreiser I found a reason of bad performance for default case ( |
@genadz |
you should fix this in all algorithms, because you call |
9f9f38e
to
5aff53a
Compare
@genadz yeah i saw that too and made the change locally but still saw poor performance. I'm not sure what the heck i was doing wrong (probably just too many things at once). If someone has the time that this no longer shows the performance regression i think we can get it reviewed and merged |
35fe1b5
to
c454372
Compare
@keygang @kevinkreiser I tested this branch (with |
yeah i would expect no change for defaults. maybe its the loki thing |
Checking... |
@keygang @kevinkreiser sorry, I checked timings again and didn't notice any difference. so, probably some other local stuff hit timings when I measured them first time. |
I have found that performance drops if EdgeLabels needs to be resized during route computation. One can check the reserved size against the # of labels at the end of computation (I always look at the log statement in FormPath). |
d6e77f8
to
3a2b4a5
Compare
Yes, when the flag is |
732000a
to
17786e0
Compare
@@ -32,8 +32,10 @@ class PathAlgorithm { | |||
/** | |||
* Constructor | |||
*/ | |||
PathAlgorithm() | |||
: interrupt(nullptr), has_ferry_(false), not_thru_pruning_(true), expansion_callback_() { | |||
PathAlgorithm(uint32_t max_reserved_labels_count, bool clear_reserved_memory) |
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.
imho, it's better to pass property tree into the constructor. in that case we would have common parameters parsing
config.get<uint32_t>("max_reserved_labels_count", kInitialEdgeLabelCountBD),
config.get<bool>("clear_reserved_memory", false)
only once 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.
yeah good, remark, but how set kInitialEdgeLabelCountBD
beautifully in PathAlgorithm
? In different algos we set different kInitialEdgeLabelCountBD
, for example for Astar we set 1000000 but for multimodal we set 200000
@genadz
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.
we can update config by default value and forward it to PathAlgoritm
as a copy, is it good?
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.
hm, I don't see a good way how to set different max_reserved_labels_count
for different algorithms and keep parsing only in PathAlgorithm
. so, probably for now it's okay to have constructor as you proposed
PathAlgorithm(uint32_t max_reserved_labels_count, bool clear_reserved_memory)
@kevinkreiser WDYT ?
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.
17786e0
to
2c0f0be
Compare
33ca465
to
ee03fdb
Compare
5d50dbb
to
8248848
Compare
Add boolean parameter to clear memory for edge labels from thor path algorithms.
It will be good for mobile platforms to free extra memory because building a route is a rare task. It frees up about 50mb.