Replace the nodesTentativeTime from a map to a vector #262

greenscientist · 2023-10-23T16:22:28Z

We used the node global as a max entry to size the vector.

Fetching the temporary data from a map in the calculation loop is too intensive. Using a vector with a constant access is more efficient. We just need to be careful in match the UID of the Nodes

We used the node global as a max entry to size the vector. Fetching the temporary data from a map in the calculation loop is too intensive. Using a vector with a constant access is more efficient. We just need to be careful in match the UID of the Nodes

greenscientist · 2023-10-23T16:23:01Z

Same benchmark result
Summary time 10.911 10,911 Summary time 14.969 14,969 27,11 %
Summary time 0.008 0,008 Summary time 0.009 0,009 11,11 %
Summary time 0.006 0,006 Summary time 0.013 0,013 53,85 %
Summary time 0.007 0,007 Summary time 0.031 0,031 77,42 %
Summary time 0.010 0,01 Summary time 0.022 0,022 54,55 %
Summary time 0.032 0,032 Summary time 0.059 0,059 45,76 %
Summary time 16.168 16,168 Summary time 21.754 21,754 25,68 %
Summary time 11.704 11,704 Summary time 15.889 15,889 26,34 %
Summary time 8.169 8,169 Summary time 10.793 10,793 24,31 %
Summary time 4.409 4,409 Summary time 5.852 5,852 24,66 %
Summary time 1.778 1,778 Summary time 2.588 2,588 31,30 %
Summary time 5.821 5,821 Summary time 8.292 8,292 29,80 %
Summary time 4.769 4,769 Summary time 6.161 6,161 22,59 %
Summary time 5.414 5,414 Summary time 7.193 7,193 24,73 %
Summary time 6.806 6,806 Summary time 9.563 9,563 28,83 %
Summary time 5.642 5,642 Summary time 7.872 7,872 28,33 %
Summary time 14.244 14,244 Summary time 18.694 18,694 23,80 %
Summary time 10.944 10,944 Summary time 14.432 14,432 24,17 %
Summary time 8.180 8,18 Summary time 9.947 9,947 17,76 %
Summary time 0.667 0,667 Summary time 0.695 0,695 4,03 %
Summary time 0.033 0,033 Summary time 0.042 0,042 21,43 %
Summary time 0.029 0,029 Summary time 0.039 0,039 25,64 %

greenscientist · 2023-10-23T16:23:15Z

Confirmed that we get the same data as result.

greenscientist · 2023-10-23T16:23:31Z

This is an alternative to #261

tahini · 2023-10-23T16:33:51Z

Benchmark is new vs old? The old is not the same as the old from the other PR, normal?

tahini · 2023-10-23T16:44:09Z

connection_scan_algorithm/src/resets.cpp

@@ -80,7 +80,8 @@ namespace TrRouting
      int footpathDistanceMeters;
      nodesAccess.clear();
      forwardJourneysSteps.clear();
-      nodesTentativeTime.clear();
+      nodesTentativeTime.assign(Node::getMaxUid() + 1, MAX_INT); //Assign default values to all indexes


So this might explode if we re-read the cache thousands of times in a run, right? But once or twice, it's just wasted space. We're not even sure re-reading the cache works. I think we have an API call, but it was never tested. Maybe we could reset the global ID when we start reading the cache? Anyway, good enough for now!

Yeah, the feature might be broken as you say, and we don't have use case for it at the moment, so we will be good for now.

tahini

This PR is a simpler change than the other one. I think I prefer it

tahini · 2023-10-23T16:49:17Z

connection_scan_algorithm/include/calculator.hpp

@@ -113,7 +113,7 @@ namespace TrRouting
    int              minEgressTravelTime;
    long long        calculationTime;

-    std::unordered_map<Node::uid_t, int> nodesTentativeTime; // arrival time at node
+    std::vector<int> nodesTentativeTime; // arrival time at node, using the Node::id as index
    std::unordered_map<Node::uid_t, int> nodesReverseTentativeTime; // departure time at node


As in the other PR, should you change this too?

What do you mean for this?

I left a comment in the other PR, but it is still pending, so you don't know about it... ;-)

But it's the same kind of map as the one you are changing, is it used anywhere? a little or a lot? Should it receive the same treatment as the nodesTentativeTime and switch to a vector

Yes, it will. It's not are main time sink for the moment, so I wanted to do it it a separate commit at least. I can bundle it in this PR if you want or wait for a following. I kind of wanted to see the impact on the infra with just the first part. But I can do both if you thinks it's better

Ok, we can try this one in the infra. It's just that those 2 are symetric, so they might be done at the same time, but it would be interesting to benchmark them separately and see if it has any impact at all or none to change it. If you're going to do it next, then I'll approve this one.

greenscientist · 2023-10-23T17:03:50Z

Benchmark is new vs old? The old is not the same as the old from the other PR, normal?

New vs old yes.
I've rerun the old to make sure I did not have too much external impact on my computer.

tahini · 2023-10-23T17:13:07Z

connection_scan_algorithm/include/calculator.hpp

@@ -113,7 +113,7 @@ namespace TrRouting
    int              minEgressTravelTime;
    long long        calculationTime;

-    std::unordered_map<Node::uid_t, int> nodesTentativeTime; // arrival time at node
+    std::vector<int> nodesTentativeTime; // arrival time at node, using the Node::id as index
    std::unordered_map<Node::uid_t, int> nodesReverseTentativeTime; // departure time at node


Ok, we can try this one in the infra. It's just that those 2 are symetric, so they might be done at the same time, but it would be interesting to benchmark them separately and see if it has any impact at all or none to change it. If you're going to do it next, then I'll approve this one.

greenscientist · 2023-10-23T17:17:49Z

Yep, I'll this that one next.
After the data, the tripQueryData should also gives us a little boost

greenscientist requested a review from tahini October 23, 2023 16:22

tahini reviewed Oct 23, 2023

View reviewed changes

tahini approved these changes Oct 23, 2023

View reviewed changes

greenscientist merged commit 9b91783 into chairemobilite:v2c Oct 23, 2023
4 checks passed

greenscientist mentioned this pull request Oct 27, 2023

Move the data from the nodesTentativeTime to a mutable inside the node object #261

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace the nodesTentativeTime from a map to a vector #262

Replace the nodesTentativeTime from a map to a vector #262

greenscientist commented Oct 23, 2023

greenscientist commented Oct 23, 2023

greenscientist commented Oct 23, 2023

greenscientist commented Oct 23, 2023

tahini commented Oct 23, 2023

tahini Oct 23, 2023

greenscientist Oct 23, 2023

tahini left a comment

tahini Oct 23, 2023

greenscientist Oct 23, 2023

tahini Oct 23, 2023

greenscientist Oct 23, 2023

tahini Oct 23, 2023

greenscientist commented Oct 23, 2023

tahini Oct 23, 2023

greenscientist commented Oct 23, 2023

Replace the nodesTentativeTime from a map to a vector #262

Replace the nodesTentativeTime from a map to a vector #262

Conversation

greenscientist commented Oct 23, 2023

greenscientist commented Oct 23, 2023

greenscientist commented Oct 23, 2023

greenscientist commented Oct 23, 2023

tahini commented Oct 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tahini left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greenscientist commented Oct 23, 2023

Choose a reason for hiding this comment

greenscientist commented Oct 23, 2023