Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refining support for ITTNotify #2473

Merged
merged 6 commits into from Feb 3, 2017
Merged

Refining support for ITTNotify #2473

merged 6 commits into from Feb 3, 2017

Conversation

hkaiser
Copy link
Member

@hkaiser hkaiser commented Feb 1, 2017

This patch reduces the overheads introduced when using ITTNotify to tie in VTune (or other tools).

  • adding meta-data support for tasks to pass more information to external tools
  • flyby: explicitly defining more base_lco_with_value types

- making itt task API more efficient (by caching the itt string for the task name)
- flyby: explicitly defining all necessary base_lco_with_value types
template <> HPX_ALWAYS_EXPORT \
char const* get_action_name<action>(); \
template <> HPX_ALWAYS_EXPORT \
util::itt::string_handle const& get_action_name_itt<action>(); \
Copy link
Member

Choose a reason for hiding this comment

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

Can we just factor the get_action_name_itt declaration out? this would avoid the duplication of the code in those macros.

return BOOST_PP_STRINGIZE(actionname); \
} \
template<> HPX_ALWAYS_EXPORT \
util::itt::string_handle const& get_action_name_itt<action>() \
Copy link
Member

Choose a reason for hiding this comment

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

Same as below

HPX_EXPORT void itt_task_end(___itt_domain const*);

HPX_EXPORT ___itt_domain* itt_domain_create(char const*);
HPX_EXPORT ___itt_string_handle* itt_task_handle_create(char const*);
HPX_EXPORT ___itt_string_handle* itt_string_handle_create(char const*);
Copy link
Member

Choose a reason for hiding this comment

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

do we have some minimum itt version we support?

Copy link
Member Author

Choose a reason for hiding this comment

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

None we could check at compile time. The Intel itt_notify.h has the same version number in it for years, even while the API itself has changed significantly.

Copy link
Member

Choose a reason for hiding this comment

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

Should we document the supported version somewhere?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't even know what version is the correct one... But using the newest VTune is probably a safe bet ;)

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough ...

@hkaiser
Copy link
Member Author

hkaiser commented Feb 2, 2017

@sithhell Your review comments have been addressed.

@sithhell sithhell merged commit aae15dd into master Feb 3, 2017
@sithhell sithhell deleted the itt_notify branch February 3, 2017 06:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants