Add first pass of rocm kernel profiler by mwootton · Pull Request #10911 · microsoft/onnxruntime

mwootton · 2022-03-17T10:23:03Z

Description: Add a profiler to collect ROCm kernel and copy executions.

Motivation and Context
This will allow attribution of AMD gpu executions to ops.

No Issue yet?

Deleted user (ghost) · 2022-03-17T10:23:15Z

All CLA requirements met.

mwootton · 2022-03-17T21:54:51Z

Some background. The libroctracer api used to collect rocm execution is similar to cupti. However, it is a bit more rudimentary and requires more support code. That code ends up in every implementation. The code in this PR is based on a Kineto implementation I did which was in turn based on a standalone profiler I have done.

What I have done is refactor the code into two pieces:

RoctracerLogger.* is the "common" code. It does the machinations to collect and store the roctracer data. It is free of any concerns outside of roctracer.
rocm_profiler.* is the ORT "adapter". It switches the RoctracerLogger on and off. It takes the data collected and delivers it into the onnxruntime::profiling::EventRecord "ecosystem".

The ultimate goal here would be to use the RoctracerLogger across multiple projects. That way features and bug fixes can happen in one place. I will go back and refactor kineto to use RoctracerLogger (and an adapter). I'm not sure if it is practical to share the actual source file, ideas for that appreciated.

mwootton · 2022-03-17T22:13:01Z

Issues to resolve:

API calls
Do we want to introduce api calls (ie. runtime)? The cupti implementation enables CUPTI_ACTIVITY_KIND_RUNTIME and CUPTI_ACTIVITY_KIND_DRIVER but ignores them in CudaProfiler::BufferCompleted. So only the gpu ops are being recorded.
These "api calls" would show up on the cpu thread, in e.g. chrome trace, as things like: "hipMalloc", "hipMemcpyAsync", "hipLaunchKernel" under the operators that called them. Yao though this would be helpful. All tracers I have worked on output these.

We will need to add an EventCategory, e.g. (SESSION_EVENT, NODE_EVENT, KERNEL_EVENT). Preference? API_EVENT?

Kernel Name Demangling
I didn't see demangling happening in the cupti implementation (and I recall they come mangled). Can we set up a common file to do this? I can send an example.

String Formatting
Is there a preferred way to do string formatting? For example I need to convert a pointer into "0x12345678". I have used fmt in the past.

RandySheriffH (RandySheriffH) · 2022-03-29T00:19:16Z

@@ -0,0 +1,194 @@
+// (c) Meta Platforms, Inc. and affiliates. Confidential and proprietary.


Pls make sure to have all file headers aligned.

Yeah. This is one of the files we want to share between framework profilers. I'm wondering if we should just add the files as a submodule.

RandySheriffH (RandySheriffH) · 2022-03-29T00:35:31Z

+      : data(data), validSize(validSize) {}
+
+  ~RoctracerActivityBuffer() {
+    free(data);


data

Should RoctracerActivityBuffer support move/copy? if yes should we reset data? Otherwise we should mark then as deleted explicitly.

I don't anticipate anyone reusing this structure, it is just a wrapper to make sure some malloc memory is freed. These will never be copied or moved. Just keeping it simple, no interest in copy semantics or blocking operator= or copy constructors.

Then u should mark move/copy as deleted.

RandySheriffH (RandySheriffH) · 2022-03-29T00:37:14Z

+  }
+
+  // Allocated by malloc
+  uint8_t* data{nullptr};


data

Mind name it to "data_" to follow onnxruntime coding style? Same apply to entire PR.

Ah yes, the horrid google style. Missed that one, and one other, thanks.

RandySheriffH (RandySheriffH) · 2022-03-29T00:40:01Z

+  bool invertMode() { return invert_; }
+  void setInvertMode(bool invert) { invert_ = invert; }
+  void add(std::string apiName);
+  void remove(std::string apiName);


std::string

Is this passing by value a deliberately expected?

Obviously not, fixed, thanks.

RandySheriffH (RandySheriffH) · 2022-03-29T01:07:27Z

+
+//namespace profiling {
+
+int32_t systemThreadId();


onnxruntime/include/onnxruntime/core/common/logging/logging.h

Line 324 in 3b6cee8

unsigned int GetThreadId();

I tried to switch over to using the methods in logging.h but had issues with linking. Simple approach caused a failure at runtime to load the rocm_provider.so because the symbols couldn't be resolved. Pulling logging.cc into the rocm_provider caused a duplicated symbol. Couldn't spot an example in the cmake files of how to do this correctly. I lost interest at this point.

