Fix RAII issue by introducing wrapper classes for backend plans #208

yasahi-hpc · 2024-12-03T14:56:36Z

Resolves #197

In this PR, we try to resolve RAII issues by introducing a thin wrapper for backend fft plans.
Following modifications are made,

Introducing a KokkosFFT_FFTW_Types.hpp file including the FFTW plan wrapper class and common code bases related fftw
Introducing a wrapper class for automatic deallocation. Wrappers are added for FFTW, cufft, hipfft, and rocfft (Scoped***Plan)
For CUDA, we have

struct ScopedCufftPlanType {
   cufftHandle m_plan;
   // Should we raise an error here, if it fails?
   ScopedCufftPlanType() { cufftCreate(&m_plan); }
   
   // Should we abort here, if it fails?
   ~ScopedCufftPlanType() { cufftDestroy(m_plan); }

   cufftHandle &plan() { return m_plan; }
};

Info and Buffer that are required only for rocfft are moved inside rocfft helper class
Add missing cleanup_threads() in the destructor of FFTW helper class
Remove destroy_plan_and_info since it is automatically done in destructors of helper classes

Questions

We are creating plans inside a constructor for cufft and hipfft. Should we raise assertions in constructor if it fails?
What should we do if destroy functions in destructor fails. Should we abort?

tpadioleau · 2024-12-05T10:47:48Z

I would go a step further and make these RAII classes unique-owners. That implies to disallow shallow copy either by simply deleting copy constructor and assignment or make them allocate and copy a new plan if backends offer such a copy function. We can keep the move semantics but we have to keep track if the current object must call cufftDestroy in the destructor. If we delete both the copy and move semantics that gives:

class ScopedCufftPlanType {
 private:
  cufftHandle m_plan;

 public:
  ScopedCufftPlanType() {
    cufftResult cufft_rt = cufftCreate(&m_plan);
    KOKKOSFFT_THROW_IF(cufft_rt != CUFFT_SUCCESS, "cufftCreate failed");
  }

  ScopedCufftPlanType(ScopedCufftPlanType const& rhs) = delete;

  ScopedCufftPlanType(ScopedCufftPlanType&& rhs) = delete;

  ~ScopedCufftPlanType() noexcept {
    cufftResult cufft_rt = cufftDestroy(m_plan);
    if (cufft_rt != CUFFT_SUCCESS) Kokkos::abort("cufftDestroy failed");
  }

  ScopedCufftPlanType& operator=(ScopedCufftPlanType const& rhs) = delete;

  ScopedCufftPlanType& operator=(ScopedCufftPlanType&& rhs) = delete;

  cufftHandle plan() const noexcept { return m_plan; }
};

The ScopedCufftPlanType::plan can return the backend handle by copy for CUDA, I don't know for the other backends.

I notice that we could also improve the error messages, I see that cufftCreate can return different error types https://docs.nvidia.com/cuda/cufft/index.html?highlight=cufftMakePlan1d#c.cufftCreate.

yasahi-hpc · 2024-12-05T11:11:04Z

I would go a step further and make these RAII classes unique-owners. That implies to disallow shallow copy either by simply deleting copy constructor and assignment or make them allocate and copy a new plan if backends offer such a copy function. We can keep the move semantics but we have to keep track if the current object must call cufftDestroy in the destructor.

I would prefer to disallow both copy and move, because it may be unsafe for some backends.

If we delete both the copy and move semantics that gives:

class ScopedCufftPlanType {
 private:
  cufftHandle m_plan;

 public:
  ScopedCufftPlanType() {
    cufftResult cufft_rt = cufftCreate(&m_plan);
    KOKKOSFFT_THROW_IF(cufft_rt != CUFFT_SUCCESS, "cufftCreate failed");
  }

  ScopedCufftPlanType(ScopedCufftPlanType const& rhs) = delete;

  ScopedCufftPlanType(ScopedCufftPlanType&& rhs) = delete;

