Dynamic & stream-aware scratchpad #3667

mzient · 2022-02-09T16:55:52Z

Category:

New feature (non-breaking change which adds functionality)

Description:

Dynamic scratchpad is an implementation of the Scratchpad interface which uses built-in monotonic resources instead of preallocated buffers.
Another important feature is that device memory can be allocated and deallocated in steam order. Pinned host memory can be deallocated in stream order, too, which is essential for safe fire-and-forget H2D copying.
To facilitate stream-ordered deallocation of upstream blocks in monotonic resources, an adapter called fixed_ordered_memory_resource is added, which executes all allocations and deallocations in a predefined order (stream or host).

Additional information:

Affected modules and functionalities:

Monotonic buffer received minor modifications.

Key points relevant for the review:

N/A

Checklist

Tests

Documentation

DALI team only

Requirements

Implements new requirements
Affects existing requirements
N/A

REQ IDs: N/A

JIRA TASK: DALI-2449

dali-automaton · 2022-02-09T17:02:38Z

CI MESSAGE: [3926424]: BUILD STARTED

dali/kernels/dynamic_scratchpad_test.cc

JanuszL · 2022-02-09T17:32:51Z

dali/kernels/dynamic_scratchpad_test.cc

+  std::vector<double> alloc_times[nkinds];
+  std::vector<double> destroy_times;
+  for (auto &v : alloc_times)
+    v.reserve(max_attempts*10);


Suggested change

v.reserve(max_attempts*10);

v.reserve(max_attempts*1024);

I think we do on average 1024 allocations per attempt.

I think now we do up to a 100. 1024 is the size, in bytes.

Yes, 100. I confused it with size_dist. So average would be like 50 and 100 max.

dali/kernels/dynamic_scratchpad_test.cc

JanuszL · 2022-02-09T17:37:54Z

dali/kernels/dynamic_scratchpad_test.cc

+
+    std::sort(v.begin(), v.end());
+    double sum = std::accumulate(v.begin(), v.end(), 0);
+    auto b98 = v.begin() + v.size()/100;
+    auto e98 = v.end() - v.size()/100;
+    double sum98 = std::accumulate(b98, e98, 0);
+    std::cout << "Allocation performance for " << names[k] << " memory.\n"
+              << "Median time:            " << v[v.size()/2] << " ns\n"
+              << "90th percentile:        " << v[v.size()*90/100] << " ns\n"
+              << "99th percentile:        " << v[v.size()*99/100] << " ns\n"
+              << "Mean time:              " << sum/v.size() << " ns\n"
+              << "Mean time (middle 98%): " << sum98/(e98-b98) << " ns\n";


I guess this could be a function.

JanuszL · 2022-02-09T17:44:54Z

dali/kernels/dynamic_scratchpad.h

+template <typename T, typename... Ts>
+struct index_in_pack;


Why does it repeat L36-L37?

I'll remove.

dali-automaton · 2022-02-09T17:48:45Z

CI MESSAGE: [3926424]: BUILD FAILED

dali/kernels/dynamic_scratchpad.h

jantonguirao · 2022-02-10T08:15:45Z

dali/kernels/dynamic_scratchpad.h

