Move all NVTX infrastructure into core and create DALI domain #2472

JanuszL · 2020-11-16T20:16:14Z

libdali_core now provides NVTX ranges functionality
core is responsible for creating one DALI domain
regular DALI code should use DomainTimeRange from core lib
plain TimeRange is still header-only and should be used only for the development

Signed-off-by: Janusz Lisiecki jlisiecki@nvidia.com

Why we need this PR?

Pick one, remove the rest

It moves all NVTX infrastructure into core and create DALI domain

What happened in this PR?

Fill relevant points, put NA otherwise. Replace anything inside []

What solution was applied:
libdali_core now provides NVTX ranges functionality
Affected modules and functionalities:
core
all modules using NVTX ranges
Key points relevant for the review:
NA
Validation and testing:
CI
Documentation (including examples):
NA

JIRA TASK: [DALI-1728]

JanuszL · 2020-11-16T20:16:21Z

!build

JanuszL · 2020-11-16T20:41:17Z

dali-automaton · 2020-11-16T20:43:42Z

CI MESSAGE: [1803789]: BUILD STARTED

dali-automaton · 2020-11-16T20:55:47Z

CI MESSAGE: [1803789]: BUILD FAILED

- libdali_core now provides NVTX ranges functionality - core is responsible for creating one DALI domain - regular DALI code should use DomainTimeRange from core lib - plain TimeRange is still header-only and should be used only for the development Signed-off-by: Janusz Lisiecki <jlisiecki@nvidia.com>

JanuszL · 2020-11-16T21:00:59Z

!build

dali-automaton · 2020-11-16T21:21:38Z

CI MESSAGE: [1803930]: BUILD STARTED

dali-automaton · 2020-11-16T22:53:53Z

CI MESSAGE: [1803930]: BUILD PASSED

klecki · 2020-11-17T09:55:00Z

dali/operators/reader/reader_op.h