  ~ScopedCufftPlanType() noexcept {
    cufftResult cufft_rt = cufftDestroy(m_plan);
    if (cufft_rt != CUFFT_SUCCESS) Kokkos::abort("cufftDestroy failed");
  }

  ScopedCufftPlanType& operator=(ScopedCufftPlanType const& rhs) = delete;

  ScopedCufftPlanType& operator=(ScopedCufftPlanType&& rhs) = delete;

  cufftHandle plan() const noexcept { return m_plan; }
};

The ScopedCufftPlanType::plan can return the backend handle by copy for CUDA, I don't know for the other backends.

Does it return a shallow copy of the cufftHandle? I could not find the details of copy constructor of cufftHandle. That should work for oneMKL backend, but I am not sure about the behavior of other backends

I notice that we could also improve the error messages, I see that cufftCreate can return different error types https://docs.nvidia.com/cuda/cufft/index.html?highlight=cufftMakePlan1d#c.cufftCreate.

That is true. We can improve the error messages

tpadioleau · 2024-12-05T13:57:55Z

I would prefer to disallow both copy and move, because it may be unsafe for some backends.

Ok for me. I don't think there should be any problem with implementing the move semantics later. It requires us to only pass these objects by reference.

Does it return a shallow copy of the cufftHandle? I could not find the details of copy constructor of cufftHandle. That should work for oneMKL backend, but I am not sure about the behavior of other backends

I mean if we do

ScopedCufftPlanType plan1;
ScopedCufftPlanType plan2;
plan2 = plan1;

This compiles fine and plan2 will get the same cufftHandle than plan1. The plan2 will destroy correctly the handle but then plan1 will likely fail to do so.

yasahi-hpc · 2024-12-05T14:09:41Z

I would prefer to disallow both copy and move, because it may be unsafe for some backends.

Ok for me. I don't think there should be any problem with implementing the move semantics later. It requires us to only pass these objects by reference.

Does it return a shallow copy of the cufftHandle? I could not find the details of copy constructor of cufftHandle. That should work for oneMKL backend, but I am not sure about the behavior of other backends

I mean if we do
ScopedCufftPlanType plan1;
ScopedCufftPlanType plan2;
plan2 = plan1;
This compiles fine and plan2 will get the same cufftHandle than plan1. The plan2 will destroy correctly the handle but then plan1 will likely fail to do so.

That is right. This does not work in the current implementation. Yes, for safety, it is better to disallow copy

fft/src/KokkosFFT_Cuda_types.hpp

fft/src/KokkosFFT_Cuda_transform.hpp

fft/src/KokkosFFT_Cuda_types.hpp

fft/src/KokkosFFT_FFTW_Types.hpp

fft/src/KokkosFFT_ROCM_types.hpp

tpadioleau · 2024-12-17T08:42:37Z

fft/src/KokkosFFT_ROCM_types.hpp

+  void allocate_work_buffer(std::size_t workbuffersize) {
+    m_buffer = BufferViewType("workbuffer", workbuffersize);
+  }
+  rocfft_plan &plan() { return m_plan; }


Suggested change

rocfft_plan &plan() { return m_plan; }

rocfft_plan plan() const noexcept { return m_plan; }

tpadioleau · 2024-12-17T08:51:30Z

fft/src/KokkosFFT_ROCM_plans.hpp

+                  [](rocfft_plan_description* desc) {
+                    rocfft_plan_description_destroy(*desc);
+                  });


Suggested change

[](rocfft_plan_description* desc) {

rocfft_plan_description_destroy(*desc);

});

[](rocfft_plan_description* desc) {

rocfft_plan_description_destroy(*desc);

delete desc;

});

I introduced a wrapper for description as well for RAII. This part has been removed

tpadioleau · 2024-12-17T08:54:51Z

fft/src/KokkosFFT_ROCM_plans.hpp

+                  [](rocfft_plan_description* desc) {
+                    rocfft_plan_description_destroy(*desc);
+                  });
+  rocfft_status status = rocfft_plan_description_create(&(*description));


I think we can improve the situation by also wrapping rocfft_plan_description.

