contract over nodes connecting edges with differing names/speed #4608

nilsnolde · 2024-02-24T21:19:37Z

most of the changes are concerning tests to play nicely with the new logic. the real changes are just removing a few lines.

I'll share some more insights once I got the last test to build, astar has an obnoxious test which is hard to adapt to the new logic..

Co-authored-by: Kevin Kreiser <kevinkreiser@gmail.com>

…even remove most of edgeinfo stuff for shortcuts, no need to store name, pronunciations etc; adapt all tests which were relying on the fact that names/speed break up shortcuts

nilsnolde · 2024-02-24T21:20:21Z

.gitignore

@@ -61,6 +61,7 @@ CMakeLists.txt.user
 .idea
 /.tidytmp
 vcpkg*/
+*.log


doing some graph build experiments and logging to .log. happens frequently, might as well put it in .gitignore

nilsnolde · 2024-02-24T21:22:17Z

src/mjolnir/shortcutbuilder.cc

-      // Get names - they apply over all edges of the shortcut
-      auto names = edgeinfo.GetNames();
-      auto tagged_values = edgeinfo.GetTaggedValues();
-      auto linguistics = edgeinfo.GetLinguisticTaggedValues();
-
-      auto types = edgeinfo.GetTypes();


those are not needed anymore either

nilsnolde · 2024-02-24T21:24:22Z

test/gurka/test_shortcut.cc