I am still using the original functions. End of day they are probably a bit faster as they are using tls to cache the pid,tid.

RandySheriffH (RandySheriffH) · 2022-03-29T01:08:12Z

+ private:
+  void addEventRecord(const roctracerRow &item, int64_t pstart, const std::initializer_list<std::pair<std::string, std::string>> &args, std::map<uint64_t, std::vector<EventRecord>> &event_map);
+
+  RoctracerLogger *d;


d

a better name?

This is just indicating that it is emulating "d pointer" pattern, i.e. pimpl. I can't think of another name that would add any value. Whatever makes you happy.

RandySheriffH (RandySheriffH) · 2022-03-29T01:24:07Z

+  if (!singleton().externalCorrelationEnabled_) {
+    return;
+  }
+  t_externalIds[type].push_back(id);


t_externalIds[type].push_back(id);

so for rocm profiler, the pushCorrelationID(...) will not place an id on device?

This is a limitation of libroctracer. If we have roctracer pass back digest-style api calls (a buffer at a time like cupti does) then those pages lack the call arguments. So, as you can see, we are collecting the calls and args inline. Roctracer only generates external correlation events when using that page-at-time method. This is fine, my inline method is faster, gets the args, and can generate external correlation.

Unless you understand more about external correlation implementation than I do... your statement about "place an id on device" is a misconception. All that happens is that a mapping from internal correlation id to (one or more) external correlation ids is generated when the api call is made. The 'device' is running ops and tracking them by internal correlation id (that is shares with the generating api call), just like normal.

Because I have 'bitten the bullet' and I am doing inline collection I can implement external correlation here. Without using inline callbacks cupti would have to do this for me and deliver a bunch of records. My way is clearly faster, so it probably turned into an advantage.

RandySheriffH (RandySheriffH)

Pls do paste some profiled numbers as example in PR description, and add at least one test case.

Add Api EventRecords

mwootton · 2022-05-12T16:14:40Z

mwootton · 2022-05-12T16:44:36Z

I'm going to create a second PR with the cupti changes (api calls and name demangling). The cupti side will need to use demangle.cc and demangle.h (which are in providers/rocm right now). What is a good common location for those 2 files in your opinion?

RandySheriffH (RandySheriffH) · 2022-05-31T19:16:19Z

+
+//namespace onnxruntime {
+
+//namespace profiling {


profiling

Uncomment the namespace?
Like discussed - we prefer RocTracer to go with built-in threading utilities. But reallly we have to add it here, let's put them into a namespace.

RandySheriffH (RandySheriffH) · 2022-05-31T19:32:28Z

I'm going to create a second PR with the cupti changes (api calls and name demangling). The cupti side will need to use demangle.cc and demangle.h (which are in providers/rocm right now). What is a good common location for those 2 files in your opinion?

Would u mind to merge the content of demangle.* into profiler_common.*?

mwootton · 2022-06-09T19:59:21Z

30 second run:

Close up:

python benchmark.py -g -m bert-base-cased --sequence_length 384 --batch_sizes 128 --provider=rocm -p fp16 --disable_embed_layer_norm

I'll attempt to link against the profiler_common code again. If I can get it to work I'll move the demangle into there.
Also, I fixed the issue where the gpuTraceBuffers weren't being cleared when everything else was. I will push the remaining changes soon.

…pu buffer clearing.

mwootton · 2022-06-22T17:37:09Z

So I half-missed your comment about moving demangle into profiler-common. I did put those files into into core/common, but that is not exactly what you asked. But I think I would have had an issue with what you asked:

There is no profiler_common.cc, it is all declaration and it is in "onnxruntime/include/onnxruntime/core/common/profiler_common.h". So there is no place for the demangle implementation.
There is "/onnxruntime/onnxruntime/core/common/profiler.cc" (and matching profiler.h) but that is more of the external interface to profiling, not profiler utils.

So where would you want the demangle implementation added?

RandySheriffH (RandySheriffH) · 2022-06-27T22:04:41Z

So I half-missed your comment about moving demangle into profiler-common. I did put those files into into core/common, but that is not exactly what you asked. But I think I would have had an issue with what you asked:

