Add boolean parameter to clear memory for edge labels from thor

[keygang]

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.

[kevinkreiser]

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.

[kevinkreiser]

@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

[kevinkreiser]

bad news! performance is absolutely aweful compared to master... im not quite sure where the hell its going though... @genadz any thoughts?

[kevinkreiser]

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();
}

[kevinkreiser]

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

[keygang]

@kevinkreiser
you haven't watched this PR again?

[kevinkreiser]

@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?

[genadz]

@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 (clear_reserved_memory=false). Function Algorithm::Clear() is called on each request. In that function we initialize (vector::resize) labels vector with max_reserved_labels_count elements and then immediately clear it. So, we shouldn't call resize if number of elements less than allowed capacity.

[keygang]

@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 (clear_reserved_memory=false). Function Algorithm::Clear() is called on each request. In that function we initialize (vector::resize) labels vector with max_reserved_labels_count elements and then immediately clear it. So, we shouldn't call resize if number of elements less than allowed capacity.

@genadz
#2789 (comment)
yes, but @kevinkreiser checked without resize when size is small the max_reserved_labels

[genadz]

yes, but @kevinkreiser checked without resize when size is small the max_reserved_labels

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

you should fix this in all algorithms, because you call Clear() here https://github.com/valhalla/valhalla/blob/master/src/thor/worker.cc#L343-L356 per each request

[kevinkreiser]

I found a reason of bad performance for

@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

[genadz]

@keygang @kevinkreiser I tested this branch (with clear_reserved_memory=false) locally against master and found out that this branch is slower by ~2%. interesting why

[kevinkreiser]

yeah i would expect no change for defaults. maybe its the loki thing

[genadz]

yeah i would expect no change for defaults. maybe its the loki thing

Checking...

[genadz]

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.

[dnesbitt61]

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).

[keygang]

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).

Yes, when the flag is true, we need to resize labels during route computation. But we need to clean up labels in order not to take up extra memory on mobile platforms which we don't use, because these labels take up a lot of space

[genadz]

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.

[keygang]

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

[keygang]

we can update config by default value and forward it to PathAlgoritm as a copy, is it good?

[genadz]

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 ?

[kevinkreiser]

@genadz @keygang i agree, its completley fine to keep it that way until we think of something cleaner