Skip to content

Commit

Permalink
Reland "[segmentation_platform] Fix nesting of model execution trace …
Browse files Browse the repository at this point in the history
…events"

This reverts commit c52c5f7.

Currently the ModelExecutionImpl::ExecuteModel trace event would finish
after its parent ModelExecutionImpl::ExecutionState event. The parent
is closed within RunModelExecutionCallback, whereas the child is closed
when ModelExecutionImpl::ExecuteModel ends since it is on the stack.

This could lead to confusing timestamps in the trace log.

This CL changes the implementation of this to ensure that the order of
closing the events is correct.

Original CL:
https://chromium-review.googlesource.com/c/chromium/src/+/4098382

Changes from original CL:
-   perfetto::Track is no longer used when looking up trace tracks.

Bug: 1393919, 1401661
Change-Id: I17549dc8a740c0c7497a6c86e168cf3b893e6b72
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4114468
Reviewed-by: Siddhartha S <ssid@chromium.org>
Commit-Queue: Tommy Nyquist <nyquist@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1084691}
  • Loading branch information
tommynyquist authored and Chromium LUCI CQ committed Dec 17, 2022
1 parent 8a457c6 commit 6e61ffa
Show file tree
Hide file tree
Showing 2 changed files with 21 additions and 15 deletions.
Expand Up @@ -109,7 +109,7 @@ void ModelExecutorImpl::ExecuteModel(
*state);

if (!state->model_provider || !state->model_provider->ModelAvailable()) {
RunModelExecutionCallback(std::move(state),
RunModelExecutionCallback(*state, std::move(state->callback),
std::make_unique<ModelExecutionResult>(
ModelExecutionStatus::kSkippedModelNotReady));
return;
Expand All @@ -119,8 +119,9 @@ void ModelExecutorImpl::ExecuteModel(
if (metadata_utils::ValidateSegmentInfo(segment_info) !=
metadata_utils::ValidationResult::kValidationSuccess) {
RunModelExecutionCallback(
std::move(state), std::make_unique<ModelExecutionResult>(
ModelExecutionStatus::kSkippedInvalidMetadata));
*state, std::move(state->callback),
std::make_unique<ModelExecutionResult>(
ModelExecutionStatus::kSkippedInvalidMetadata));
return;
}

Expand All @@ -141,8 +142,9 @@ void ModelExecutorImpl::OnProcessingFeatureListComplete(
if (error) {
// Validation error occurred on model's metadata.
RunModelExecutionCallback(
std::move(state), std::make_unique<ModelExecutionResult>(
ModelExecutionStatus::kSkippedInvalidMetadata));
*state, std::move(state->callback),
std::make_unique<ModelExecutionResult>(
ModelExecutionStatus::kSkippedInvalidMetadata));
return;
}
state->input_tensor.insert(state->input_tensor.end(), input_tensor.begin(),
Expand Down Expand Up @@ -205,28 +207,29 @@ void ModelExecutorImpl::OnModelExecutionComplete(
}
}
ModelProvider::Request input_tensor = state->input_tensor;
RunModelExecutionCallback(std::move(state),
RunModelExecutionCallback(*state, std::move(state->callback),
std::make_unique<ModelExecutionResult>(
std::move(input_tensor), *result));
} else {
VLOG(1) << "Segmentation model returned no result for segment "
<< proto::SegmentId_Name(state->segment_info.segment_id());
RunModelExecutionCallback(std::move(state),
RunModelExecutionCallback(*state, std::move(state->callback),
std::make_unique<ModelExecutionResult>(
ModelExecutionStatus::kExecutionError));
}
}

void ModelExecutorImpl::RunModelExecutionCallback(
std::unique_ptr<ExecutionState> state,
const ExecutionState& state,
ModelExecutionCallback callback,
std::unique_ptr<ModelExecutionResult> result) {
stats::RecordModelExecutionDurationTotal(
state->segment_info.segment_id(), result->status,
clock_->Now() - state->total_execution_start_time);
stats::RecordModelExecutionStatus(state->segment_info.segment_id(),
state->record_metrics_for_default,
state.segment_info.segment_id(), result->status,
clock_->Now() - state.total_execution_start_time);
stats::RecordModelExecutionStatus(state.segment_info.segment_id(),
state.record_metrics_for_default,
result->status);
std::move(state->callback).Run(std::move(result));
std::move(callback).Run(std::move(result));
}

} // namespace segmentation_platform
Expand Up @@ -67,8 +67,11 @@ class ModelExecutorImpl : public ModelExecutor {
const absl::optional<ModelProvider::Response>& result);

// Helper function for synchronously invoking the callback with the given
// result and status.
void RunModelExecutionCallback(std::unique_ptr<ExecutionState> state,
// result and status. Before invoking this, it is required to move the
// ExecutionState::callback out as a separate parameter, e.g.:
// `RunModelExecutionCallback(*state, std::move(state->callback), ...)`.
void RunModelExecutionCallback(const ExecutionState& state,
ModelExecutionCallback callback,
std::unique_ptr<ModelExecutionResult> result);

const raw_ptr<base::Clock> clock_;
Expand Down

0 comments on commit 6e61ffa

Please sign in to comment.