[PROFILING] Use PAPI to collect hardware performance counters on CPU and CUDA #7983

tkonolige · 2021-05-05T21:13:02Z

This PR adds an optional dependency on PAPI (https://bitbucket.org/icl/papi/) in order to collect hardware performance counters on CPU and CUDA. These performance counters include data like total cycles, instructions executed, and cache misses. Users can control which performance counters are collected by setting the TVM_PAPI_${DEVICE}_METRICS environment variable to a semicolon separated list of metrics.

@leandron @areusch @junrushao1994 @icemelon9

include/tvm/runtime/c_backend_api.h

leandron

I think this is a good improvement so that we can get better data for performance analysis.

As this is a new dependency, I looked into their website, and it seems they license the project on BSD terms, https://bitbucket.org/icl/papi/src/master/LICENSE.txt.

Based on that, I think this PR should also add a copy of this license under licenses/ (https://github.com/apache/tvm/tree/main/licenses)? cc @tqchen @tmoreau89

CMakeLists.txt

leandron · 2021-05-06T09:05:34Z

cmake/modules/contrib/PAPI.cmake

+  set(ENV{PKG_CONFIG_PATH} "${USE_PAPI}:$ENV{PKG_CONFIG_PATH}")
+  pkg_check_modules(PAPI REQUIRED IMPORTED_TARGET papi>=6.0)
+  list(APPEND TVM_RUNTIME_LINKER_LIBS PkgConfig::PAPI)
+  list(APPEND RUNTIME_SRCS src/runtime/contrib/papi/papi.cc)


Do you know whether this works as expected for cross-compilation?

I'm not sure if this works with cross compiling. PAPI does support cross compilation though. I think you'd have to set PKG_CONFIG_PATH or USE_PAPI to point to the cross compiled library.

leandron · 2021-05-06T09:09:08Z

These performance counters include data like total cycles, instructions executed, and cache misses. Users can control which performance counters are collected by setting the TVM_PAPI_${DEVICE}_METRICS environment variable to a semicolon separated list of metrics.

Can you also document the use of $TVM_PAPI_${DEVICE}_METRICS somewhere? @hogepodge can probably advise here.

tqchen · 2021-05-06T12:14:52Z

licenses will be needed if we bundle PAI's source code in our source distribution (in the case when it is included as a submodule). If it is a system level dep, we do not need to add licenses to the licenses folder

hogepodge · 2021-05-06T16:33:40Z

@leandron @tkonolige I would suggest two pieces of documentation. The first would be a description of all of the variables available to configure Papi integration. The second would be a "how-to" guide that would walk the user through installing with Papi and running a basic instrumentation. The combination of these two should help users who are interested be successful with Papi.

areusch · 2021-05-06T16:59:51Z

include/tvm/runtime/profiling.h

  Timer timer;
  /*! Extra performance metrics */
  std::unordered_map<std::string, ObjectRef> extra_metrics;
+  /*! User defined metric collectors */


any reason this isn't a map? could you explain in a comment? what's the second item in the pair?

I never do a lookup by key; I just iterate over all the pairs. I could switch it to a map if you like.

include/tvm/runtime/profiling.h

src/runtime/contrib/papi/papi.cc

src/runtime/vm/executable.cc

areusch · 2021-05-10T17:47:34Z

src/runtime/profiling.cc


-  return profiling::Report(rows, device_metrics);
+  // the last couple of call frames are the overall times
+  double overall_time = 0;


suggest unit suffix: overall_time_us

still suggest this

areusch · 2021-05-10T20:35:02Z

src/runtime/vm/profiler/vm.cc

    metrics["Argument Shapes"] = profiling::ShapeString(shapes);

-    prof_.StartCall(packed_index_map_[packed_index], dev, metrics);
+    prof_.operator*().StartCall(packed_index_map_[packed_index], dev, metrics);


does -> not work?

nope :( -> is const only I think.

areusch · 2021-05-10T20:50:10Z

src/runtime/contrib/papi/papi.cc

+      int event_set = it->second;
+      std::vector<long_long> values(papi_metrics[dev].size());
+      PAPI_CALL(PAPI_read(event_set, values.data()));
+      return ObjectRef(make_object<PAPIEventSetNode>(values, dev));


could you give an example of nesting? I just see

StartCall() // run kernel StopCall()

my concern is that putting a bunch of malloc calls in between time points may inject a bunch of noise in the measurements

tkonolige · 2021-05-10T21:59:46Z

@areusch and I chatted a little offline about how to handle mallocs in profiling code. Right now there are a couple of places that do allocation and they would be pretty difficult to remove. Also, as long as there are not any nested Start calls (besides the top level nesting), the overhead of malloc is counted overhead section of the full model execution and has no effect on the performance of each op invocation. We should move forward with this PR as it stands and I will think about ways of reducing the amount of allocation that we do.

tkonolige · 2021-06-10T21:17:29Z

@leandron @tqchen @areusch Could you all re-review? I've pushed a change to the api to make it more closely match PassIntrument.

tkonolige · 2021-06-28T19:10:18Z

@leandron @tqchen @areusch Can you review?

leandron · 2021-06-29T10:33:15Z

python/tvm/contrib/debugger/debug_executor.py

        return ret.strip(",").split(",") if ret else []

-    def profile(self, **input_dict):
+    def profile(self, collectors=[], **input_dict):  # pylint: disable=dangerous-default-value


I see you have a # pylint: disable=dangerous-default-value, after setting the collectors=[].

Just wanted to double check whether this is intended, as collectors=[] behaves like a global, and there will be only one list that will be shared across all the calls to profile() in which collectors is not provided?

yeah don't use a list as a kwarg default. Instead, use None, and say collectors = collectors if collectors is not None else [] in function body

This list is never modified, so it should be fine.

leandron · 2021-06-29T10:34:56Z

python/tvm/runtime/profiler_vm.py

        return ""

-    def profile(self, *args, func_name="main", **kwargs):
+    def profile(self, *args, func_name="main", collectors=[], **kwargs):


Interesting pylint didn't complain about this one.

can you fix this one too?

leandron · 2021-06-29T10:39:35Z

src/runtime/vm/executable.cc

    primitive_names[packed_index] = it.first;
  }
  strm->Write(primitive_names);
+  // TODO(tkonolige): cannot serialize ObjectRefs with dmlc's serializer.


If this is not needed, maybe can be removed?

It is needed, saving and loading an executable will not preserve the extra op attributes.

…and CUDA This PR adds an optional dependency on PAPI (https://bitbucket.org/icl/papi/) in order to collect hardware performance counters on CPU and CUDA. These performance counters include data like total cycles, instructions executed, and cache misses. Users can control which performance counters are collected by setting the TVM_PAPI_${DEVICE}_METRICS environment variable to a semicolon separated list of metrics.

Co-authored-by: Leandro Nunes <leandro.nunes@arm.com>

areusch · 2021-06-29T15:44:50Z

python/tvm/contrib/debugger/debug_executor.py

        return ret.strip(",").split(",") if ret else []

-    def profile(self, **input_dict):
+    def profile(self, collectors=[], **input_dict):  # pylint: disable=dangerous-default-value


yeah don't use a list as a kwarg default. Instead, use None, and say collectors = collectors if collectors is not None else [] in function body

areusch · 2021-06-29T15:50:01Z

src/runtime/profiling.cc

-  // Stop all global timers. We wait to synchronize until we are making the report.
-  for (auto p : global_timers_) {
-    p.second->Stop();
+  is_running_ = false;


any thoughts on how errors should get handled here?

I'm not sure what errors would occur here?

areusch · 2021-06-29T15:50:11Z

src/runtime/profiling.cc


-  return profiling::Report(rows, device_metrics);
+  // the last couple of call frames are the overall times
+  double overall_time = 0;


still suggest this

tkonolige · 2021-07-01T17:34:17Z

@areusch ping

areusch

just a couple more questions, i think this looks fine otherwise

areusch · 2021-07-01T17:46:35Z

python/tvm/runtime/profiler_vm.py

        return ""

-    def profile(self, *args, func_name="main", **kwargs):
+    def profile(self, *args, func_name="main", collectors=[], **kwargs):


can you fix this one too?

src/runtime/profiling.cc

src/runtime/thread_pool.cc

tkonolige · 2021-07-02T17:19:30Z

@leandron Does this PR look good to you or would you like some more changes?

areusch · 2021-07-02T18:12:27Z

src/runtime/thread_pool.cc

-    queues_.clear();
-    threads_.reset();
+
+  void Init() {


wait but this should be private, no?

areusch · 2021-07-07T16:43:02Z

@leandron @tqchen please take a look and explicitly approve

tqchen · 2021-07-12T17:28:02Z

include/tvm/runtime/contrib/papi.h

+ *
+ * PAPI is avaliable at https://bitbucket.org/icl/papi/src/master/.
+ */
+struct PAPIMetricCollectorNode final : public MetricCollectorNode {


Given this is a implementation of an existing interface, we usually prefer hide as much as possible. As a result, let us to start as private header(move the def to cc file or the header file to src)

Users need to construct this to pass it to the profiler, so it needs to be in a public header.

Because the profiler itself is a public interface, what we could do instead is to only expose a factory function that returns the instance, but not the actual data structure itself

I've moved all definitions except a factory function into the .cc file.

tqchen · 2021-07-12T17:28:49Z

include/tvm/runtime/profiling.h

 Timer DefaultTimer(Device dev);

 namespace profiling {
+/*! \brief Wrapper for `Device` because `Device` is not passable across the


Device cannot be put as part of object container. How about DeviceObject?

DeviceObjectNode sounds a little weird. I think wrapper clearly explains what this is doing.

I am not strongly attached to the choice, given it is in the profiling namespace, i will let you decide then

tqchen · 2021-07-12T17:36:03Z

include/tvm/runtime/profiling.h

  std::unordered_map<std::string, ObjectRef> extra_metrics;
+  /*! User defined metric collectors. Each pair is the MetricCollector and its
+   * associated data (returned from MetricCollector.Start).
+   */


Not related to this PR but just in general about hiding. In this case, we could use https://en.cppreference.com/w/cpp/language/pimpl to hide the CallFrame def, recommend to come with a followup refactor

…and CUDA (apache#7983) * [PROFILING] Use PAPI to collect hardware performance counters on CPU and CUDA This PR adds an optional dependency on PAPI (https://bitbucket.org/icl/papi/) in order to collect hardware performance counters on CPU and CUDA. These performance counters include data like total cycles, instructions executed, and cache misses. Users can control which performance counters are collected by setting the TVM_PAPI_${DEVICE}_METRICS environment variable to a semicolon separated list of metrics. * Update CMakeLists.txt Co-authored-by: Leandro Nunes <leandro.nunes@arm.com> * move thread pool reset out of crt * add docs * comments * formatting * forgot one doc * kDLGPU -> kDLCUDA * Refactor API to more closely match pass instrument's. * forgot files * formatting * more lint * fix docs * optional loading of papi metric collector in python * more formatting * fix check * update docs and default value * formatting * addressing andrews comments * fix docs * address comments * move shared initialization code into private function * move most definitions from papi header to implementation file Co-authored-by: Leandro Nunes <leandro.nunes@arm.com>

tqchen requested changes May 5, 2021

View reviewed changes

include/tvm/runtime/c_backend_api.h Outdated Show resolved Hide resolved

leandron reviewed May 6, 2021

View reviewed changes

areusch requested changes May 6, 2021

View reviewed changes

areusch requested changes May 10, 2021

View reviewed changes

tkonolige mentioned this pull request May 11, 2021

[DOCKER,CI] Add PAPI to docker images #8016

Merged

areusch added the status: need update need update based on feedbacks label May 12, 2021

tkonolige force-pushed the profiler_papi branch 3 times, most recently from 8a6cba8 to df95b27 Compare June 10, 2021 21:16

leandron reviewed Jun 29, 2021

View reviewed changes

tkonolige and others added 14 commits June 29, 2021 09:56

Update CMakeLists.txt

d55c729

Co-authored-by: Leandro Nunes <leandro.nunes@arm.com>

move thread pool reset out of crt

41dbd74

add docs

8fdb981

comments

339dc7c

formatting

a1729ce

forgot one doc

3866076

kDLGPU -> kDLCUDA

4c4378b

Refactor API to more closely match pass instrument's.

3c816be

forgot files

a4321af

formatting

3342ff2

more lint

d2accca

fix docs

95836d2

optional loading of papi metric collector in python

51c73e5

tkonolige added 3 commits June 29, 2021 09:58

more formatting

78f7285

fix check

8ab30be

update docs and default value

b17850d

tkonolige force-pushed the profiler_papi branch from fb191d7 to b17850d Compare June 29, 2021 17:27

formatting

6a70fcb

areusch reviewed Jun 29, 2021

View reviewed changes

tkonolige added 2 commits June 29, 2021 11:55

addressing andrews comments

573231a

fix docs

19ad056

areusch requested changes Jul 1, 2021

View reviewed changes

address comments

34e657b

areusch reviewed Jul 2, 2021

View reviewed changes

src/runtime/thread_pool.cc Outdated

queues_.clear();

threads_.reset();

void Init() {

Copy link

Contributor

areusch Jul 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wait but this should be private, no?

move shared initialization code into private function

3600171

areusch approved these changes Jul 7, 2021

View reviewed changes

tqchen requested changes Jul 12, 2021

View reviewed changes

move most definitions from papi header to implementation file

45bfc17

tqchen approved these changes Jul 12, 2021

View reviewed changes

areusch merged commit d67514b into apache:main Jul 13, 2021

[PROFILING] Use PAPI to collect hardware performance counters on CPU and CUDA #7983

[PROFILING] Use PAPI to collect hardware performance counters on CPU and CUDA #7983

Uh oh!

Conversation

tkonolige commented May 5, 2021

Uh oh!

Uh oh!

leandron left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leandron commented May 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tqchen commented May 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hogepodge commented May 6, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

tkonolige May 10, 2021 • 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!

tkonolige commented May 10, 2021

Uh oh!

tkonolige commented Jun 10, 2021

Uh oh!

tkonolige commented Jun 28, 2021

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!

tkonolige commented Jul 1, 2021

Uh oh!

areusch left a comment

Choose a reason for hiding this comment

leandron commented May 6, 2021 •

edited

Loading

tqchen commented May 6, 2021 •

edited

Loading

tkonolige May 10, 2021 •

edited

Loading