gds: init scc computation using kosaraju's algorithm #4893
Conversation
| void compute() { | ||
| for (const auto& [tableID, numNodes] : numNodesMap) { | ||
| sccState.pinTable(tableID); | ||
| for (auto& [fromEntry, toEntry, relEntry] : graph->getRelFromToEntryInfos()) { |
There was a problem hiding this comment.
Is there a better way to loop through the vertices and their neighbors?
There was a problem hiding this comment.
This is fine. Though I would constraint kosaraju to single rel table for now until we figure out what's the right API for DFS. Let's validate this in StronglyConnectedComponent::bind()
| return; | ||
| }; | ||
| sccState.updateVisit(node.offset, true); | ||
| auto scanState = graph->prepareRelScan(relEntry, ""); |
There was a problem hiding this comment.
This state should be recreated in each recursion, right?
There was a problem hiding this comment.
Yes but the performance might be terrible. I would do something like
auto state = graph->prepareRelScan(); // Always reuse this scan state
dfs(n):
nbrs;
for chunk : scanFwd(state) {
// update nbrs
}
for nbr : nbrs : {
dfs(nbr)
}
There was a problem hiding this comment.
So I implemented this if I understood you correctly. Is it the case that maintaining a second copy of the graph is faster than scanning the main graph?
There was a problem hiding this comment.
I don't quite understand what do you by a copy of graph. I was trying to reuse scanState in the example above.
There was a problem hiding this comment.
Overall LGTM. Let's discuss a bit more with @semihsalihoglu-uw
| return; | ||
| }; | ||
| sccState.updateVisit(node.offset, true); | ||
| auto scanState = graph->prepareRelScan(relEntry, ""); |
There was a problem hiding this comment.
Yes but the performance might be terrible. I would do something like
auto state = graph->prepareRelScan(); // Always reuse this scan state
dfs(n):
nbrs;
for chunk : scanFwd(state) {
// update nbrs
}
for nbr : nbrs : {
dfs(nbr)
}
| } | ||
|
|
||
| private: | ||
| std::vector<nodeID_t> queue; |
There was a problem hiding this comment.
I wonder if this will perform if the size of nodes is 1B. Let's see it's performance first.
Benchmark ResultMaster commit hash:
|
There was a problem hiding this comment.
I just have minor comments.
Let's get some benchmark numbers both in terms of performance and correctness. You can validate this against Neo4j. If the benchmark goes fine, let's extend it to other algorithms as well, Howe did some setup here. You can take a look how much of it will be useful.
Also let's understand if std::queue will scale or not.
| return true; | ||
| } | ||
|
|
||
| bool getVisit(offset_t offset) { return visits[offset]; } |
| for (auto chunk : graph->scanBwd(nodeID, scanState)) { | ||
| chunk.forEach([&](auto nbrNodeID, auto) { nbrs.push_back(nbrNodeID.offset); }); | ||
| } | ||
| neighbors[node] = nbrs; |
There was a problem hiding this comment.
Do we need to save nbrs into neighbors. I thought you could just keep it as local variable. Same for fwdDFS
There was a problem hiding this comment.
This helps reuse the allocation from forwardDFS in backwardsDFS.
| return; | ||
| }; | ||
| sccState.updateVisit(node.offset, true); | ||
| auto scanState = graph->prepareRelScan(relEntry, ""); |
There was a problem hiding this comment.
I don't quite understand what do you by a copy of graph. I was trying to reuse scanState in the example above.
| void compute() { | ||
| for (const auto& [tableID, numNodes] : numNodesMap) { | ||
| sccState.pinTable(tableID); | ||
| for (auto& [fromEntry, toEntry, relEntry] : graph->getRelFromToEntryInfos()) { |
There was a problem hiding this comment.
This is fine. Though I would constraint kosaraju to single rel table for now until we figure out what's the right API for DFS. Let's validate this in StronglyConnectedComponent::bind()
Description
Single-threaded implementation of SCC using Kosaraju's_algorithm.
EdgeCompute, but instead is directly called fromexec().Contributor agreement