-        gurka::findEdge(reader, layout, "highway", end_node, baldr::GraphId{}, 0, true);
+  for (const auto& pair :
+       node_pairs{{"A", "C"}, {"C", "A"}, {"H", "J"}, {"J", "H"}, {"J", "L"}, {"L", "J"}}) {
+    const auto shortcut = gurka::findEdgeByNodes(reader, layout, pair.first, pair.second);


the findEdges is tripping now that we don't keep the name for the shortcut anymore. in the end actually could keep it, it has no adverse effects other than a few more lines of redundant code (which is why I removed it)

nilsnolde · 2024-02-24T21:24:46Z

test/gurka/test_shortcut.cc

@@ -67,38 +67,6 @@ TEST(Shortcuts, LoopWithoutShortcut) {
  EXPECT_FALSE(shortcut.Is_Valid()) << "Shortcuts found. Check the map.";
 }

-// Here no shortcuts are created. There could be one from A to C with speed 80 but in the opposite


this is entirely redundant now

…es: seems like it's randomly selecting one of the 2, no matter what the costing says..

nilsnolde · 2024-02-25T00:41:34Z

test/astar.cc

@@ -1571,7 +1571,7 @@ TEST(BiDiAstar, test_recost_path) {
  )";
  const gurka::ways ways = {
      // make ABC to be a shortcut
-      {"ABC", {{"highway", "primary"}, {"maxspeed", "80"}}},
+      {"ABC", {{"highway", "motorway"}, {"maxspeed", "80"}}},


this is needed so we still create separate shortcuts

nilsnolde · 2024-02-25T00:49:27Z

CHANGELOG.md

@@ -36,6 +36,9 @@
   * FIXED: fix config generator with thor.costmatrix_allow_second_pass [#4567](https://github.com/valhalla/valhalla/pull/4567)
   * FIXED: infinite loop or other random corruption in isochrones when retrieving partial shape of an edge [#4547](https://github.com/valhalla/valhalla/pull/4547)
   * FIXED: Aggregation updates: update opposing local idx after aggregating the edges, added classification check for aggregation, and shortcut length changes [#4570](https://github.com/valhalla/valhalla/pull/4570)
+   * FIXED: Fix segfault in OSRM serializer with bannerInstructions when destination is on roundabout [#4480](https://github.com/valhalla/valhalla/pull/4481)


those were misplaced in the "Enhancement" section. I didn't make the effort to align them properly time-wise, still better than before

nilsnolde · 2024-02-25T00:50:01Z

src/mjolnir/restrictionbuilder.cc

@@ -270,8 +270,8 @@ ComplexRestrictionBuilder CreateComplexRestriction(const OSMRestriction& restric
 };

 struct Result {
-  uint32_t forward_restrictions_count;
-  uint32_t reverse_restrictions_count;
+  uint32_t forward_restrictions_count = 0;


this finally fixes an issue where we report ridiculous numbers for restrictions, like 2 billion restrictions on level 2

nilsnolde · 2024-02-25T00:51:07Z

test/gurka/test_avoids.cc

@@ -1,4 +1,5 @@
 #include "gurka.h"
+#include <boost/format.hpp>


fixes osx ci

nilsnolde · 2024-02-25T01:40:25Z

wtf.. just wanted to see the performance improvement and sources_to_targets runs into OOM.. even with a single thread and quite a small bbox for random locations. master is quick and hardly any memory.. can't see yet how that's possible..

EDIT: another fluke.. I had a crazy high max_matrix_distance in my config (5 mio km 😂), which creates a super bloated adjacency queue and does lead to OOM. And I had another instance for master, with another config..

nilsnolde · 2024-02-25T13:57:30Z

this is ready for review now

I’ll do some more analysis next week, at least with a country size extract.

nilsnolde · 2024-02-25T14:22:27Z

next week more..

nilsnolde · 2024-02-26T18:04:02Z

there's some minor improvement for de_benchmark_routes.txt with 115 vs 108 secs for the bunch on a persistent server, so like 6-7% improvement. spot checking 15-20 route diffs there was no difference in narration or time/distance.

performance improvement will have an exponential relationship with overall distance, but since bidir a* is pretty directed, it's a pretty scaled down exponential relationship.. I'd expect the p90 routes to have a clearer improvement, e.g. moscow -> madrid or so. diffs for (dijkstra) costmatrix are a lot more pronounced, 20 x 20 random locations in germany over 50 requests show 200 secs (branch) vs 260 secs (master).

here the shortcut status for germany-latest:

features (accumulative)	level 0			level 1
	total shortcuts	total superseded	avg edges per shorcut	total shortcuts	total superseded	avg edges per shorcut
master	161872	849550	6	623364	3863670	5
branch	107146	889932	8	359422	3976950	11

resulting in a minor size reduction of 15 MB for level tiles (411 MB total) and 4 MB for level 0 tiles (113 MB total).

I think this particular one doesn't really need a full planet RAD test yet. It's pretty straight forward and it's fairly easy to see that it won't have any impact on expansion (other than performance) or narration. however, next ones are more sensitive and there I'd run a full planet RAD.

what do you think @gknisely @kevinkreiser @dnesbitt61 ?

nilsnolde · 2024-02-26T18:08:12Z

test/astar.cc

+// TODO(nils): this test fails currently, because bidir A* has a problem with 2 shortcuts between the
+// same nodes: https://github.com/valhalla/valhalla/issues/4609


this disabled test is the reason why the reported coverage is so low BTW.

also, good to mention: if I run this on master (with the appropriate edits to make sure it's 2 shortcuts with different superseded edges between A and E), it fails too.

nilsnolde · 2024-02-29T12:51:52Z

closing in favor of #4613

nilsnolde and others added 6 commits February 24, 2024 16:28

make shortcut logging more useful

ff1f877

comment

764b743

misplaced count

2998344

Update src/mjolnir/shortcutbuilder.cc

5d1afba

Co-authored-by: Kevin Kreiser <kevinkreiser@gmail.com>

address review

c3a12f7

[skip ci] remove names and speed requirements from node contraction; …

875b394

…even remove most of edgeinfo stuff for shortcuts, no need to store name, pronunciations etc; adapt all tests which were relying on the fact that names/speed break up shortcuts

nilsnolde commented Feb 24, 2024

View reviewed changes

Merge branch 'master' into nn-shortcuts-names-speed

d427e65

nilsnolde commented Feb 24, 2024

View reviewed changes

nilsnolde added 4 commits February 24, 2024 22:28

changelog

6026a90

fix osx ci

6d139de

bidir astar is having a problem with 2 shortcuts between the same nod…

26f0751

…es: seems like it's randomly selecting one of the 2, no matter what the costing says..

add failing test but put it disabled

34a7054

nilsnolde commented Feb 25, 2024

View reviewed changes

comment

50bbba7

nilsnolde commented Feb 25, 2024

View reviewed changes

test/gurka/test_avoids.cc

@@ -1,4 +1,5 @@

#include "gurka.h"

#include <boost/format.hpp>

Copy link

Member Author

nilsnolde Feb 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixes osx ci

nilsnolde requested review from kevinkreiser, gknisely and dnesbitt61 and removed request for kevinkreiser February 25, 2024 00:51

nilsnolde added 2 commits February 25, 2024 02:41

roll back changes to name/pronunciation etc

88d8c19

remove names/pronunciations etc from shortcuts again

bb42b4d

nilsnolde closed this Feb 25, 2024

nilsnolde reopened this Feb 26, 2024

nilsnolde commented Feb 26, 2024

View reviewed changes

Merge branch 'master' into nn-shortcuts-names-speed

db7ea53

nilsnolde mentioned this pull request Feb 27, 2024

contract edges with differing non-conditional access restrictions #4613

Merged

nilsnolde closed this Feb 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

contract over nodes connecting edges with differing names/speed #4608

contract over nodes connecting edges with differing names/speed #4608

nilsnolde commented Feb 24, 2024 •

edited

nilsnolde Feb 24, 2024

nilsnolde Feb 24, 2024

nilsnolde Feb 24, 2024

nilsnolde Feb 24, 2024

nilsnolde Feb 25, 2024

nilsnolde Feb 25, 2024

nilsnolde Feb 25, 2024

nilsnolde Feb 25, 2024

nilsnolde commented Feb 25, 2024 •

edited

nilsnolde commented Feb 25, 2024 •

edited

nilsnolde commented Feb 25, 2024

nilsnolde commented Feb 26, 2024 •

edited

nilsnolde Feb 26, 2024

nilsnolde commented Feb 29, 2024

		@@ -1,4 +1,5 @@
		#include "gurka.h"
		#include <boost/format.hpp>

		// TODO(nils): this test fails currently, because bidir A* has a problem with 2 shortcuts between the
		// same nodes: https://github.com/valhalla/valhalla/issues/4609

contract over nodes connecting edges with differing names/speed #4608

contract over nodes connecting edges with differing names/speed #4608

Conversation

nilsnolde commented Feb 24, 2024 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nilsnolde commented Feb 25, 2024 • edited

nilsnolde commented Feb 25, 2024 • edited

nilsnolde commented Feb 25, 2024

nilsnolde commented Feb 26, 2024 • edited

Choose a reason for hiding this comment

nilsnolde commented Feb 29, 2024

nilsnolde commented Feb 24, 2024 •

edited

nilsnolde commented Feb 25, 2024 •

edited

nilsnolde commented Feb 25, 2024 •

edited

nilsnolde commented Feb 26, 2024 •

edited