Encapsulate Storage_base class #528

IvanaGyro · 2024-12-01T18:43:18Z

The first two commits are the same as #525. Please start to review from the third commit.
There are too many files changed to review. I leave comments on the changed lines which are not trivially renaming the variables. You can just review those parts.

With the current class definition, developers are able to change the variable storing the size of the storage without allocating the underlying storage, which potentially causes memory leaks.

To ensure there will be no surprising behavior caused by directly accessing the variables of Storage_base, all variables of Stroage_base are moved to its subclass, StorageImplementation, and are set as private. All functions outside of StorageImplementation should access the value of the variable via the getters.

The next step in refactoring the components related to Storage is to remove the Storage_base class.

Test result

This PR breaks no tests with CUDA, cuQuantum, cuTENSOR, MKL, CUTT, and HPTT enabled.

codecov · 2024-12-01T19:27:43Z

Codecov Report

Attention: Patch coverage is 14.52282% with 2678 lines in your changes missing coverage. Please review.

Project coverage is 17.71%. Comparing base (672b7e9) to head (09b2c37).
Report is 1 commits behind head on dev-master.

Files with missing lines	Patch %	Lines
src/backend/linalg_internal_cpu/Sub_internal.cpp	9.91%	327 Missing ⚠️
src/backend/linalg_internal_cpu/Div_internal.cpp	11.57%	321 Missing ⚠️
src/backend/linalg_internal_cpu/Mod_internal.cpp	1.23%	240 Missing ⚠️
src/backend/linalg_internal_cpu/Cpr_internal.cpp	0.00%	198 Missing ⚠️
src/backend/linalg_internal_cpu/iAdd_internal.cpp	10.67%	184 Missing ⚠️
src/backend/linalg_internal_cpu/iMul_internal.cpp	10.67%	184 Missing ⚠️
src/backend/linalg_internal_cpu/iSub_internal.cpp	11.65%	182 Missing ⚠️
src/backend/linalg_internal_cpu/iDiv_internal.cpp	12.62%	180 Missing ⚠️
src/backend/linalg_internal_cpu/Add_internal.cpp	18.18%	162 Missing ⚠️
src/backend/linalg_internal_cpu/Mul_internal.cpp	21.21%	156 Missing ⚠️
... and 33 more

Additional details and impacted files

@@              Coverage Diff               @@
##           dev-master     #528      +/-   ##
==============================================
+ Coverage       17.58%   17.71%   +0.12%     
==============================================
  Files             211      211              
  Lines           44733    44748      +15     
  Branches        14941    13920    -1021     
==============================================
+ Hits             7868     7928      +60     
- Misses          32751    32774      +23     
+ Partials         4114     4046      -68

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

IvanaGyro · 2024-12-02T05:30:52Z

include/backend/Scalar.hpp

@@ -392,6 +392,9 @@ namespace cytnx {
      return nullptr;
    }