In case rocfft_plan_description_create fails, the next KOKKOSFFT_THROW_IF will throw, calling the destructor of description thus calling rocfft_plan_description_destroy even though the rocfft_plan_description object has not been initialised.

I have introduced a wrapper accordingly

tpadioleau · 2024-12-17T08:58:40Z

fft/src/KokkosFFT_ROCM_plans.hpp

  plan   = std::make_unique<PlanType>();
-  status = rocfft_plan_create(&(*plan), place, fft_direction, precision,
+  status = rocfft_plan_create(&(plan->plan()), place, fft_direction, precision,
                              reversed_fft_extents.size(),  // Dimension
                              reversed_fft_extents.data(),  // Lengths
-                              howmany,     // Number of transforms
-                              description  // Description
+                              howmany,      // Number of transforms
+                              *description  // Description


Why not making this part of the constructor of ScopedRocfftPlan ?

I moved the logic into the constructor

yasahi-hpc

@tpadioleau
Thank you for reviews. I have fixed accordingly

yasahi-hpc · 2024-12-18T02:04:29Z

fft/src/KokkosFFT_FFTW_Types.hpp

+    if constexpr (std::is_same_v<plan_type, fftwf_plan>) {
+      if (m_is_created) fftwf_destroy_plan(m_plan);
+    } else {
+      if (m_is_created) fftw_destroy_plan(m_plan);
+    }


I may expect that fftw_plan_many_dft may fail if we request too much memory

fft/src/KokkosFFT_Cuda_types.hpp

fft/src/KokkosFFT_Cuda_transform.hpp

fft/src/KokkosFFT_Cuda_types.hpp

fft/src/KokkosFFT_ROCM_types.hpp

yasahi-hpc · 2024-12-18T02:22:44Z

fft/src/KokkosFFT_ROCM_types.hpp

+  void allocate_work_buffer(std::size_t workbuffersize) {
+    m_buffer = BufferViewType("workbuffer", workbuffersize);
+  }
+  rocfft_plan &plan() { return m_plan; }


yasahi-hpc · 2024-12-18T02:23:22Z

fft/src/KokkosFFT_ROCM_plans.hpp

+                  [](rocfft_plan_description* desc) {
+                    rocfft_plan_description_destroy(*desc);
+                  });


I introduced a wrapper for description as well for RAII. This part has been removed

yasahi-hpc · 2024-12-18T02:23:58Z

fft/src/KokkosFFT_ROCM_plans.hpp

+                  [](rocfft_plan_description* desc) {
+                    rocfft_plan_description_destroy(*desc);
+                  });
+  rocfft_status status = rocfft_plan_description_create(&(*description));


I have introduced a wrapper accordingly

yasahi-hpc · 2024-12-18T02:24:22Z

fft/src/KokkosFFT_ROCM_plans.hpp

  plan   = std::make_unique<PlanType>();
-  status = rocfft_plan_create(&(*plan), place, fft_direction, precision,
+  status = rocfft_plan_create(&(plan->plan()), place, fft_direction, precision,
                              reversed_fft_extents.size(),  // Dimension
                              reversed_fft_extents.data(),  // Lengths
-                              howmany,     // Number of transforms
-                              description  // Description
+                              howmany,      // Number of transforms
+                              *description  // Description


I moved the logic into the constructor

yasahi-hpc self-assigned this Dec 3, 2024

yasahi-hpc added bug Something isn't working enhancement New feature or request cleanup labels Dec 3, 2024

yasahi-hpc force-pushed the fix-RAII-issue branch from e302632 to 05a023b Compare December 3, 2024 14:59

yasahi-hpc changed the title ~~Fix raii issue~~ Fix RAII issue by introducing wrapper classes for backend plans Dec 3, 2024

yasahi-hpc marked this pull request as draft December 3, 2024 15:04

yasahi-hpc marked this pull request as ready for review December 4, 2024 19:30

yasahi-hpc mentioned this pull request Dec 5, 2024

Remove unnecessary cufftCreate and hipfftCreate to avoid creating plans twice #212

Merged

yasahi-hpc force-pushed the fix-RAII-issue branch from e510c5e to 46a3630 Compare December 5, 2024 15:22

tpadioleau reviewed Dec 17, 2024

View reviewed changes

Yuuichi Asahi added 15 commits December 17, 2024 22:40

fix: conflicts

8fb15ac

Fix and wrapper for FFTW handle

737f3ca

Wrapper for cufft handle

95419cb

fix: conflicts

9baa59d

Wrapper for rocfft handle

8d952e5

fix: conflicts

dd907a8

Cleanup plan class based on the introduction of wrappers

6b1059b

fix: conflicts

f46c56d

fix: conflicts

5116c1c

fix: unused variable

b93c07c

fix: work buffer allocation

1b908fe

remove unused variable

4010401

remove unused lines

ac5b34d

Add missing include header file in KokkosFFT_ROCM_types.hpp

734a55a

fix: fftwHandle type in SYCL types

f161d46

Yuuichi Asahi added 20 commits December 17, 2024 22:40

Do not return const plan type for fftw

31cd597

fix: remove const

2030d04

fix: fftw plan creation

a8741bd

fix: set created

a3c94e3

fix: cleanup

0d8a616

fix constructor of fftw wrapper

f1f4f30

fix: conflicts

2b75678

Remove non-default constructors from FFTW wrapper

88310c9

Remove non-default constructors from cufft wrapper

995bb4a

Remove non-default constructors from hipfft wrapper

751810c

Remove non-default constructors from rocfft wrapper

4ba04f0

update FFTW wrapper class name

2eac65c

fix: host plan type

35afb3d

fix: fftw rapper name in ROCM_types

5c25bce

update cuda backed based on reviews

c363d3d

update hip backend based on reviews

975b2f1

update rocm backend based on reviews

405b36f

update host backend based on revies

d394672

fix: Rocm types

7c085eb

fix: ROCM types

a6ccd56

yasahi-hpc force-pushed the fix-RAII-issue branch from 7a6946c to a6ccd56 Compare December 17, 2024 13:43

Yuuichi Asahi added 6 commits December 17, 2024 22:59

fix: Rocm types

701136e

fix: header files

622ac0e

fix: rocm types

1d33d7a

fix: rocm types

34ed2b3

remove unused lines

45c489b

fix: rocm types

3fdd5bf

yasahi-hpc commented Dec 18, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix RAII issue by introducing wrapper classes for backend plans #208

Fix RAII issue by introducing wrapper classes for backend plans #208

yasahi-hpc commented Dec 3, 2024 •

edited

Loading

tpadioleau commented Dec 5, 2024

yasahi-hpc commented Dec 5, 2024

tpadioleau commented Dec 5, 2024

yasahi-hpc commented Dec 5, 2024

tpadioleau Dec 17, 2024

yasahi-hpc Dec 18, 2024

tpadioleau Dec 17, 2024

yasahi-hpc Dec 18, 2024

tpadioleau Dec 17, 2024

yasahi-hpc Dec 18, 2024

tpadioleau Dec 17, 2024

yasahi-hpc Dec 18, 2024

yasahi-hpc left a comment

yasahi-hpc Dec 18, 2024

yasahi-hpc Dec 18, 2024

yasahi-hpc Dec 18, 2024

yasahi-hpc Dec 18, 2024

yasahi-hpc Dec 18, 2024

	rocfft_plan &plan() { return m_plan; }
	rocfft_plan plan() const noexcept { return m_plan; }

Fix RAII issue by introducing wrapper classes for backend plans #208

Are you sure you want to change the base?

Fix RAII issue by introducing wrapper classes for backend plans #208

Conversation

yasahi-hpc commented Dec 3, 2024 • edited Loading

tpadioleau commented Dec 5, 2024

yasahi-hpc commented Dec 5, 2024

tpadioleau commented Dec 5, 2024

yasahi-hpc commented Dec 5, 2024

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

yasahi-hpc 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

yasahi-hpc commented Dec 3, 2024 •

edited

Loading