Add support for device timers in summary hook #1313

upsj · 2023-03-24T21:04:55Z

This adds support for device timing to the summary hook used in benchmarks etc.
I want to use this to eventually replace the timers in benchmark, but could use some feedback on the design.

include/ginkgo/core/base/timer.hpp

hip/base/timer.hip.cpp

yhmtsai · 2023-03-29T13:56:22Z

include/ginkgo/core/base/timer.hpp

+/**
+ * An opaque wrapper for a time point generated by a timer.
+ */
+class time_point {


Do you need to store time_point outside the timer?
maybe put the time_point stack in the timer without using tag and union

That would mean re-implementing the stack logic for all timers. IMO it is a cleaner separation to make timers only provide functionality for recording times and differences. Many applications don't really need a stack of time points, but only a beginning and end, for that it could be overkill.

include/ginkgo/core/base/timer.hpp

yhmtsai

If there are some cpu operations after the GPU operations or only CPU operations, the GPU timer may give the wrong time region. That's why it is not suggested in solver benchmark.
the set_synchronize may give proper region with more overhead.
For example, the test in the test/base/timer.cpp is only true when the GPU is not busy before recording.

Although I think we can not solve it, users need to know these possibilities

core/base/timer.cpp

core/log/profiler_hook_summary.cpp

include/ginkgo/core/base/timer.hpp

test/log/profiler_hook.cpp

MarcelKoch

I have some issues with the Timer class. I think it can be broken down to removing the wait function, because it is IMO ambiguous. The record and difference should be enough for our use cases.

Personally, using int64 for the difference is fine with me.

include/ginkgo/core/base/timer.hpp

include/ginkgo/core/log/profiler_hook.hpp

core/log/profiler_hook_summary.cpp

MarcelKoch

LGTM, thanks for the updates. I only have some minor comments left.

include/ginkgo/core/base/timer.hpp

core/base/timer.cpp

MarcelKoch · 2023-05-17T10:14:03Z

core/base/timer.cpp

+namespace gko {
+
+
+time_point::time_point() : type_{type::cpu}, data_{} {}


nit:

Suggested change

time_point::time_point() : type_{type::cpu}, data_{} {}

time_point::time_point() : type_{type::cpu}, data_{nullptr} {}

It might be just me, but I don't think the default initialization of a union type is very clear.

this is necessary since std::chrono::steady_clock::time_point doesn't have a default constructor, the pointers work fine

include/ginkgo/core/base/timer.hpp

yhmtsai

I think it touches the public interface.
If we can not make it in this release, it will introduce the public interface break.

core/device_hooks/dpcpp_hooks.cpp

core/device_hooks/hip_hooks.cpp

include/ginkgo/core/log/profiler_hook.hpp

yhmtsai · 2023-05-23T14:07:50Z

test/base/timer.cpp

+ timer->record(start);
+ std::this_thread::sleep_for(std::chrono::seconds{5});
+ timer->record(stop);
+ timer->wait(stop);


maybe we never face the situation, but it is possible that the difference from start and stop is less than 1 second.

that's quite possible, but so far it doesn't seem to be happening. If it does, we could rethink the tests? For now, I only want to make sure they don't throw any exceptions

include/ginkgo/core/base/timer.hpp

yhmtsai · 2023-05-23T14:57:59Z

core/log/profiler_hook_summary.cpp

+ const auto cpu_now2 = cpu_clock::now();
+ // we need to exclude the wait for the timer from the overhead
+ // measurement
+ timer->wait(now);


should pop also contain the wait when the profilehook contains synchronize feature?
although I think the event logger is broken if the device does not execute them yet.
another concern:
should pop get time when it pops? pop only pops the time_point out for the start/end pair but do not compute the time.
you will have a time_point list and it will be filled by the time information by execution.
only compute the time difference when writing the summary.
We can have only one sync before writing the summary not each pop

If I understand it correctly, you mean storing an ever growing vector of time_points and only computing the durations afterwards? That would make the timing fully asynchronous, but at the cost of much more complex memory management for time points and nested tree nodes. The current approach can run as long as we want without need for cleanup, while storing time points without evaluating the durations requires something like garbage collection at regular intervals, which I'm not sure we should try to implement efficiently.

On the first point, can you elaborate how the logger would be broken in the case you described (synchronization enabled, I guess)?

yes, I also thought it also require more work to do fully asynchronized timing.
I mean, if we do not enable the synchronization and do not have wait() here, the time event will possibly be not set yet to lead broken.

I see, then I understood you correctly :)

- don't measure time waiting for timer as overhead - avoid cyclical dependency between time_point (core) and timers (device) - fix DPCPP timers - remove unnecessary scoping

- make wait non-const - extract duplicated code - move time point record in push to the end - guard device ID in CUDA/HIP timer operations - improve documentation - replace init_time_point by create_time_point Co-authored-by: Yuhsiang M. Tsai <[email protected]> Co-authored-by: Marcel Koch <[email protected]>

- change return type to chrono::nanoseconds - separate waiting from difference - add documentation Co-authored-by: Marcel Koch <[email protected]>

Co-authored-by: Yuhsiang M. Tsai <[email protected]>

sonarcloud · 2023-05-25T05:13:43Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
58 Code Smells

75.2% Coverage
0.0% Duplication

Release 1.6.0 of Ginkgo. The Ginkgo team is proud to announce the new Ginkgo minor release 1.6.0. This release brings new features such as: - Several building blocks for GPU-resident sparse direct solvers like symbolic and numerical LU and Cholesky factorization, ..., - A distributed Schwarz preconditioner, - New FGMRES and GCR solvers, - Distributed benchmarks for the SpMV operation, solvers, ... - Support for non-default streams in the CUDA and HIP backends, - Mixed precision support for the CSR SpMV, - A new profiling logger which integrates with NVTX, ROCTX, TAU and VTune to provide internal Ginkgo knowledge to most HPC profilers! and much more. If you face an issue, please first check our [known issues page](https://github.com/ginkgo-project/ginkgo/wiki/Known-Issues) and the [open issues list](https://github.com/ginkgo-project/ginkgo/issues) and if you do not find a solution, feel free to [open a new issue](https://github.com/ginkgo-project/ginkgo/issues/new/choose) or ask a question using the [github discussions](https://github.com/ginkgo-project/ginkgo/discussions). Supported systems and requirements: + For all platforms, CMake 3.13+ + C++14 compliant compiler + Linux and macOS + GCC: 5.5+ + clang: 3.9+ + Intel compiler: 2018+ + Apple Clang: 14.0 is tested. Earlier versions might also work. + NVHPC: 22.7+ + Cray Compiler: 14.0.1+ + CUDA module: CUDA 9.2+ or NVHPC 22.7+ + HIP module: ROCm 4.5+ + DPC++ module: Intel OneAPI 2021.3+ with oneMKL and oneDPL. Set the CXX compiler to `dpcpp`. + Windows + MinGW: GCC 5.5+ + Microsoft Visual Studio: VS 2019+ + CUDA module: CUDA 9.2+, Microsoft Visual Studio + OpenMP module: MinGW. ### Version Support Changes + ROCm 4.0+ -> 4.5+ after [#1303](#1303) + Removed Cygwin pipeline and support [#1283](#1283) ### Interface Changes + Due to internal changes, `ConcreteExecutor::run` will now always throw if the corresponding module for the `ConcreteExecutor` is not build [#1234](#1234) + The constructor of `experimental::distributed::Vector` was changed to only accept local vectors as `std::unique_ptr` [#1284](#1284) + The default parameters for the `solver::MultiGrid` were improved. In particular, the smoother defaults to one iteration of `Ir` with `Jacobi` preconditioner, and the coarse grid solver uses the new direct solver with LU factorization. [#1291](#1291) [#1327](#1327) + The `iteration_complete` event gained a more expressive overload with additional parameters, the old overloads were deprecated. [#1288](#1288) [#1327](#1327) ### Deprecations + Deprecated less expressive `iteration_complete` event. Users are advised to now implement the function `void iteration_complete(const LinOp* solver, const LinOp* b, const LinOp* x, const size_type& it, const LinOp* r, const LinOp* tau, const LinOp* implicit_tau_sq, const array<stopping_status>* status, bool stopped)` [#1288](#1288) ### Added Features + A distributed Schwarz preconditioner. [#1248](#1248) + A GCR solver [#1239](#1239) + Flexible Gmres solver [#1244](#1244) + Enable Gmres solver for distributed matrices and vectors [#1201](#1201) + An example that uses Kokkos to assemble the system matrix [#1216](#1216) + A symbolic LU factorization allowing the `gko::experimental::factorization::Lu` and `gko::experimental::solver::Direct` classes to be used for matrices with non-symmetric sparsity pattern [#1210](#1210) + A numerical Cholesky factorization [#1215](#1215) + Symbolic factorizations in host-side operations are now wrapped in a host-side `Operation` to make their execution visible to loggers. This means that profiling loggers and benchmarks are no longer missing a separate entry for their runtime [#1232](#1232) + Symbolic factorization benchmark [#1302](#1302) + The `ProfilerHook` logger allows annotating the Ginkgo execution (apply, operations, ...) for profiling frameworks like NVTX, ROCTX and TAU. [#1055](#1055) + `ProfilerHook::created_(nested_)summary` allows the generation of a lightweight runtime profile over all Ginkgo functions written to a user-defined stream [#1270](#1270) for both host and device timing functionality [#1313](#1313) + It is now possible to enable host buffers for MPI communications at runtime even if the compile option `GINKGO_FORCE_GPU_AWARE_MPI` is set. [#1228](#1228) + A stencil matrices generator (5-pt, 7-pt, 9-pt, and 27-pt) for benchmarks [#1204](#1204) + Distributed benchmarks (multi-vector blas, SpMV, solver) [#1204](#1204) + Benchmarks for CSR sorting and lookup [#1219](#1219) + A timer for MPI benchmarks that reports the longest time [#1217](#1217) + A `timer_method=min|max|average|median` flag for benchmark timing summary [#1294](#1294) + Support for non-default streams in CUDA and HIP executors [#1236](#1236) + METIS integration for nested dissection reordering [#1296](#1296) + SuiteSparse AMD integration for fillin-reducing reordering [#1328](#1328) + Csr mixed-precision SpMV support [#1319](#1319) + A `with_loggers` function for all `Factory` parameters [#1337](#1337) ### Improvements + Improve naming of kernel operations for loggers [#1277](#1277) + Annotate solver iterations in `ProfilerHook` [#1290](#1290) + Allow using the profiler hooks and inline input strings in benchmarks [#1342](#1342) + Allow passing smart pointers in place of raw pointers to most matrix functions. This means that things like `vec->compute_norm2(x.get())` or `vec->compute_norm2(lend(x))` can be simplified to `vec->compute_norm2(x)` [#1279](#1279) [#1261](#1261) + Catch overflows in prefix sum operations, which makes Ginkgo's operations much less likely to crash. This also improves the performance of the prefix sum kernel [#1303](#1303) + Make the installed GinkgoConfig.cmake file relocatable and follow more best practices [#1325](#1325) ### Fixes + Fix OpenMPI version check [#1200](#1200) + Fix the mpi cxx type binding by c binding [#1306](#1306) + Fix runtime failures for one-sided MPI wrapper functions observed on some OpenMPI versions [#1249](#1249) + Disable thread pinning with GPU executors due to poor performance [#1230](#1230) + Fix hwloc version detection [#1266](#1266) + Fix PAPI detection in non-implicit include directories [#1268](#1268) + Fix PAPI support for newer PAPI versions: [#1321](#1321) + Fix pkg-config file generation for library paths outside prefix [#1271](#1271) + Fix various build failures with ROCm 5.4, CUDA 12, and OneAPI 6 [#1214](#1214), [#1235](#1235), [#1251](#1251) + Fix incorrect read for skew-symmetric MatrixMarket files with explicit diagonal entries [#1272](#1272) + Fix handling of missing diagonal entries in symbolic factorizations [#1263](#1263) + Fix segmentation fault in benchmark matrix construction [#1299](#1299) + Fix the stencil matrix creation for benchmarking [#1305](#1305) + Fix the additional residual check in IR [#1307](#1307) + Fix the cuSPARSE CSR SpMM issue on single strided vector when cuda >= 11.6 [#1322](#1322) [#1331](#1331) + Fix Isai generation for large sparsity powers [#1327](#1327) + Fix Ginkgo compilation and test with NVHPC >= 22.7 [#1331](#1331) + Fix Ginkgo compilation of 32 bit binaries with MSVC [#1349](#1349)

upsj added the 1:ST:need-feedback The PR is somewhat ready but feedback on a blocking topic is required before a proper review. label Mar 24, 2023

upsj requested a review from a team March 24, 2023 21:04

upsj self-assigned this Mar 24, 2023

upsj commented Mar 24, 2023

View reviewed changes

include/ginkgo/core/base/timer.hpp Show resolved Hide resolved

yhmtsai reviewed Mar 29, 2023

View reviewed changes

yhmtsai reviewed Mar 30, 2023

View reviewed changes

upsj force-pushed the device_timer branch from d10f27c to f605aec Compare March 30, 2023 15:03

MarcelKoch requested changes Apr 6, 2023

View reviewed changes

upsj force-pushed the device_timer branch from f605aec to f81a51d Compare May 7, 2023 14:33

upsj requested review from MarcelKoch and yhmtsai May 7, 2023 14:34

upsj added 1:ST:ready-for-review This PR is ready for review and removed 1:ST:need-feedback The PR is somewhat ready but feedback on a blocking topic is required before a proper review. labels May 7, 2023

MarcelKoch requested changes May 17, 2023

View reviewed changes

upsj force-pushed the device_timer branch from 6273a28 to 2af8f8d Compare May 17, 2023 13:55

upsj requested a review from MarcelKoch May 22, 2023 13:16

MarcelKoch approved these changes May 22, 2023

View reviewed changes

yhmtsai approved these changes May 23, 2023

View reviewed changes

upsj added 1:ST:ready-to-merge This PR is ready to merge. and removed 1:ST:ready-for-review This PR is ready for review labels May 23, 2023

create device timers for profiler_hook

9cd9387

upsj and others added 7 commits May 23, 2023 21:19

reduce overhead of time points

c136ddd

improvements and compilation fixes

a5e0ec7

- don't measure time waiting for timer as overhead - avoid cyclical dependency between time_point (core) and timers (device) - fix DPCPP timers - remove unnecessary scoping

review updates

0c7fa28

- change return type to chrono::nanoseconds - separate waiting from difference - add documentation Co-authored-by: Marcel Koch <[email protected]>

set profiling queue property for dpc++ tests

b77bad0

fix compilation

feb85e8

review updates

722005b

Co-authored-by: Yuhsiang M. Tsai <[email protected]>

upsj force-pushed the device_timer branch from aaf916b to 722005b Compare May 23, 2023 19:20

upsj added 4 commits May 24, 2023 10:02

fix dpcpp timer

8bfb9b3

work around compiler issues

fb7c0bc

work around another nvcc-hip issue

ad36725

fix union initialization in timer.cpp

6859a26

upsj merged commit 516b0c3 into develop May 25, 2023
10 of 13 checks passed

upsj deleted the device_timer branch May 25, 2023 04:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for device timers in summary hook #1313

Add support for device timers in summary hook #1313

upsj commented Mar 24, 2023

yhmtsai Mar 29, 2023

upsj Mar 29, 2023

yhmtsai left a comment •

edited

Loading

MarcelKoch left a comment

MarcelKoch left a comment

MarcelKoch May 17, 2023

upsj May 17, 2023

yhmtsai left a comment

yhmtsai May 23, 2023

upsj May 23, 2023

yhmtsai May 23, 2023

upsj May 23, 2023

upsj May 23, 2023

yhmtsai May 23, 2023

upsj May 24, 2023

sonarcloud bot commented May 25, 2023

		namespace gko {


		time_point::time_point() : type_{type::cpu}, data_{} {}

Add support for device timers in summary hook #1313

Add support for device timers in summary hook #1313

Conversation

upsj commented Mar 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yhmtsai left a comment • edited Loading

Choose a reason for hiding this comment

MarcelKoch left a comment

Choose a reason for hiding this comment

MarcelKoch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yhmtsai 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

sonarcloud bot commented May 25, 2023

yhmtsai left a comment •

edited

Loading