+    /**
+     * @deprecated This method is not in use anymore.
+     */
    virtual void *get_raw_address() const {
      cytnx_error_msg(true, "[ERROR] Void Type Scalar cannot have operation!!%s", "\n");
      return nullptr;


Mark this function as deprecated. It may be deleted or renamed to data() like std convention.

IvanaGyro · 2024-12-02T05:34:44Z

include/backend/Storage.hpp

+      cytnx_error_msg(true, "Not implemented.%s", "");
+    }
+    virtual const unsigned long long size() const {
+      cytnx_error_msg(true, "Not implemented.%s", "");


No need to return a const reference for the ScalarType.

IvanaGyro · 2024-12-02T05:38:19Z

include/backend/Storage.hpp

+    virtual const unsigned long long size() const {
+      cytnx_error_msg(true, "Not implemented.%s", "");
+    }
+    virtual ~Storage_base();


Base classes must have a virtual deconstructor. Without virtual, the deconstructor will not be called when deleting the instance of the derived class held by the pointer of the base classes.

IvanaGyro · 2024-12-02T05:39:05Z

include/backend/Storage.hpp

+    // throw a runtime error here.
+    virtual void *data() const { return nullptr; }
+    virtual int dtype() const { return Type.Void; }
+    virtual int device() const { return Device.cpu; }


New getters due to the variables of Storage_base are moved to the derived class.

IvanaGyro · 2024-12-02T05:40:39Z

include/backend/Storage.hpp

+      start_ = nullptr;
+      size_ = 0;
+      capacity_ = 0;
+      return original_start;


This should only be used in the functions converting Tensor and Storage to numpy arrays.

IvanaGyro · 2024-12-02T05:41:34Z

include/backend/Storage.hpp

+    unsigned long long size_;
+    unsigned long long capacity_;
+    unsigned int dtype_;
+    int device_;


Variables moved from Storage_base class to its derived class, StorageImplementation<DType>.

IvanaGyro · 2024-12-02T05:45:39Z

pybind/storage_py.cpp

-           // delegate numpy array with it's ptr, and swap a auxiliary ptr for intrusive_ptr to
-           // free.
-           void *pswap = malloc(sizeof(bool));
-           tmpIN._impl->Mem = pswap;


Variables should be private. release() is designed to provide the same functionality.

IvanaGyro · 2024-12-02T05:46:20Z

pybind/tensor_py.cpp

-        // free.
-        if (share_mem == false) {
-          void *pswap = malloc(sizeof(bool));
-          tmpIN.storage()._impl->Mem = pswap;


Do the similar change as the change for Storage.

IvanaGyro · 2024-12-02T05:53:40Z

src/linalg/Add.cpp

+      Storage Cnst(1, lc.dtype());
+      Cnst.set_item(0, lc);



To prevent direct access to the variables defined in a class, we should use the public interface of the class. Copying a scalar is cheap almost like assigning a pointer, so we don't have to worry about the cost here.

I also did the same job in Cpr.cpp, Div.cpp, Mod.cpp, Mul.cpp, and Sub.cpp.

IvanaGyro · 2024-12-02T06:00:24Z

src/backend/Storage.cpp

-    f.write((char *)&this->size(), sizeof(unsigned long long));
-    f.write((char *)&this->dtype(), sizeof(unsigned int));
-    f.write((char *)&this->device(), sizeof(int));
+    auto write_number = [&f](auto number) {
+      f.write(reinterpret_cast<char *>(&number), sizeof(number));
+    };
+    write_number(this->size());
+    write_number(this->dtype());
+    write_number(this->device());


this->size() and the other statements are not lvalue now, so we cannot access the address by &.

manuschneider · 2024-12-02T12:55:42Z

src/Tensor.cpp

Where are these functions used?

Actually, they are not used now. Their declarations were introduced at #514 and their definitions (the code you see here) were merged at #525. The change of this section of code is only to change _impl->Mem to _impl->data().

`Storage::release()` serves for the function converting Storage and Tensor to the numpy array. The reason for implementing this method is that we are encapsulating Storage and the relative classes, which mean we don't want other classes or functions to directly access the member variables. This is just a temporary solution and will be removed when we use std::vector as the underlying storage because it is impossible to make std::vector drops its ownership of the underlying storage.

The size of Scalar is small, so it's cheap for creating a temporary Storage containing only a copy of the value of Scalar. This change is a step toward encapsulating the classes related to Storage.

With the current class definition, developers are able to change the variable storing the size of the storage without allocating the underlying storage, which potentially causes memory leaks. To ensure there will be no surprising behavior caused by directly accessing the variables of Storage_base, all variables of Stroage_base are moved to its subclass, StorageImplementation<DType>, and are set as private. All functions outside of StorageImplementation<DType> should access the value of the variable via the getters. The next step in refactoring the components related to Storage is to remove the Storage_base class.

We can check whether HPTT is enabled from the compiler definitions in compile_commands.json.

IvanaGyro force-pushed the encapsulate-storage branch 2 times, most recently from 0dd1be9 to 97a2597 Compare December 1, 2024 19:12

IvanaGyro commented Dec 2, 2024

View reviewed changes

manuschneider reviewed Dec 2, 2024

View reviewed changes

IvanaGyro requested a review from manuschneider December 2, 2024 13:34

IvanaGyro added 2 commits December 2, 2024 21:48

Do not assign the value to the variable of Stroage

ad6610f

The size of Scalar is small, so it's cheap for creating a temporary Storage containing only a copy of the value of Scalar. This change is a step toward encapsulating the classes related to Storage.

IvanaGyro force-pushed the encapsulate-storage branch from 97a2597 to 36f65fd Compare December 2, 2024 13:51

IvanaGyro added 2 commits December 2, 2024 22:06

Remove unsed debug message

09b2c37

We can check whether HPTT is enabled from the compiler definitions in compile_commands.json.

IvanaGyro force-pushed the encapsulate-storage branch from 36f65fd to 09b2c37 Compare December 2, 2024 14:07

IvanaGyro requested a review from hunghaoti December 3, 2024 15:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Encapsulate Storage_base class #528

Encapsulate Storage_base class #528

IvanaGyro commented Dec 1, 2024 •

edited

Loading

codecov bot commented Dec 1, 2024 •

edited

Loading

IvanaGyro Dec 2, 2024 •

edited

Loading

IvanaGyro Dec 2, 2024

IvanaGyro Dec 2, 2024

IvanaGyro Dec 2, 2024

IvanaGyro Dec 2, 2024

IvanaGyro Dec 2, 2024

IvanaGyro Dec 2, 2024

IvanaGyro Dec 2, 2024

IvanaGyro Dec 2, 2024

IvanaGyro Dec 2, 2024 •

edited

Loading

manuschneider Dec 2, 2024

IvanaGyro Dec 2, 2024 •

edited

Loading

Encapsulate Storage_base class #528

Are you sure you want to change the base?

Encapsulate Storage_base class #528

Conversation

IvanaGyro commented Dec 1, 2024 • edited Loading

Test result

codecov bot commented Dec 1, 2024 • edited Loading

Codecov Report

IvanaGyro Dec 2, 2024 • edited Loading

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

IvanaGyro Dec 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IvanaGyro Dec 2, 2024 • edited Loading

Choose a reason for hiding this comment

IvanaGyro commented Dec 1, 2024 •

edited

Loading

codecov bot commented Dec 1, 2024 •

edited

Loading

IvanaGyro Dec 2, 2024 •

edited

Loading

IvanaGyro Dec 2, 2024 •

edited

Loading

IvanaGyro Dec 2, 2024 •

edited

Loading