There is no profiler_common.cc, it is all declaration and it is in "onnxruntime/include/onnxruntime/core/common/profiler_common.h". So there is no place for the demangle implementation. There is "/onnxruntime/onnxruntime/core/common/profiler.cc" (and matching profiler.h) but that is more of the external interface to profiling, not profiler utils.

So where would you want the demangle implementation added?

You could create there be a profile_common.cc.

azure-pipelines · 2022-08-16T06:13:42Z

Azure Pipelines successfully started running 9 pipeline(s).

azure-pipelines · 2022-08-16T06:14:03Z

Azure Pipelines successfully started running 8 pipeline(s).

ytaous · 2022-08-16T23:17:34Z

Please fix "Python format (pull_request) " error, thx

ytaous · 2022-08-16T23:18:09Z

RandySheriffH (@RandySheriffH) - please have another look? thx

ytaous · 2022-08-18T20:35:45Z

/azp run Windows CPU CI Pipeline, Windows GPU CI Pipeline, Windows GPU TensorRT CI Pipeline, Windows WebAssembly CI Pipeline, orttraining-amd-gpu-ci-pipeline, orttraining-linux-ci-pipeline, orttraining-linux-gpu-ci-pipeline, orttraining-ortmodule-distributed, onnxruntime-python-checks-ci-pipeline

ytaous · 2022-08-18T20:36:00Z

/azp run Linux CPU CI Pipeline, Linux CPU Minimal Build E2E CI Pipeline, Linux GPU CI Pipeline, Linux GPU TensorRT CI Pipeline, Linux Nuphar CI Pipeline, Linux OpenVINO CI Pipeline, MacOS CI Pipeline, ONNX Runtime Web CI Pipeline, onnxruntime-binary-size-checks-ci-pipeline

azure-pipelines · 2022-08-18T20:36:24Z

Azure Pipelines successfully started running 8 pipeline(s).

azure-pipelines · 2022-08-18T20:36:41Z

Azure Pipelines successfully started running 9 pipeline(s).

ytaous · 2022-08-24T21:42:53Z

Lint / Python format (pull_request)

please fix this, otherwise it will block u from merging it, thx
mwootton

ytaous · 2022-08-26T19:52:32Z

/azp run Windows CPU CI Pipeline, Windows GPU CI Pipeline, Windows GPU TensorRT CI Pipeline, Windows WebAssembly CI Pipeline, orttraining-amd-gpu-ci-pipeline, orttraining-linux-ci-pipeline, orttraining-linux-gpu-ci-pipeline, orttraining-ortmodule-distributed, onnxruntime-python-checks-ci-pipeline

ytaous · 2022-08-26T19:52:48Z

/azp run Linux CPU CI Pipeline, Linux CPU Minimal Build E2E CI Pipeline, Linux GPU CI Pipeline, Linux GPU TensorRT CI Pipeline, Linux Nuphar CI Pipeline, Linux OpenVINO CI Pipeline, MacOS CI Pipeline, ONNX Runtime Web CI Pipeline, onnxruntime-binary-size-checks-ci-pipeline

azure-pipelines · 2022-08-26T19:53:05Z

Azure Pipelines successfully started running 8 pipeline(s).

azure-pipelines · 2022-08-26T19:53:23Z

Azure Pipelines successfully started running 9 pipeline(s).

ytaous · 2022-08-26T20:00:02Z

please run "black" on your local machine, it will correct the format for you.

lgtm-com · 2022-08-26T21:15:04Z

This pull request fixes 5 alerts when merging 74084d2 into a972db0 - view on LGTM.com

fixed alerts:

5 for Uncontrolled data used in path expression

ytaous · 2022-08-27T00:19:54Z

/azp run Linux CPU CI Pipeline, Linux CPU Minimal Build E2E CI Pipeline, Linux GPU CI Pipeline, Linux GPU TensorRT CI Pipeline, Linux Nuphar CI Pipeline, Linux OpenVINO CI Pipeline, MacOS CI Pipeline, ONNX Runtime Web CI Pipeline, onnxruntime-binary-size-checks-ci-pipeline

ytaous · 2022-08-27T00:20:06Z

/azp run Windows CPU CI Pipeline, Windows GPU CI Pipeline, Windows GPU TensorRT CI Pipeline, Windows WebAssembly CI Pipeline, orttraining-amd-gpu-ci-pipeline, orttraining-linux-ci-pipeline, orttraining-linux-gpu-ci-pipeline, orttraining-ortmodule-distributed, onnxruntime-python-checks-ci-pipeline

