Skip to content
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

Distances in MLD #5019

Merged
merged 3 commits into from
May 8, 2018
Merged

Distances in MLD #5019

merged 3 commits into from
May 8, 2018

Conversation

ghoshkaj
Copy link
Member

@ghoshkaj ghoshkaj commented Apr 12, 2018

Issue

#5013 - Distances in Matrix, MLD

Similar to unpacking in CH, we need to do two things for MLD

  1. From the bidirectional search and the unidirectional search in the table plugin for MLD, we need to recreate the "shortcut" or "packed" path.
  2. We need to unpack the path and calculate the distance of each packed edge while doing so

Details

  1. Finding the packed path
  • ManyToMany/Bidirectional search: Path finding already happens in the forward routing steps and the backward routing steps in the MLD ManyToManySearch function. The idea is to use the heap from the ForwardRoutingStep and the search space buckets in the BackwardRoutingStep to recreate the packed or shortcutted path.

  • OneToMany/Unidirectional Search: Path finding already happens during the unidirectional search - however, we need to extract the packed path from the heap and the multi-map that it uses.

  1. We need as the original issue says, to repurpose the unpacking part of the Search function to compute the distances with. We also have to repurpose some of the heaps I believe, as the SearchEngineData heaps aren't manyToMany heaps, but rather, just regular heaps.

Tasklist

Requirements / Relations

Link any requirements here. Other pull requests this PR is based on?

@ghoshkaj ghoshkaj force-pushed the mld-distances branch 3 times, most recently from 1beecb1 to 0e2092a Compare April 20, 2018 02:47
@chaupow chaupow changed the base branch from master to distances April 20, 2018 10:22
@ghoshkaj ghoshkaj force-pushed the distances branch 3 times, most recently from 649e0af to 5ebc145 Compare April 20, 2018 21:21
@ghoshkaj ghoshkaj force-pushed the mld-distances branch 5 times, most recently from 1048bd3 to 1ac22d0 Compare April 23, 2018 22:53
@TheMarex TheMarex added this to the distance-matrix milestone Apr 24, 2018
@ghoshkaj ghoshkaj force-pushed the mld-distances branch 6 times, most recently from 181486a to 0470d55 Compare April 30, 2018 11:04
@ghoshkaj ghoshkaj changed the base branch from distances to master May 1, 2018 01:31
@chaupow
Copy link
Member

chaupow commented May 2, 2018

the changes in profiles/testbot.lua break the test in cucumber features/testbot/traffic_speeds.feature:34

We need to investigate the profile changes and adjust features/testbot/traffic_speeds.feature if we want the change.

cc @TheMarex @ghoshkaj

@oxidase oxidase force-pushed the mld-distances branch 2 times, most recently from d3d24b1 to 40c025c Compare May 6, 2018 14:33
@oxidase oxidase force-pushed the mld-distances branch 2 times, most recently from e5598d7 to ecf8016 Compare May 6, 2018 15:41
@oxidase
Copy link
Contributor

oxidase commented May 7, 2018

@ghoshkaj @chaupow I have pushed a commit that contains some fixes for the branch:

  • phantom nodes selection for unpacking at ecf8016#diff-33262109709646f9865da9ee18f04f50R390 and ecf8016#diff-33262109709646f9865da9ee18f04f50R653
    Phantom nodes are used in getNodeQueryLevel functions to compute an overlay level for nodes processed by MLD.

  • added DIRECTION argument to retrievePackedPathFromSearchSpace and if DIRECTION is REVERSE_DIRECTION edges will be reversed.

  • simplified logic to compute phantom nodes offsets, by moving checks DIRECTION == REVERSE_DIRECTION before of the conditional branches and swapping source_phantom_index and target_phantom_index

  • extended Testbot - Multi level routing test case to check also the distances table.

@TheMarex TheMarex force-pushed the mld-distances branch 2 times, most recently from 2351bdc to 8537df0 Compare May 7, 2018 15:56
@chaupow
Copy link
Member

