merge kp-kernels-timers #235
Conversation
|
Can you please describe the behavior when setting |
|
Moreover, |
|
Thanks @blegouix I would like to request changing the environment variable name to The format for tools environment variables is |
|
I just want to point it out that there seems to be an error in one of the CI build tests. It comes from PAPI connector. The error is in the screenshot below. The CI simple-build (without Kokkos) passes. 
|
|
I think the PAPI thing is just a warning, but there is indeed an error there:
I'll check it, it is possible that namespace changed or something |
Great, thanks! Yeah, the Kokkos Tools CI tests don’t pass unless all warnings are eliminated. |
|
Can you run the CI again ? |
|
Can you run the CI again ? Also, I do not think I have the right version of clang-format, could you format it for me ? Thx, |
Done.
Use clang-format-8. On a Mac (if you have it), you can use homebrew and do |
| for (auto kernel_itr = count_map.begin(); kernel_itr != count_map.end(); | ||
| kernel_itr++) { | ||
| kernel_itr->second->writeToBinaryFile(output_data); | ||
| fprintf(output_data, "}\n}"); |
There was a problem hiding this comment.
It would be nice to have a test for this. Otherwise, how could a reviewer be sure that nothing broke ?
|
I have no idea why the remaining test fails :( |
|
Hello, can we merge this or do you think there are remaining things to do ? |
|
Started CI workflow. I am OK with not sorting the times. I see a broader motivation though - this is how CUDA nvtx tools display timing output, and so there is an expectation by a Kokkos user for this may be similar. I think the environment variable mentioned in your last message is a good temporary solution. Thanks a lot for your help and work here! |
|
The environment variable is not in the scope of this MR right ? Im on holidays next week so would be great if we can merge before that |
| fprintf(output_data, " \"total-kernel-times\" : %10.3f,\n", | ||
| kernelTimes); | ||
| fprintf(output_data, " \"total-non-kernel-times\" : %10.3f,\n", | ||
| (totalExecuteTime - kernelTimes)); | ||
|
|
||
| const double percentKokkos = (kernelTimes / totalExecuteTime) * 100.0; | ||
| fprintf(output_data, " \"percent-in-kernels\" : %6.2f,\n", |
There was a problem hiding this comment.
I'm not sure that aggregates should be part of the JSON report.
It makes sense that "console" reports do aggregate times so the user is presented with a nice summary.
But IMO JSON output should be as raw as possible, since I guess the sole purpose of JSON format is to post-process results somewhere else in a user-specific way.
I think it is not valuable that Kokkos Tools adds aggregated metrics in the JSON output...
There was a problem hiding this comment.
Maybe but it is not in the scope of the MR. It was already there in the timer_json.cpp
(but actually as a user I would say that if you remove aggregated metrics in the output you should provide an easy way to post-process automatically (like execute python script when kokkos finalizes), otherwise it makes things more complicated than they should for people who just want to monitor the performance of their code in development stage)
There was a problem hiding this comment.
This is good enough for now.
As a follow up I want us to consider using an environment variable (and command-line argument) to select the output format instead of the KOKKOS_TOOLS_TIMER_JSON boolean option.
I also want us to reuse the JSON writing code from kp_json_writer.cpp instead of duplicating.
And finally we need to add tests for that tool.
Environment flag "KOKKOS_TOOLS_TIMER_JSON" determines if export is done in .dat or .json format.
Regions are also exported in the .json (it was not the case previously)