Refactored the IdTable class to make interfaces simpler + more correct #320

joka921 · 2020-03-10T12:26:47Z

The underlying storage is now std::vector<Id> instead of Id* + malloc + reinterpret_cast (which
was as I understand it technically UB!
We now have two really separate types IdTableStatic<COLS> which owns its storage and
IdTableView<COLS> which is a readOnly, non-owning view of an IdTable. this makes the interface much more explicit and helped me track a small bug. (Previously both were IdTableStatic where some where internally managing there storage and some not.
Cleaned up the code of the IdTable module by introduing some using directives to get rid of a lot of IdTableStatic<COLS>:: namespace usages which made the code somewhat hard to read.

niklas88 · 2020-04-12T20:54:11Z

src/engine/Join.cpp

+        // this fixes a bug that was not discovered by testing due to 0
+        // initialization of IdTables used for testing and should not occur in
+        // typical use cases but it is still wrong.
+        return;


Do we maybe want to do this as an independent PR now? Seems simple enough

- Removed a lot of code and implemented const_iterators and read-only views.

niklas88

LGTM with just a few nitpicks and questions

niklas88 · 2020-04-13T19:56:42Z

src/engine/Comparators.h

@@ -16,8 +16,8 @@ class OBComp {
      : _sortIndices(sortIndices) {}

  template <int I>
-  bool operator()(const typename IdTableImpl<I>::Row& a,
-                  const typename IdTableImpl<I>::Row& b) const {
+  bool operator()(const typename IdTableStatic<I>::Row& a,


Let's rename the I to WIDTH as in Engine.h

niklas88 · 2020-04-13T19:56:50Z

src/engine/CountAvailablePredicates.cpp

@@ -231,7 +231,7 @@ void CountAvailablePredicates::computePatternTrick(
    const CompactStringVector<Id, Id>& hasPredicate,
    const CompactStringVector<size_t, Id>& patterns, const size_t subjectColumn,
    RuntimeInformation* runtimeInfo) {
-  const IdTableStatic<I> input = dynInput.asStaticView<I>();
+  const IdTableView<I> input = dynInput.asStaticView<I>();


Same as above

niklas88 · 2020-04-13T20:14:05Z

src/engine/IdTable.h

+      return *this;
+    }
+    // This class cannot use move semantics if at least one of the two
+    // rows invovlved in an assigment does not manage it's data, but rather


niklas88 · 2020-04-13T20:16:59Z

src/engine/IdTable.h

-      return *this;
+    // Copy over the data from the other row to this row
+    if (_cols == other._cols && _data != nullptr && other._data != nullptr) {
+      std::memcpy(_data, other._data, sizeof(Id) * _cols);


couldn't oter._data be the one we previously set nullptr in line 418? I.e. we should only do the std::memcpy if other was a view.

No, because line 420 returns.
I just checked again (this logic has been here for quite some time)
and all 4 cases (this and other can both be allocated or views) are handled correctly.

If both are allocated, we just redirect pointers and return (line 411).
In any other case we have to do the memcpy either to the IdTable directly (this is a view) or to newly
allocated storage (this is allocated).

niklas88 · 2020-04-13T20:34:56Z

src/engine/IdTable.h

-      return matches;
+  bool operator==(const Row& other) const {
+    bool matches = _cols == other._cols;
+    for (size_t i = 0; matches && i < _cols; i++) {


Ahh this is one of these cases that make you wonder if a shortcut exit at first difference is faster or slower, depending on how clever the hardware people made branch prediction and pipelining. Maybe we should use stdd:memcmp and have the libc deal with that? Fun fact s390x of course has a CPU instruction for that :D https://github.com/torvalds/linux/blob/8f3d9f354286745c751374f5f1fcafee6b3f3136/arch/s390/lib/string.c#L343

memcmp is not the solution, because it immediately breaks the code as soon as Ids are not plain ints and have a custom comparison.

return _cols == other._cols && std::equal(_data, _data + _cols, other._data);

(std::equal should become std::memcmp for builtin types.)
Also this fixes the bug, that in case the two sizes don't match (which should never happen in calling code), there is an out of bounds read.

niklas88 · 2020-04-13T20:39:44Z

src/engine/IdTable.h

+
+  const value_type* operator->() const { return &_rowView; }
+
+  // access the element the is i steps ahead


niklas88 · 2020-04-13T20:49:37Z

src/engine/IdTable.h

+  template <int NEW_COLS, typename = std::enable_if_t<DATA::ManagesStorage>>
+  IdTableTemplated<NEW_COLS, DATA> moveToStatic() {
+    return IdTableTemplated<NEW_COLS, DATA>(
+        std::move(*this));  // let the private conversions do all the work
  };

  /**
   * @brief Creates an IdTable that now owns this
   * id tables data. This is effectively a move operation that also
   * changes the type to an equibalent one.


niklas88 · 2020-08-15T10:46:50Z

What's the status of this?

niklas88 · 2020-11-28T22:39:31Z

@joka921 @floriankramer ping for my last question

joka921 · 2020-11-29T09:08:13Z

I just fixed all the small nitpicks from your last review and fixed the conflict.
So I think this is now ready to roll.

joka921 requested review from floriankramer and niklas88 March 10, 2020 12:27

niklas88 reviewed Apr 12, 2020

View reviewed changes

joka921 force-pushed the f.SerializeResults branch from 87232e3 to 2fb6a45 Compare April 13, 2020 16:06

Refactored the IdTable to use a std::vector as its data storage.

29b1579

- Removed a lot of code and implemented const_iterators and read-only views.

joka921 force-pushed the f.SerializeResults branch from 2fb6a45 to 29b1579 Compare April 13, 2020 16:20

niklas88 reviewed Apr 13, 2020

View reviewed changes

joka921 added 3 commits November 29, 2020 09:54

small changes from Niklas' review.

8946135

Reset abseil to the master version, the change was unintended.

37cef83

Clang format

6dcd2fd

niklas88 merged commit 93db17f into ad-freiburg:master Nov 29, 2020

joka921 deleted the f.SerializeResults branch May 8, 2021 09:13

RobinTF mentioned this pull request Jan 25, 2022

Remove unused non-compiling function #551

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactored the IdTable class to make interfaces simpler + more correct #320

Refactored the IdTable class to make interfaces simpler + more correct #320

joka921 commented Mar 10, 2020

niklas88 Apr 12, 2020

niklas88 left a comment

niklas88 Apr 13, 2020

niklas88 Apr 13, 2020

niklas88 Apr 13, 2020

niklas88 Apr 13, 2020

joka921 Nov 29, 2020

niklas88 Apr 13, 2020

joka921 Nov 29, 2020

niklas88 Apr 13, 2020

niklas88 Apr 13, 2020

niklas88 commented Aug 15, 2020

niklas88 commented Nov 28, 2020

joka921 commented Nov 29, 2020


		const value_type* operator->() const { return &_rowView; }

		// access the element the is i steps ahead

Refactored the IdTable class to make interfaces simpler + more correct #320

Refactored the IdTable class to make interfaces simpler + more correct #320

Conversation

joka921 commented Mar 10, 2020

Choose a reason for hiding this comment

niklas88 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

Choose a reason for hiding this comment

niklas88 commented Aug 15, 2020

niklas88 commented Nov 28, 2020

joka921 commented Nov 29, 2020