Refactoring MetaDataObject out of DenseMatrix

[corepointer]

This PR moves the MetaDataObject (MDO) functionality out of DenseMatrix and generalizes it to be used by other classes derived from Structure as well.

Furthermore, this contains a performance improvement to prevent excessive allocation ID lookups and a separation of ranged and full allocations.

All tests are running except the distributed ones.

[corepointer]

The numerous force pushes are a result of my local clang-format disagreeing with the CI's clang-format:

 --- src/runtime/local/datastructures/AllocationDescriptorGRPC.h	(original)
+++ src/runtime/local/datastructures/AllocationDescriptorGRPC.h	(reformatted)
@@ -35,7 +35,7 @@
   public:
     AllocationDescriptorGRPC() = default;
     AllocationDescriptorGRPC(DaphneContext *ctx, const std::string &address, const DistributedData &data)
-        : ctx(ctx), workerAddress(address), distributedData(data) {};
+        : ctx(ctx), workerAddress(address), distributedData(data){};
 
     ~AllocationDescriptorGRPC() override = default;
     [[nodiscard]] ALLOCATION_TYPE getType() const override { return type; };

[corepointer]

Explaining the labels:

• feature: CUDA handling CSRMatrix is new
• Accelerator: it's (also) about CUDA ops
• Distributed: the refactoring affects this component
• Performance: besides this one explaining itself, the pinning and being able to run sparse stuff on GPU help with performance 💪

[philipportner]

Hi @corepointer, thanks for putting so much effort into improving this code.

This PR is pretty big and includes quite a few important changes at the core of our data structures.

As there's a lot of changes to this core code, but no changes to the documentation, could you please update the documentation describing the overall idea behind the whole MetaDataObject and IAllocatoinDescriptor functionality?

Especially the knowledge that is hard to get from just reading the code, like what it can and what it cannot do (and what it should do?). From what I can remember of previous discussions and you mention "ranged and full allocations", but maybe I'm wrong, it should handle parts of a DenseMatrix being allocated on CPU and parts of it being moved to GPU? Stuff like that :)

Additionally, it would be great if this can explicitly be tested as part of our test suite. Proper testing of this core functionality would be incredibly beneficial for working on this code in the future. This should be doable by providing mock implementations of the IAllocationDescriptor.

Lastly, could you please provide some performance results for these changes that measure these improvements you mention concerning excessive allocation ID lookups?

I'd also be happy to do a review of the PR if you like.

[pdamme]

I was also just finishing reading the code: Thanks for this contribution, @corepointer. Refactoring the use of the meta data objects to also support CSRMatrix and to solve some of the performance issues of the vectorized engine runtime is highly welcome!

Major comments:

1. I think I have a very rough idea of how the meta data object etc. works on main, but I cannot claim that my understanding is deep enough to be able to judge all implications of your changes. While I didn't spot any obvious big problems, I totally second @philipportner's request for more developer documentation (e.g., a page about the ideas behind the meta data object and related entities and how they are used by the data structures etc.).
2. Likewise about the performance implications: You mentioned that the "pinning" of the allocation can make the repeated calls to getValues() in the vectorized engine faster. Do you have a concrete example where this performance improvement can be observed? A quick experiment of bin/daphne --vec test/api/cli/algorithms/kmeans.daphne r=1000000 f=1000 c=5 i=10 didn’t show any improvement compared to the main branch on my system (but I didn’t try hard to find a good example).

A few detailed comments:

1. CSRMatrix.h/cpp: The renaming of getValues() to getRowValues() and getColIdxs() to getColIdxsOfRow() is inconsistent. Could we call the former getValuesOfRow() (or, do we really need the renaming at all)?
2. AggCum.h: You introduced variables arg_inc and res_inc, which are either the numCols or the rowSkip, depending on whether the respective matrix is a vew. However, the rowSkip is meant to abstract exactly that: no matter if a matrix is an "original" or a view, the rowSkip is always the number by which one must increase a pointer in order to get to the element below.

[corepointer]

Thanks for the feedback @philipportner and @pdamme. And thanks for a taste of my one medicine (I frequently note the lacking documentation and test cases in code reviews) 😆 I will, of course, fix this.

• Pinning performance improvement: The idea originated from an observation of @philipportner that a lot of time is spent in getDataPlacementByID(), which ran a double for loop on every get/set of a value. Do you remember (@philipportner) what that script was where you encountered the issue? I'd use that as a starting point.
• Adding a getRowValues() (in addition to getValues()) was an ad-hoc decision concerning the naming and can certainly be changed.
• In the AggCum.h issue I can't remember off the top of my head now what exactly the issue was. I think I found a case where the increment in the algorithm needed to be numCols for some cases and rowSkip for other cases depending on whether there is a view or a "real" matrix.