Profile externally loaded mechanisms #1691
Conversation
| @@ -55,8 +57,8 @@ class mechanism { | |||
|
|
|||
| // Forward to interface methods | |||
| void initialize() { ppack_.vec_t = *time_ptr_ptr; iface_.init_mechanism(&ppack_); } | |||
There was a problem hiding this comment.
It's probably not as useful, but I would suggest -- just for completeness -- to instrument all methods here.
There was a problem hiding this comment.
The reason they are not is because they're included in another section of the profiler. I agree though, I'll see what I can move around.
| @@ -68,6 +70,21 @@ class mechanism { | |||
| arb_mechanism_interface iface_; | |||
| arb_mechanism_ppack ppack_; | |||
| arb_value_type** time_ptr_ptr = nullptr; | |||
|
|
|||
| private: | |||
| #ifdef ARB_PROFILE_ENABLED | |||
There was a problem hiding this comment.
This seems like these two snippets would be useful elsewhere. I would therefore suggest moving them to arb::profile.
However, I am a wary of the static, it means that id is initialised once and only the first region is tagged. See here for a minimal example
https://gcc.godbolt.org/z/f3GPeWaW6
Therefore my final suggestion would be this
#ifdef ARB_PROFILE_ENABLED
#define PROFILE_ENTER(x) do { static auto id = arb::profile::profiler_region_id(x); arb::profile::profiler_enter(id); } while(0)
#else
#define PROFILE_ENTER(x) do {} while(0)
#endifThere was a problem hiding this comment.
You're right about static, all the instances of the mechanism class end up having the same name, and the profiler doesn't work. Does your suggestion fix this? I went a different route.
There was a problem hiding this comment.
Since it basically pastes in the static id = once per invocation, it must fix it, yes.
|
👋 Any updates? It seems this is mergeable? |
noraabiakar
changed the title
|
I pieced this together in a bit of a rush and it needs a bit of fine tuning. For example, this fix doesn't very well for mechanisms that have |
noraabiakar
force-pushed
the
profile_extern_cat
branch
from
c1862a3
to
5a49cca
Compare
noraabiakar
changed the title
|
bors try |
tryAlready running a review |
tryBuild failed: |
|
bors try |
tryBuild failed: |
|
bors try |
tryBuild succeeded: |
- Add support for profiling externally loaded mechanisms. - Move the profiler calls out of the generated C++ code into the `arb::mechanism` methods. - Remove the `--profile` flag from the modcc flags. - Replace `_` delimiter with `:` in profiler. - Replace `const char*` with `const std::string&` for profiler region representation. - Profiler may now contain empty regions that were registered but not profiled, so add some code to filer those out when generating final profile.
arb::mechanismmethods.--profileflag from the modcc flags._delimiter with:in profiler.const char*withconst std::string&for profiler region representation.