+class DynamicScratchpadImplT {
+ protected:
+  template <typename Kind>
+  void set_upstream_resrouce(mm::memory_resource<Kind> *rsrc) {


Suggested change

void set_upstream_resrouce(mm::memory_resource<Kind> *rsrc) {

void set_upstream_resource(mm::memory_resource<Kind> *rsrc) {

jantonguirao · 2022-02-10T08:16:13Z

dali/kernels/dynamic_scratchpad.h

+  }
+
+  template <typename Kind>
+  void set_upstream_resrouce(mm::async_memory_resource<Kind> *rsrc,


Suggested change

void set_upstream_resrouce(mm::async_memory_resource<Kind> *rsrc,

void set_upstream_resource(mm::async_memory_resource<Kind> *rsrc,

jantonguirao · 2022-02-10T08:38:47Z

dali/kernels/dynamic_scratchpad_test.cc

+    if (was_running && !running)
+      break;
+  }
+  if (!was_running)


how about ASSERT_TRUE(was_running)?

I don't want it to just fail. On some machines it might be impossible to reach this kind of concurrency, e.g. due to CPU load.

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

Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>

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

- extract perf printing to a function - remove duplicate forward-declaration - fix typos - properly reserve vectors for perf results Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>

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

dali/kernels/dynamic_scratchpad.h

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

dali-automaton · 2022-02-10T12:20:18Z

CI MESSAGE: [3933823]: BUILD STARTED

dali-automaton · 2022-02-10T13:48:04Z

CI MESSAGE: [3933823]: BUILD PASSED

* Fix monotonic resource with 0 initial size. * Add dynamic scratchpad with tests and benchmarks. * Add fixed_order_memory_resource - a wrapper which exposes a streamless interface for stream-ordered resources Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>

JanuszL reviewed Feb 9, 2022

View reviewed changes

dali/kernels/dynamic_scratchpad_test.cc Show resolved Hide resolved

JanuszL reviewed Feb 9, 2022

View reviewed changes

dali/kernels/dynamic_scratchpad_test.cc Show resolved Hide resolved

JanuszL reviewed Feb 9, 2022

View reviewed changes

dali/kernels/dynamic_scratchpad_test.cc Show resolved Hide resolved

JanuszL reviewed Feb 9, 2022

View reviewed changes

dali/kernels/dynamic_scratchpad.h Show resolved Hide resolved

JanuszL self-assigned this Feb 9, 2022

jantonguirao changed the title ~~Dynamic & stream-aware sratchpad~~ Dynamic & stream-aware scratchpad Feb 10, 2022

jantonguirao self-assigned this Feb 10, 2022

jantonguirao approved these changes Feb 10, 2022

View reviewed changes

mzient and others added 6 commits February 10, 2022 11:17

Add async free_all to monotonic_resource.

b6691c2

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

Rebase.

d01f4a7

Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>

Fix monotonic resource with 0 initial size.

a01d6b1

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

Add dynamic scratchpad (with) tests.

9466944

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

Remove free_all_async and add fixed_order_resource.

841bc07

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

Fix review issues:

57aa697

- extract perf printing to a function - remove duplicate forward-declaration - fix typos - properly reserve vectors for perf results Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>

mzient force-pushed the DynamicSratchpad branch from 8337aaf to 57aa697 Compare February 10, 2022 12:01

Silence clang warning about non-virtual destructor.

b26fa2d

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

JanuszL reviewed Feb 10, 2022

View reviewed changes

dali/kernels/dynamic_scratchpad.h Show resolved Hide resolved

JanuszL approved these changes Feb 10, 2022

View reviewed changes

Add documentation for the constructor.

5af45e7

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

NVIDIA deleted a comment from dali-automaton Feb 10, 2022

mzient merged commit bf16cc8 into NVIDIA:main Feb 10, 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

Dynamic & stream-aware scratchpad #3667

Dynamic & stream-aware scratchpad #3667

mzient commented Feb 9, 2022 •

edited

Loading

dali-automaton commented Feb 9, 2022

JanuszL Feb 9, 2022

mzient Feb 9, 2022

JanuszL Feb 9, 2022

JanuszL Feb 9, 2022

JanuszL Feb 9, 2022

mzient Feb 10, 2022

dali-automaton commented Feb 9, 2022

jantonguirao Feb 10, 2022

jantonguirao Feb 10, 2022

jantonguirao Feb 10, 2022

mzient Feb 10, 2022

dali-automaton commented Feb 10, 2022

dali-automaton commented Feb 10, 2022

	void set_upstream_resrouce(mm::memory_resource<Kind> *rsrc) {
	void set_upstream_resource(mm::memory_resource<Kind> *rsrc) {

	void set_upstream_resrouce(mm::async_memory_resource<Kind> *rsrc,
	void set_upstream_resource(mm::async_memory_resource<Kind> *rsrc,

Dynamic & stream-aware scratchpad #3667

Dynamic & stream-aware scratchpad #3667

Conversation

mzient commented Feb 9, 2022 • edited Loading

Category:

Description:

Additional information:

Affected modules and functionalities:

Key points relevant for the review:

Checklist

Tests

Documentation

DALI team only

Requirements

dali-automaton commented Feb 9, 2022

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 9, 2022

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 10, 2022

dali-automaton commented Feb 10, 2022

mzient commented Feb 9, 2022 •

edited

Loading