[XPTI][INFRA] Refactored API implementations to improve performance #19160
Conversation
Signed-off-by: Vasanth Tovinkere <[email protected]>
Refactors XPTI API calls to improve performance of event creation and string look up. Signed-off-by: Vasanth Tovinkere <[email protected]>
Signed-off-by: Vasanth Tovinkere <[email protected]>
tovinkere
had a problem deploying
to
WindowsCILock
GitHub Actions
Failure
Signed-off-by: Vasanth Tovinkere <[email protected]>
Signed-off-by: Vasanth Tovinkere <[email protected]>
xpti/include/xpti/xpti_data_types.h
Outdated
| @@ -515,12 +565,26 @@ struct payload_t { | |||
| stack_trace = caller_callee; | |||
| flags |= (uint64_t)payload_flag_t::StackTraceAvailable; | |||
| } | |||
| line_no = invalid_id<>; ///< Invalid line number | |||
| column_no = invalid_id<>; ///< Invalid column number | |||
There was a problem hiding this comment.
should we set source_file to invalid too?
There was a problem hiding this comment.
Initialization of the structure will set source_file = nullptr and "flags" = 0. So, it is safe. However, lines 568-569 can go as they are a part of initialization as well.
| Payload->line_no != xpti::invalid_id<uint32_t>; | ||
| // We ignore checking for column info as it may not always be available | ||
| isValid = ((hasSourceFile && hasLineInfo) || hasName || hasCodePtrVa) && | ||
| (Payload->flags != 0); |
There was a problem hiding this comment.
check (hasSourceFile && hasLineInfo) || hasName || hasCodePtrVa) implies Payload->flags != 0
There was a problem hiding this comment.
Agreed that the last test can go.
xptifw/src/xpti_trace_framework.cpp
Outdated
| LineNo = Payload->line_no; | ||
| ColNo = Payload->column_no; | ||
| // If the payload's source file is available, add it to the string table | ||
| // and get its id Also, get the line number and column number from the |
There was a problem hiding this comment.
we are not getting column and line number any more here. is it expected?
xptifw/src/xpti_trace_framework.cpp
Outdated
| } | ||
|
|
||
| // If the payload's source file is available, add it to the string table | ||
| // and get its id Also, get the line number and column number from the |
There was a problem hiding this comment.
same question here
There was a problem hiding this comment.
I do see we assign them below. then we should update comments.
There was a problem hiding this comment.
Fixed the comments
|
Regarding ABI - it should be backward compatible as the function that did change its signature is never used in SYCL instrumentation. So, no one is affected. Instrumentation in SYCL will incorporate the new API moving forward. |
can anyone else use these functions? |
|
No one is using the 128-bit ID queries as they chose to retain the 64-bit backward compatibility modes, hence ABI breakage will not affect anyone. Once SYCL transitions to the newer API, the tool chains can continue to use backward compatibility modes or use the new API. Ideally we may want to deprecate some functions, however, with there not being a significant performance impact anymore, they should co-exist nicely. |
Signed-off-by: Vasanth Tovinkere <[email protected]>
There was a problem hiding this comment.
I trust @tovinkere with ABI assessment as xptifw owner.
Other comments are fixed.
LGTM.
|
@intel/llvm-gatekeepers This can be merged after the checks are complete.. |
tovinkere
had a problem deploying
to
WindowsCILock
GitHub Actions
Failure
As a part of the performance study to improve SYCL runtime performance, it was documented that XPTI when enabled by a tool or collector was having higher than usual performance impact. This is the second PR to address the specific xptiMakeEvent* API and current implementation is ~30% faster than previous implementation. The last PR in this series will change the instrumentation in SYCL to address the large performance impact caused by mixing debug and performance streams in the runtime today.