Fix operator trace caching #5580
Conversation
Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>
Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>
| } | ||
|
|
||
|
|
||
| private: | ||
| std::string trace_name_; |
There was a problem hiding this comment.
The test was passing, because the trace name was the same in all operators - now it's configurable per-operator.
| const char *operator_name = operator_under_test_names[op]; | ||
| const char *trace_name = operator_trace_names[op]; | ||
| EXPECT_EQ(daliHasOperatorTrace(&h, operator_name, "this_trace_does_not_exist"), 0); | ||
| ASSERT_NE(daliHasOperatorTrace(&h, operator_name, trace_name), 0) |
There was a problem hiding this comment.
The test was passing, because the trace name was the same in all operators. Now there are different trace names so we know that we're looking at the right operator's trace.
| if (!operator_traces_) | ||
| operator_traces_ = &iter_data_->operator_traces.Get(operator_name); | ||
| return *operator_traces_; | ||
| return iter_data_->operator_traces.Get(operator_name); |
There was a problem hiding this comment.
Actual fix: We must query the map directly here.
| if (!operator_traces_) | ||
| operator_traces_ = &iter_data_->operator_traces.Get(operator_instance_name_); |
There was a problem hiding this comment.
Fix: The optimization should go here, not in the query by operator name.
| @@ -186,11 +195,13 @@ class OperatorTraceTestExternalInput : public OperatorTraceTest { | |||
| pipeline_->AddOperator(OpSpec("PassthroughWithTraceOp") | |||
| .AddArg("device", "cpu") | |||
| .AddInput("OP_TRACE_IN_CPU", "cpu") | |||
| .AddArg("trace_name", operator_trace_names[0]) | |||
There was a problem hiding this comment.
This caused a run-time (not compile-time!) error without the OpSpec fix.
|
CI MESSAGE: [16923093]: BUILD STARTED |
| @@ -52,8 +52,12 @@ OperatorTraceTestParam operator_trace_test_params_pipelined_executor_separate_qu | |||
| {2, 2, true, false}, | |||
| }; | |||
|
|
|||
| std::array<std::string, 2> operator_under_test_names = { | |||
| "PassthroughWithTraceOpCpu", "PassthroughWithTraceOpGpu" | |||
| const char *operator_under_test_names[] = { | |||
There was a problem hiding this comment.
We don't really want std::string for C-API tests.
| @@ -174,6 +174,11 @@ class DLL_PUBLIC OpSpec { | |||
| return this->SetArg<std::string>(name, c_str); | |||
| } | |||
|
|
|||
| // Forward to string implementation | |||
| DLL_PUBLIC inline OpSpec& SetArg(const string &name, const char *c_str) { | |||
There was a problem hiding this comment.
Without this overload, passing const char * (but not a literal, which is properly handled above) resulted in a run-time error - there was an attempt to set a pointer value to the argument, which was a bug.
Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>
mzient
force-pushed
the
fix_operator_trace_query
branch
from
a46e996 to
12525af
Compare
|
CI MESSAGE: [16923393]: BUILD STARTED |
|
CI MESSAGE: [16923393]: BUILD PASSED |
Category:
Bug fix (non-breaking change which fixes an issue)
Description:
Operator traces have an optimization that caches the trace map for the current executor. This was done with an assumption that the access function is not used for reading trace values for multiple operators - and in C API it was used exactly this way, causing errors.
This PR fixes the problem and adds tests that didn't pass prior to the fix.
Additionally, AddArg overload is added to OpSpec, making it possible to pass
const char *. This was bugged, causing weird run-time errors when attempting to add an argument this way.Additional information:
Affected modules and functionalities:
Key points relevant for the review:
Tests:
Checklist
Documentation
DALI team only
Requirements
REQ IDs: N/A
JIRA TASK: N/A