-
Notifications
You must be signed in to change notification settings - Fork 48
add support of formatted metadata #174
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
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Alexey Kireev <[email protected]> Signed-off-by: Kireev, Alexey <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds formatted metadata support for the ITT task API, enabling formatted strings for task metadata and introducing support for overlapping ITT tasks.
- Added new functions (__itt_formatted_metadata_add and __itt_formatted_metadata_add_overlapped) and their macro definitions in ittnotify_static.h and ittnotify.h
- Introduced a new metadata type (__itt_metadata_string) in the metadata type enumeration
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
File | Description |
---|---|
src/ittnotify/ittnotify_static.h | Added macro stubs for formatted metadata functions to support the new API |
include/ittnotify.h | Added function declarations and documentation for the new formatted metadata API, and introduced a new metadata type (__itt_metadata_string) |
Comments suppressed due to low confidence (1)
src/ittnotify/ittnotify_static.h:213
- [nitpick] The parameter name 'id' in the formatted_metadata_add_overlapped function is inconsistent with the documentation, which refers to it as 'taskid'. Consider renaming it to 'taskid' for clarity and consistency.
ITT_STUBV(ITTAPI, void, formatted_metadata_add_overlapped, (const __itt_domain *domain, __itt_id id, __itt_string_handle *format, ...), \
Signed-off-by: Kireev, Alexey <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces support for formatted metadata in the ITT task API, enabling additional grouping options in VTune analysis views.
- Added two new functions (__itt_formatted_metadata_add and __itt_formatted_metadata_add_overlapped) to allow formatted metadata insertion between task begin and end calls.
- Updated header files and auto-generated Rust bindings (for Windows, macOS, and Linux) by upgrading the rust-bindgen version.
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
src/ittnotify/ittnotify_static.h | Added new function definitions for formatted metadata support |
rust/ittapi-sys/src/windows/jitprofiling_bindings.rs | Updated version notice for rust-bindgen |
rust/ittapi-sys/src/windows/ittnotify_bindings.rs | Added new function pointer definitions for formatted metadata |
rust/ittapi-sys/src/macos/jitprofiling_bindings.rs | Updated version notice for rust-bindgen |
rust/ittapi-sys/src/macos/ittnotify_bindings.rs | Added new function pointer definitions for formatted metadata |
rust/ittapi-sys/src/linux/jitprofiling_bindings.rs | Updated version notice for rust-bindgen |
rust/ittapi-sys/src/linux/ittnotify_bindings.rs | Added new function pointer definitions for formatted metadata |
include/ittnotify.h | Introduced formatted metadata API declarations and corresponding macros |
Comments suppressed due to low confidence (2)
src/ittnotify/ittnotify_static.h:210
- Consider adding tests for the new __itt_formatted_metadata_add and __itt_formatted_metadata_add_overlapped functions to ensure they integrate correctly with __itt_task_begin and __itt_task_end.
ITT_STUBV(ITTAPI, void, formatted_metadata_add, (const __itt_domain *domain, __itt_string_handle *format, ...), \
include/ittnotify.h:2461
- It is recommended to include usage examples or tests in the header documentation to validate the proper functionality of the formatted metadata API in both standard and overlapped contexts.
/* New formatted metadata API declarations */
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Please update API_VERSION_BUILD here:
ittapi/src/ittnotify/ittnotify_config.h
Line 211 in 03f7260
#define API_VERSION_BUILD 20250113 - Please update API_VERSION_NUM here:
ittapi/src/ittnotify/ittnotify_config.h
Line 214 in 03f7260
#define API_VERSION_NUM 3.25.4 - Please add new APIs calls into reference collector: src/ittnotify_refcol/itt_refcol_impl.c
include/ittnotify.h
Outdated
@@ -2454,9 +2454,59 @@ typedef enum { | |||
__itt_metadata_u16, /**< Unsigned 16-bit integer */ | |||
__itt_metadata_s16, /**< Signed 16-bit integer */ | |||
__itt_metadata_float, /**< Signed 32-bit floating-point */ | |||
__itt_metadata_double /**< SIgned 64-bit floating-point */ | |||
__itt_metadata_double, /**< Signed 64-bit floating-point */ | |||
__itt_metadata_string /**< String*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do you need this new __itt_metadata_string
type? I don't see it being used anywhere
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll need it later in another component to avoid type and/or code duplication
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you please provide an example of usage?
* @param[in] format The format of the metadata | ||
* @param[in] ... The metadata itself as multiple arguments | ||
*/ | ||
void ITTAPI __itt_formatted_metadata_add(const __itt_domain *domain, __itt_string_handle *format, ...); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you plan to separate char*
and wchar_t*
strings parameter somehow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, thank you for pointing into it. it's supported and will be covered in another component
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are you going to provide users the ability to explicitly specify what type of strings style they are using?
it would be better to document this somehow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it could be specified using the standard %s / %ls print format specifiers. As an example, there's a printf format string for metadata, say "Arguments go here %s %ls %ld", and we call the new API like "__itt_formatted_metadata_add(domain, format, argumen1, argument2, argument3)", where argument1 is a char*, argument2 is wchar_t* and argument3 is long int.
The Rust side looks good to me! |
With this we can add formatted metadata support for ITT task API.
At first we need to declare __itt_string_handle* format in a printf format style, for example "Running [%s] task iteration %llu". Format specifiers in square brackets create additional grouping options in VTune analysis views
Then we call function __itt_formatted_metadata_add with arguments (domain, format, arg1, arg2), where domain is associated __itt_domain, format is printf-style string, arg1 and arg2 are arguments for this specific format.
The new function should be called between __itt_task_begin and __itt_task_end calls.
Also there's a support for overlapped ITT tasks:
__itt_formatted_metadata_add_overlapped(const __itt_domain *domain, __itt_id taskid, __itt_string_handle *format, ...)
where we add taskid from the relevant overlapped ITT task.
Signed-off-by: Alexey Kireev [email protected]