Remove Scratchpad from KernelManager #3678

mzient · 2022-02-15T11:21:24Z

A follow-up for #3670

Category:

Refactoring (Redesign of existing code that doesn't affect functionality)

Description:

After dropping the use of preallocated scratchpad in KernelManager, it is quite pointless to keep ScratchpadAllocator instances in it - also, passing thread_idx to Run is no longer necessary, since it was only used for indexing scratchpads. As such, this PR removes those obsolete pieces.

Additional information:

Affected modules and functionalities:

Key points relevant for the review:

Checklist

Tests

Documentation

DALI team only

Requirements

Implements new requirements
Affects existing requirements
N/A

REQ IDs: N/A

JIRA TASK: DALI-2449

JanuszL · 2022-02-15T14:11:20Z

dali/kernels/kernel_manager.h

+      auto *old_scratchpad = context.scratchpad;
+      context.scratchpad = &scratchpad;
+      inst.get<Kernel>().Run(context, std::forward<OutInArgs>(out_in_args)...);
+      context.scratchpad = old_scratchpad;


Suggested change

auto *old_scratchpad = context.scratchpad;

context.scratchpad = &scratchpad;

inst.get<Kernel>().Run(context, std::forward<OutInArgs>(out_in_args)...);

context.scratchpad = old_scratchpad;

context.scratchpad = &scratchpad;

inst.get<Kernel>().Run(context, std::forward<OutInArgs>(out_in_args)...);

context.scratchpad = nullptr;

…ead_idx from KernelManager API. Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>

…rnelManager::Run exits, even if the kernel throws. Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>

mzient · 2022-02-15T17:41:44Z

dali/core/mm/default_resources.cc

-struct CallAtExit {
-  explicit CallAtExit(Callable &&c) : callable(std::move(c)) {}
-  ~CallAtExit() {
-    callable();
-  }
-  Callable callable;
-};
-
-template <typename Callable>
-CallAtExit<Callable> AtExit(Callable &&c) {
-  return CallAtExit<Callable>(std::forward<Callable>(c));
-}


This has been moved to a separate header.

mzient · 2022-02-15T17:42:02Z

include/dali/core/call_at_exit.h

+template <typename Callable>
+struct CallAtExit {
+  explicit CallAtExit(Callable &&c) : callable(std::move(c)) {}
+  ~CallAtExit() noexcept(false) {
+    callable();
+  }
+  Callable callable;
+};
+
+}  // namespace detail
+
+template <typename Callable>
+detail::CallAtExit<Callable> AtBlockExit(Callable &&c) {
+  return detail::CallAtExit<Callable>(std::forward<Callable>(c));
+}


Copied from default_resources.cc

JanuszL · 2022-02-15T18:26:27Z

dali/operators/signal/fft/spectrogram.cc

@@ -204,8 +204,7 @@ bool SpectrogramImplCpu<time_major>::SetupImpl(std::vector<OutputDesc> &out_desc
    if (!w.raw_data()) w.set_pinned(false);
  }

-  kmgr_window_.Initialize<WindowKernel>();


This call was unnecessary even before this PR, right?

Yes, it's done implicitly in Resize

dali-automaton · 2022-02-16T09:05:20Z

CI MESSAGE: [3975335]: BUILD STARTED

dali-automaton · 2022-02-16T10:33:46Z

CI MESSAGE: [3975335]: BUILD PASSED

jantonguirao

LGTM, with some minor comments

jantonguirao · 2022-02-16T10:28:13Z

dali/kernels/imgproc/jpeg/jpeg_distortion_gpu_test.cu

    kmgr_.Initialize<Kernel>();
-    kmgr_.Resize<Kernel>(1, 1);
+    kmgr_.Resize<Kernel>(1);


Remove Initialize?

jantonguirao · 2022-02-16T10:46:39Z

dali/kernels/kernel_manager.h

+      inst.get<Kernel>().Run(context, std::forward<OutInArgs>(out_in_args)...);
+    } else {
+      inst.get<Kernel>().Run(context, std::forward<OutInArgs>(out_in_args)...);


Suggested change

inst.get<Kernel>().Run(context, std::forward<OutInArgs>(out_in_args)...);

} else {

inst.get<Kernel>().Run(context, std::forward<OutInArgs>(out_in_args)...);

}