chaupow commented May 7, 2018

ran current branch and compared with CH numbers:

What distances with MLD distances with CH
100x100 in us-west 30.7s 5.6s
100x100 in la max dist 20km 5.88s 800ms

@chaupow
Copy link
Member

chaupow commented May 8, 2018

ran current branch and measured 25x25 requests

What distances with MLD
25x25 in us-west 2.1s
25x25 in la max dist 20km 600ms

Copy link
Member

@TheMarex TheMarex left a comment

Choose a reason for hiding this comment

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

Some cleanup needs to be done.

@@ -21,6 +21,7 @@ Feature: Basic Distance Matrix
| a | 0 | 100+-1 |
| b | 100+-1 | 0 |

@ch
Copy link
Member

Choose a reason for hiding this comment

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

We should capture why these tests only work on CH.

@@ -45,6 +46,7 @@ Feature: Basic Distance Matrix
| c | | | 0 | 100+-1 |
| d | | | 100+-1 | 0 |

@ch
Copy link
Member

Choose a reason for hiding this comment

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

Same.

namespace
{
struct NodeBucket
{
NodeID middle_node;
NodeID parent_node;
bool from_clique_arc;
Copy link
Member

Choose a reason for hiding this comment

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

We cam optimize the size here with bit packing:

unsigned column_index : 31;
unsigned from_clique_arc : 1;

@@ -351,6 +446,21 @@ UnpackedPath search(SearchEngineData<Algorithm> &engine_working_data,
// Get packed path as edges {from node ID, to node ID, from_clique_arc}
auto packed_path = retrievePackedPathFromHeap(forward_heap, reverse_heap, middle);

// if (!packed_path.empty())
Copy link
Member

Choose a reason for hiding this comment

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

This is left-over debugging code and needs to be removed.

@@ -410,7 +520,96 @@ UnpackedPath search(SearchEngineData<Algorithm> &engine_working_data,
unpacked_edges.insert(unpacked_edges.end(), subpath_edges.begin(), subpath_edges.end());
}
}
// std::cout << "unpacked_nodes: ";
Copy link
Member

Choose a reason for hiding this comment

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

Same.

end
if turn.has_traffic_light then
turn.duration = turn.duration + profile.properties.traffic_light_penalty
end

io.write("after penalty turn.duration: ", turn.duration, "turn.weight: ", turn.weight, "\n")
Copy link
Member

Choose a reason for hiding this comment

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

This is debug code and needs to be removed.

@@ -74,6 +74,15 @@ InternalRouteResult directShortestPathSearch(SearchEngineData<mld::Algorithm> &e
auto &reverse_heap = *engine_working_data.reverse_heap_1;
insertNodesInHeaps(forward_heap, reverse_heap, phantom_nodes);

std::cout << "source_phantom.forward_segment_id.id: "
Copy link
Member

Choose a reason for hiding this comment

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

This is debug code and needs to be removed.


// Store settled nodes in search space bucket
search_space_with_buckets.emplace_back(
node, parent, column_index, target_weight, target_duration);
node, parent, INVALID_CLIQUE_ARC_TYPE, column_index, target_weight, target_duration);
Copy link
Member

Choose a reason for hiding this comment

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

Instead of pushing this to the call-site we can add a second constructor.

: facade.IsBackwardEdge(edge))
if ((DIRECTION == FORWARD_DIRECTION ? facade.IsForwardEdge(edge)
: facade.IsBackwardEdge(edge)) &&
!query_heap.WasInserted(facade.GetTarget(edge)))
Copy link
Member

Choose a reason for hiding this comment

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

We should pull out facade.GetTarget(edge) in an own variable.

This commit brings feature parity with CH for the `table` pluging.
@TheMarex TheMarex merged commit 3b4e2e8 into master May 8, 2018
@TheMarex TheMarex deleted the mld-distances branch May 8, 2018 15:50
@danpat danpat mentioned this pull request Oct 29, 2018
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants