Improve transitive path computation with GraphBLAS #1252

JoBuRo · 2024-01-30T15:18:04Z

This PR changes the implementation of the TransitivePath class.
Previously it used DFS to compute the transitive hull. The new implementation uses sparse, boolean matrices instead. The matrix object and functionalities come from the SuiteSparse:GraphBLAS library [website | github] .
This library is added as new dependency. The new class GrbMatrix wraps the library for this application.

joka921

I have made a first pass over the Matrix class, haven't looked at the Transitive Path and the tests yet.

joka921 · 2024-01-31T16:05:20Z

.github/workflows/macos.yml

@@ -41,6 +41,7 @@ jobs:
        run:  |
          brew install llvm@16
          brew install conan@2
+          brew install suite-sparse


This currently doesn't seem to work,
I will try to figure out how to do this on our Mac.
What we also could do is to compile GraphBLAS alongside QLever, but only if
it is explicitly requested.

(of course this would increase build times, I'd have to check if this is feasible, and probably it would be problematic for CI builds.

test/CMakeLists.txt

src/engine/GrbMatrix.h

src/engine/GrbMatrix.cpp

joka921

First pass over the rest!

joka921 · 2024-01-31T17:29:41Z

src/engine/TransitivePath.h

+  bool isContained(Id id) { return idMap_.contains(id); }
+
+  size_t addId(Id id) {
+    if (!idMap_.contains(id)) {


You can make this nicer and more efficient by using

idMap_.try_emplace().

The fact the nextIndex_ is always equal to idMap_.size() (the explicit data member is not needed.

(and equal to indexMap.size).

I'm not sure I'm using the try_emplace as you intended, but I think still need to check if idMap_ contains id. If the id is not contained I need to add to the vector indexMap_.

joka921 · 2024-01-31T17:30:11Z

src/engine/TransitivePath.h

+  ad_utility::HashMap<Id, size_t> idMap_{};
+
+  std::vector<Id> indexMap_;


Make them private and include memory limits via the AllocatorWithMemoryLimit (can be applied everywhere).

I've tried using ad_utility::HashMapWithMemoryLimit<Id, size_t>, but on compile I get:

error: call to deleted constructor of 'allocator_type' (aka 'ad_utility::AllocatorWi thLimit<std::pair<const ValueId, unsigned long>>')

src/engine/TransitivePath.h

src/engine/TransitivePath.cpp

joka921 · 2024-01-31T17:48:50Z

src/engine/TransitivePath.cpp

-      rowIndex++;
+      table(resultRowIndex, startSideCol) = startNode;
+      table(resultRowIndex, targetSideCol) = targetNode;
+      resultRowIndex++;


We can have a look at the possibilities of GraphBlas to check what is the most efficient way to get a set of rows out.

src/engine/TransitivePath.cpp

joka921 · 2024-01-31T17:53:50Z

src/engine/TransitivePath.cpp

+                                       size_t targetIndex) const {
+  GrbMatrix transformer = GrbMatrix(hull.ncols(), hull.ncols());
+  transformer.setElement(targetIndex, targetIndex, true);
+  return hull.multiply(transformer);


Isn't actually this the solution to get multiple rows at once if you have multiple start indices? Just with a transformer that has more than one entry?
That way you don't have that many getRow() calls, but a single "getAllRows" followed by an extraction.

joka921 · 2024-01-31T17:56:15Z

src/engine/TransitivePath.cpp

+  for (size_t i = 0; i < numRows; i++) {
+    auto startId = startCol[i];
+    auto targetId = targetCol[i];
+    auto startIndex = mapping.addId(startId);
+    auto targetIndex = mapping.addId(targetId);
+
+    rowIndices.push_back(startIndex);
+    colIndices.push_back(targetIndex);


Since both your input and output format are column based, write a lambda that handles one of the columns, and call it twice. (Maybe this can even be a member function addAll of the mapping.
And in general: If you know the needed size of a vector in advance, you should reserve it.

test/GrbMatrixTest.cpp

joka921

An idea to increase the visibility of the fallback.

joka921 · 2024-02-01T08:17:17Z

src/engine/TransitivePath.h

+  // A small helper function: Insert the `value` to the set at `map[key]`.
+  // As the sets all have an allocator with memory limit, this construction is a
+  // little bit more involved, so this can be a separate helper function.
+  void insertIntoMap(Map& map, Id key, Id value) const;


Thank you very much.
I think we should definitely split the two implementations.
I think we can just use basic inheritance:

A class TransitivePathBase that has all the stuff that the class currently has EXCEPT for the override of computeResult() + all the helper functions needed for it.

One class TransitivePathGraphblas and one class TransitivePathFallback that both inherit from the base and implement their repsecive computeResults.

: The base class has a function static shared_pr<TransitivePathBase> makeTransitivePath(..... all the args, bool use fallback). (Trick: only declare it in the header, and implement it in the .cpp` file, in which you can include both implementations.

- Renamed nrows, ncols and nvals - Moved code to cpp file - Nullptr is now valid for GrbMatrix::matrix_

- Renamed copy() -> clone() - Functions that create a GrbMatrix now use the unique_ptr directly

- Remove nextIndex_ - Made internal datastructures private - isContained -> contains - const Id& -> Id

- Remove C style arrays - Add checkCancellation

codecov · 2024-02-07T17:49:25Z

Codecov Report

Attention: Patch coverage is 67.80000% with 161 lines in your changes are missing coverage. Please review.

Project coverage is 86.94%. Comparing base (bcc05cc) to head (714744a).

❗ Current head 714744a differs from pull request most recent head f6e2429. Consider uploading reports for the commit f6e2429 to get more accurate results

Files	Patch %	Lines
src/engine/TransitivePath.cpp	51.02%	111 Missing and 9 partials ⚠️
src/engine/GrbMatrix.cpp	79.08%	21 Missing and 20 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1252      +/-   ##
==========================================
- Coverage   88.10%   86.94%   -1.16%     
==========================================
  Files         310      310              
  Lines       28436    28410      -26     
  Branches     3148     3168      +20     
==========================================
- Hits        25053    24701     -352     
- Misses       2231     2552     +321     
- Partials     1152     1157       +5

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Removed unnecessary unique_ptr and make_optionals

- Throw exception on GrB_NO_VALUE - unique_ptr in GrbMatrix uses custom delter - Simplified GrbMatrixTest

# Conflicts: # src/global/Constants.h # test/TransitivePathTest.cpp

sonarqubecloud · 2024-03-14T16:58:51Z

Quality Gate failed

Failed conditions
9.1% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

joka921 · 2024-04-18T13:44:16Z

We have now merged the better implementation that uses a binary search,
so we can then close this PR.

JoBuRo added 8 commits January 30, 2024 13:32

Added graphblas dependecies

fd1bdf8

Added wrapper for graphblas matrix

285528b

Replaced transitiveHull computation

935bc0d

Added extern keyword around include

069819f

Replaced std map with abseil map

4b257fe

Added graphblas dependency for GH action

d4ba6f6

Added library for mac build

1b20a1e

Removed finalize()

92621d0

joka921 requested changes Jan 31, 2024

View reviewed changes

Added fallback for GraphBLAS

b3d71bd

joka921 requested changes Feb 1, 2024

View reviewed changes

JoBuRo and others added 15 commits February 1, 2024 14:21

Reworks GrBMatrix

d1de484

- Renamed nrows, ncols and nvals - Moved code to cpp file - Nullptr is now valid for GrbMatrix::matrix_

More reworks on GrbMatrix

303ff2f

- Renamed copy() -> clone() - Functions that create a GrbMatrix now use the unique_ptr directly

Renamed getMatrix -> matrix

958b4f7

Added documentation to GrbMatrix

87bc650

Reworked build function

370ad43

Reworked extractRow and extractCol

0018c84

Reworked use of C arrays in GrbMatrix

622dddd

Additional reworks for GrbMatrix

7c7d5b6

Reworked extractTuples

21a5994

Added a quick fix for GrB_init issue

6f0f8cd

Reworked IdMapping

7269e4c

- Remove nextIndex_ - Made internal datastructures private - isContained -> contains - const Id& -> Id

Reworks on TransitivePath

97d766a

- Remove C style arrays - Add checkCancellation

Merge branch 'master' into use-graphblas

c8c6526

A tiny bugfix and make the stuff configurable.

a06ed71

Fix build error, add move assignment to GrbMatrix

6e84e8a

hannahbast mentioned this pull request Feb 8, 2024

Timeout Error When Running SPARQL Query. #1267

Closed

Simplifications

005dc9c

Removed unnecessary unique_ptr and make_optionals

JoBuRo and others added 16 commits February 9, 2024 11:12

Reworks

e7dbf4d

- Throw exception on GrB_NO_VALUE - unique_ptr in GrbMatrix uses custom delter - Simplified GrbMatrixTest

Added timer to transitive path computation

714744a

**WIP** Refactor of TransitivePath into Fallback and Graphblas

0fd9f92

Fixed timing conversion

60a37b6

Added singleton class for GraphBLAS global context

d07401e

Added some checkCancellation

1f424de

Build fix

d64f4b6

Removed dead code

816ba9a

Sonar fixes

ecdf2a7

Add timings to runtime info

12f0e3d

Some fixes.

ed6dc70

Added binary search, updated tests

dd8c841

Merge branch 'master' into use-graphblas

50e26ba

# Conflicts: # src/global/Constants.h # test/TransitivePathTest.cpp

Fix merge conflicts.

4e64224

Moved sort to constructor

b79470d

Merge remote-tracking branch 'origin/master' into use-graphblas

f6e2429

joka921 closed this Apr 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve transitive path computation with GraphBLAS #1252

Improve transitive path computation with GraphBLAS #1252

JoBuRo commented Jan 30, 2024

joka921 left a comment

joka921 Jan 31, 2024

joka921 Jan 31, 2024

joka921 left a comment

joka921 Jan 31, 2024

joka921 Jan 31, 2024

JoBuRo Feb 5, 2024

joka921 Jan 31, 2024

JoBuRo Feb 5, 2024

joka921 Jan 31, 2024

joka921 Jan 31, 2024

joka921 Jan 31, 2024

joka921 left a comment

joka921 Feb 1, 2024

codecov bot commented Feb 7, 2024 •

edited

Loading

sonarqubecloud bot commented Mar 14, 2024

joka921 commented Apr 18, 2024

		ad_utility::HashMap<Id, size_t> idMap_{};

		std::vector<Id> indexMap_;

Improve transitive path computation with GraphBLAS #1252

Improve transitive path computation with GraphBLAS #1252

Conversation

JoBuRo commented Jan 30, 2024

joka921 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joka921 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joka921 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Feb 7, 2024 • edited Loading

Codecov Report

sonarqubecloud bot commented Mar 14, 2024

Quality Gate failed

joka921 commented Apr 18, 2024

codecov bot commented Feb 7, 2024 •

edited

Loading