inst.get<Kernel>().Run(context, std::forward<OutInArgs>(out_in_args)...);

how about that?

This finally would go out of scope too early.

jantonguirao · 2022-02-16T10:47:42Z

dali/kernels/signal/dct/dct_gpu_test.cc

@@ -250,7 +249,7 @@ TEST_P(Dct1DGpuPerfTest, DISABLED_PerfTest) {
  ctx.gpu.stream = 0;
  KernelManager kmgr;
  kmgr.Initialize<Kernel>();
-  kmgr.Resize<Kernel>(1, 1);
+  kmgr.Resize<Kernel>(1);


remove initialize?

jantonguirao · 2022-02-16T10:48:19Z

dali/operators/audio/mel_scale/mel_filter_bank.cc

@@ -81,7 +80,7 @@ bool MelFilterBank<CPUBackend>::SetupImpl(std::vector<OutputDesc> &output_desc,
  TYPE_SWITCH(input.type(), type2id, T, MEL_FBANK_SUPPORTED_TYPES, (
    using MelFilterBankKernel = kernels::audio::MelFilterBankCpu<T>;
    kmgr_.Initialize<MelFilterBankKernel>();
-    kmgr_.Resize<MelFilterBankKernel>(nthreads, nsamples);
+    kmgr_.Resize<MelFilterBankKernel>(nsamples);


remove initialize

jantonguirao · 2022-02-16T10:48:48Z

dali/operators/audio/mfcc/mfcc.cu

@@ -52,7 +52,7 @@ std::vector<OutputDesc> SetupKernel(kernels::KernelManager &kmgr, kernels::Kerne
                                    span<const MFCC<GPUBackend>::DctArgs> args, int axis) {
  using Kernel = kernels::signal::dct::Dct1DGpu<T>;
  kmgr.Initialize<Kernel>();
-  kmgr.Resize<Kernel>(1, 1);
+  kmgr.Resize<Kernel>(1);


remove initialize

jantonguirao · 2022-02-16T11:01:56Z

dali/operators/image/distortion/jpeg_compression_distortion_op_gpu.cu

@@ -24,7 +24,7 @@ class JpegCompressionDistortionGPU : public JpegCompressionDistortion<GPUBackend
 public:
  explicit JpegCompressionDistortionGPU(const OpSpec &spec) : JpegCompressionDistortion(spec) {
    kmgr_.Initialize<JpegDistortionKernel>();
-    kmgr_.Resize<JpegDistortionKernel>(1, 1);
+    kmgr_.Resize<JpegDistortionKernel>(1);


remove initialize

jantonguirao · 2022-02-16T11:10:39Z

dali/operators/signal/decibel/to_decibels_op_cpu.cc


  TYPE_SWITCH(input.type(), type2id, T, (float), (
    using ToDbKernel = kernels::signal::ToDecibelsCpu<T>;
    kmgr_.Initialize<ToDbKernel>();
-    kmgr_.Resize<ToDbKernel>(nthreads, nsamples);
+    kmgr_.Resize<ToDbKernel>(nsamples);


remove initialize

jantonguirao · 2022-02-16T11:10:55Z

dali/operators/signal/fft/power_spectrum.cc


  // Other types not supported for now
  using InputType = float;
  using OutputType = float;
  VALUE_SWITCH(in_shape.sample_dim(), Dims, FFT_SUPPORTED_NDIMS, (
    using FftKernel = kernels::signal::fft::Fft1DCpu<OutputType, InputType, Dims>;
    kmgr_.Initialize<FftKernel>();
-    kmgr_.Resize<FftKernel>(nthreads, nsamples);
+    kmgr_.Resize<FftKernel>(nsamples);


remove initialize

jantonguirao · 2022-02-16T11:11:19Z

dali/operators/signal/fft/spectrogram.cc

@@ -204,8 +204,7 @@ bool SpectrogramImplCpu<time_major>::SetupImpl(std::vector<OutputDesc> &out_desc
    if (!w.raw_data()) w.set_pinned(false);
  }

-  kmgr_window_.Initialize<WindowKernel>();


Yes, it's done implicitly in Resize

jantonguirao · 2022-02-16T11:11:52Z

include/dali/core/call_at_exit.h

+}  // namespace detail
+
+template <typename Callable>
+detail::CallAtExit<Callable> AtBlockExit(Callable &&c) {


Suggested change

detail::CallAtExit<Callable> AtBlockExit(Callable &&c) {

detail::CallAtExit<Callable> AtScopeExit(Callable &&c) {

would be more understandable, I think

Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>

dali-automaton · 2022-02-16T12:18:08Z

CI MESSAGE: [3976332]: BUILD STARTED

dali-automaton · 2022-02-16T13:38:37Z

CI MESSAGE: [3976332]: BUILD PASSED

* Remove ScratchpadAllocator from KernelManager. * Remove num_threads/thread_idx from KernelManager API. * Move CallAtExit to a separate header; rename AtExit to AtScopeExit. * Remove redundant calls to KernelManager::Initialize. Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>

mzient assigned mzient and unassigned mzient Feb 15, 2022

JanuszL self-assigned this Feb 15, 2022

JanuszL reviewed Feb 15, 2022

View reviewed changes

mzient force-pushed the KernelManagerRemoveScratchpad branch from cd37436 to 12e4ab6 Compare February 15, 2022 14:49

Remove ScratchpadAllocator from KernelManager. Remove num_threads/thr…

f028d50

…ead_idx from KernelManager API. Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>

mzient force-pushed the KernelManagerRemoveScratchpad branch from 12e4ab6 to f028d50 Compare February 15, 2022 17:07

Move CallAtExit to a header. Reset scratcphad pointer to null when Ke…

94e73cc

…rnelManager::Run exits, even if the kernel throws. Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>

mzient commented Feb 15, 2022

View reviewed changes

JanuszL reviewed Feb 15, 2022

View reviewed changes

JanuszL approved these changes Feb 15, 2022

View reviewed changes

jantonguirao self-assigned this Feb 16, 2022

jantonguirao approved these changes Feb 16, 2022

View reviewed changes

Rename AtBlockExit to AtScopeExit. Remove redundant kmgr.Initialize.

794c8ee

Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>

mzient merged commit cb3c1e5 into NVIDIA:main Feb 16, 2022

JanuszL mentioned this pull request Mar 30, 2022

DALI 2022 roadmap #3774

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove Scratchpad from KernelManager #3678

Remove Scratchpad from KernelManager #3678

mzient commented Feb 15, 2022 •

edited

Loading

JanuszL Feb 15, 2022

mzient Feb 15, 2022

mzient Feb 15, 2022

mzient Feb 15, 2022

JanuszL Feb 15, 2022

jantonguirao Feb 16, 2022

dali-automaton commented Feb 16, 2022

dali-automaton commented Feb 16, 2022

jantonguirao left a comment

jantonguirao Feb 16, 2022

jantonguirao Feb 16, 2022

mzient Feb 16, 2022

jantonguirao Feb 16, 2022

jantonguirao Feb 16, 2022

jantonguirao Feb 16, 2022

jantonguirao Feb 16, 2022

jantonguirao Feb 16, 2022

jantonguirao Feb 16, 2022

jantonguirao Feb 16, 2022

jantonguirao Feb 16, 2022

dali-automaton commented Feb 16, 2022

dali-automaton commented Feb 16, 2022

	detail::CallAtExit<Callable> AtBlockExit(Callable &&c) {
	detail::CallAtExit<Callable> AtScopeExit(Callable &&c) {

Remove Scratchpad from KernelManager #3678

Remove Scratchpad from KernelManager #3678

Conversation

mzient commented Feb 15, 2022 • edited Loading

Category:

Description:

Additional information:

Affected modules and functionalities:

Key points relevant for the review:

Checklist

Tests

Documentation

DALI team only

Requirements

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

dali-automaton commented Feb 16, 2022

dali-automaton commented Feb 16, 2022

jantonguirao 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dali-automaton commented Feb 16, 2022

dali-automaton commented Feb 16, 2022

mzient commented Feb 15, 2022 •

edited

Loading