@@ -80,7 +81,8 @@ class DataReader : public Operator<Backend> {
  // perform the prefetching operation
  virtual void Prefetch() {
    // We actually prepare the next batch
-    TimeRange tr("DataReader::Prefetch #" + to_string(curr_batch_producer_), TimeRange::kRed);
+    DomainTimeRange tr("[DALI] DataReader::Prefetch #" + to_string(curr_batch_producer_),


Can we make them

Suggested change

DomainTimeRange tr("[DALI] DataReader::Prefetch #" + to_string(curr_batch_producer_),

DomainTimeRange tr("[DALI][DataReader] Prefetch #" + to_string(curr_batch_producer_),

and so on?

klecki · 2020-11-17T10:00:06Z

dali/core/nvtx.cc

+    nvtxEventAttributes_t att = {};
+    att.version = NVTX_VERSION;
+    att.size = NVTX_EVENT_ATTRIB_STRUCT_SIZE;
+    att.colorType = NVTX_COLOR_ARGB;
+    att.color = rgb | 0xff000000;
+    att.messageType = NVTX_MESSAGE_TYPE_ASCII;
+    att.message.ascii = name.c_str();


Maybe deduplicate filling the attributes?
But I don't know if it's worth, maybe until it doesn't need to diverge, we can put it in the base class?

Signed-off-by: Janusz Lisiecki <jlisiecki@nvidia.com>

JanuszL · 2020-11-17T10:35:05Z

!build

dali-automaton · 2020-11-17T10:40:33Z

CI MESSAGE: [1805949]: BUILD STARTED

Signed-off-by: Janusz Lisiecki <jlisiecki@nvidia.com>

JanuszL · 2020-11-17T10:50:23Z

!build

dali-automaton · 2020-11-17T10:58:23Z

CI MESSAGE: [1805976]: BUILD STARTED

dali-automaton · 2020-11-17T12:41:33Z

CI MESSAGE: [1805976]: BUILD PASSED

klecki

Looks ok, apart maybe from the stylistic choice of using RangeBase for colors.

A nice followup would be some .rst mentioning existence of those ranges.

klecki · 2020-11-17T13:32:20Z

dali/operators/reader/loader/loader.h

    // perform an initial buffer fill if it hasn't already happened
    if (!initial_buffer_filled_) {
-      TimeRange tr("[Loader] Filling initial buffer", TimeRange::kBlue1);
+      DomainTimeRange tr("[DALI][Loader] Filling initial buffer", RangeBase::kBlue1);


Hmm, my only nitpick would be that we use RangeBase instead of DomainTimeRange::kBlue... directly.

klecki · 2020-11-17T13:33:58Z

include/dali/core/nvtx.h

+
+namespace dali {
+
+inline void FillAtrbs(nvtxEventAttributes_t &att, const std::string &name, const uint32_t rgb) {


Why a free function and not something in the base class?

Signed-off-by: Janusz Lisiecki <jlisiecki@nvidia.com>

JanuszL · 2020-11-17T13:49:09Z

!build

dali-automaton · 2020-11-17T13:55:24Z

CI MESSAGE: [1806284]: BUILD STARTED

mzient · 2020-11-17T15:07:43Z

include/dali/core/nvtx.h

+  static const uint32_t kGreen1 = 0x859900;
+  static const uint32_t knvGreen = 0x76B900;
+
+  inline void FillAtrbs(nvtxEventAttributes_t &att, const std::string &name, const uint32_t rgb) {


Suggested change

inline void FillAtrbs(nvtxEventAttributes_t &att, const std::string &name, const uint32_t rgb) {

inline void FillAtrbs(nvtxEventAttributes_t &att, const char *name, const uint32_t rgb) {

mzient · 2020-11-17T15:08:27Z

include/dali/core/nvtx.h

+};
+
+struct DomainTimeRange : RangeBase {
+  explicit DomainTimeRange(const std::string &name, const uint32_t rgb = kBlue);


An overload with a const char *name would be welcome - we really don't have to create std::strings each time we enter a time range.

mzient · 2020-11-17T15:09:22Z

include/dali/core/nvtx.h

+
+// Basic timerange for profiling
+struct TimeRange : RangeBase {
+  TimeRange(const std::string name, const uint32_t rgb = kBlue) {  // NOLINT


Suggested change

TimeRange(const std::string name, const uint32_t rgb = kBlue) { // NOLINT

TimeRange(const std::string &name, const uint32_t rgb = kBlue) : TimeRange(name.c_str(), rgb) {} // NOLINT

TimeRange(const char *name, const uint32_t rgb = kBlue) { // NOLINT

mzient · 2020-11-17T15:17:31Z

dali/core/nvtx.cc

+};
+#endif
+
+static DomainTimeRangeImpl range_impl;


Won't compile without NVTX_ENABLED. BTW, perhaps it would be better to make it a signleton? Now we're initializing the object at load time - do we really want that (as opposed to initialize on first use)?

mzient · 2020-11-17T15:18:56Z

include/dali/core/nvtx.h

+
+  bool started = false;
+};
+


Suggested change

#if NTXT_ENABLED

mzient · 2020-11-17T15:20:09Z

include/dali/core/nvtx.h

+  explicit DomainTimeRange(const std::string &name, const uint32_t rgb = kBlue);
+  ~DomainTimeRange();
+};
+


Suggested change

#else

struct DomainTimeRange : RangeBase {

using RangeBase::RangeBase;

};

#endif

^^^ this would avoid the costly call to a DLL-exported function when not using NVTX.

dali-automaton · 2020-11-17T15:27:29Z

CI MESSAGE: [1806284]: BUILD PASSED

JanuszL · 2020-11-18T12:23:15Z

!build

dali-automaton · 2020-11-18T12:25:29Z

CI MESSAGE: [1810072]: BUILD STARTED

Signed-off-by: Janusz Lisiecki <jlisiecki@nvidia.com>

JanuszL · 2020-11-18T13:04:04Z

!build

dali-automaton · 2020-11-18T13:05:27Z

CI MESSAGE: [1810187]: BUILD STARTED

dali-automaton · 2020-11-18T14:29:38Z

CI MESSAGE: [1810187]: BUILD PASSED

JanuszL force-pushed the rework_nvtx branch from 4d87d2e to ca3686d Compare November 16, 2020 20:58

JanuszL force-pushed the rework_nvtx branch from ca3686d to 587aff1 Compare November 16, 2020 20:59

klecki reviewed Nov 17, 2020

View reviewed changes

Review fixes

3c0d75c

Signed-off-by: Janusz Lisiecki <jlisiecki@nvidia.com>

JanuszL force-pushed the rework_nvtx branch from c96cd0e to 3c0d75c Compare November 17, 2020 10:15

Review fixes

f98b8a8

Signed-off-by: Janusz Lisiecki <jlisiecki@nvidia.com>

klecki approved these changes Nov 17, 2020

View reviewed changes

Review fixes

5a2c416

Signed-off-by: Janusz Lisiecki <jlisiecki@nvidia.com>

mzient reviewed Nov 17, 2020

View reviewed changes

Review fixes

5e15d5e

Signed-off-by: Janusz Lisiecki <jlisiecki@nvidia.com>

JanuszL force-pushed the rework_nvtx branch from 72682de to 5e15d5e Compare November 18, 2020 13:03

mzient approved these changes Nov 18, 2020

View reviewed changes

JanuszL merged commit dc4bc35 into NVIDIA:master Nov 18, 2020

JanuszL deleted the rework_nvtx branch November 18, 2020 14:50

	DomainTimeRange tr("[DALI] DataReader::Prefetch #" + to_string(curr_batch_producer_),
	DomainTimeRange tr("[DALI][DataReader] Prefetch #" + to_string(curr_batch_producer_),


		namespace dali {

		inline void FillAtrbs(nvtxEventAttributes_t &att, const std::string &name, const uint32_t rgb) {

	TimeRange(const std::string name, const uint32_t rgb = kBlue) { // NOLINT
	TimeRange(const std::string &name, const uint32_t rgb = kBlue) : TimeRange(name.c_str(), rgb) {} // NOLINT
	TimeRange(const char *name, const uint32_t rgb = kBlue) { // NOLINT

Move all NVTX infrastructure into core and create DALI domain #2472

Move all NVTX infrastructure into core and create DALI domain #2472

Conversation

JanuszL commented Nov 16, 2020

Why we need this PR?

What happened in this PR?

JanuszL commented Nov 16, 2020

JanuszL commented Nov 16, 2020

dali-automaton commented Nov 16, 2020

dali-automaton commented Nov 16, 2020

JanuszL commented Nov 16, 2020

dali-automaton commented Nov 16, 2020

dali-automaton commented Nov 16, 2020

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

JanuszL commented Nov 17, 2020

dali-automaton commented Nov 17, 2020

JanuszL commented Nov 17, 2020

dali-automaton commented Nov 17, 2020

dali-automaton commented Nov 17, 2020

klecki 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

JanuszL commented Nov 17, 2020

dali-automaton commented Nov 17, 2020

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 Nov 17, 2020

JanuszL commented Nov 18, 2020

dali-automaton commented Nov 18, 2020

JanuszL commented Nov 18, 2020

dali-automaton commented Nov 18, 2020

dali-automaton commented Nov 18, 2020