azure-pipelines · 2022-08-27T00:20:26Z

Azure Pipelines successfully started running 9 pipeline(s).

azure-pipelines · 2022-08-27T00:20:38Z

Azure Pipelines successfully started running 8 pipeline(s).

ytaous · 2022-08-27T05:22:35Z

Thank you for the contribution !!

Add first pass of rocm kernel profiler

8ad6a01

Yao Zhang (zhangyaobit) requested a review from RandySheriffH (RandySheriffH) March 17, 2022 19:21

RandySheriffH (RandySheriffH) reviewed Mar 29, 2022

View reviewed changes

RandySheriffH (RandySheriffH) suggested changes Mar 29, 2022

View reviewed changes

mwootton added 3 commits May 12, 2022 01:45

Clean up rocm_profiler. Format args. Demangle kernel names.

2a996bf

Add Api EventRecords

Merge branch 'master' into rocm_profiler

232d8ee

Remove debug output

8ee2901

mwootton marked this pull request as ready for review May 12, 2022 16:46

mwootton added 2 commits May 12, 2022 12:43

Temporarily disable profiling unit test 'api record check' for cupti

e830d72

Fix compile error for non-gpu builds

4d549e7

RandySheriffH (RandySheriffH) reviewed May 31, 2022

View reviewed changes

Use common file for demangle and pid/tid. Namespace ThreadUtil. Fix g…

3b5455e

…pu buffer clearing.

Merge branch 'master' into rocm_profiler

d1217d0

mwootton added 2 commits June 28, 2022 11:07

Merge demangle into profiler_common

458a81c

Merge branch 'master' into rocm_profiler

24d62b5

ytaous reviewed Aug 16, 2022

View reviewed changes

Comment thread cmake/onnxruntime_providers.cmake Outdated

Clean up comments

9eb43ac

RandySheriffH (RandySheriffH) previously approved these changes Aug 24, 2022

View reviewed changes

Fix formatting

74084d2

mwootton dismissed RandySheriffH (RandySheriffH)’s stale review via 74084d2 August 26, 2022 19:25

Fix anal retentive formatting

fefc298

ytaous approved these changes Aug 27, 2022

View reviewed changes

ytaous merged commit 817dc94 into microsoft:main Aug 27, 2022

ytaous mentioned this pull request Sep 19, 2022

[Add] add Rocm ep doc #12971

Merged

9 tasks

cloudhan mentioned this pull request Nov 4, 2022

Make the ROCM profiler thread-safe, session-aware and preserve logical ordering between CPU and GPU events #13549

Merged

		@@ -0,0 +1,194 @@
		// (c) Meta Platforms, Inc. and affiliates. Confidential and proprietary.


		//namespace profiling {

		int32_t systemThreadId();


		//namespace onnxruntime {

		//namespace profiling {

Uh oh!

Conversation

mwootton commented Mar 17, 2022

Uh oh!

Deleted user (ghost) commented Mar 17, 2022 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mwootton commented Mar 17, 2022

Uh oh!

mwootton commented Mar 17, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RandySheriffH (RandySheriffH) Mar 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RandySheriffH (RandySheriffH) left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mwootton commented May 12, 2022

Uh oh!

mwootton commented May 12, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RandySheriffH (RandySheriffH) commented May 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mwootton commented Jun 9, 2022

Uh oh!

mwootton commented Jun 22, 2022

Uh oh!

RandySheriffH (RandySheriffH) commented Jun 27, 2022

Uh oh!

azure-pipelines Bot commented Aug 16, 2022

Uh oh!

azure-pipelines Bot commented Aug 16, 2022

Uh oh!

Uh oh!

ytaous commented Aug 16, 2022

Uh oh!

ytaous commented Aug 16, 2022

Uh oh!

ytaous commented Aug 18, 2022

Uh oh!

ytaous commented Aug 18, 2022

Uh oh!

azure-pipelines Bot commented Aug 18, 2022

Uh oh!

azure-pipelines Bot commented Aug 18, 2022

Uh oh!

ytaous commented Aug 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Deleted user (ghost) commented Mar 17, 2022 •

edited by ghost

Loading

RandySheriffH (RandySheriffH) Mar 29, 2022 •

edited

Loading

RandySheriffH (RandySheriffH) left a comment •

edited

Loading

RandySheriffH (RandySheriffH) commented May 31, 2022 •

edited

Loading

ytaous commented Aug 24, 2022 •

edited

Loading