Skip to content

Commit

Permalink
Instrument a few commit-related methods
Browse files Browse the repository at this point in the history
Summary:
## This stack

Track and down methods that are holding up the reactor and optimize them.

## This diff

Add instrumentation to a few slow methods. No expectation for performance improvements yet, but this will give us more info.

Reviewed By: RajivTS

Differential Revision: D66494769

fbshipit-source-id: 09115ab931884cbbe8d7d74efc474861ebf37cde
  • Loading branch information
andreacampi authored and facebook-github-bot committed Dec 5, 2024
1 parent b38e498 commit e4780c2
Showing 1 changed file with 44 additions and 13 deletions.
57 changes: 44 additions & 13 deletions eden/mononoke/scs/scs_methods/src/methods/commit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ use futures::stream::FuturesOrdered;
use futures::stream::StreamExt;
use futures::stream::TryStreamExt;
use futures::try_join;
use futures_watchdog::WatchdogExt;
use hooks::HookExecution;
use hooks::HookOutcome;
use itertools::Either;
Expand Down Expand Up @@ -264,13 +265,21 @@ impl SourceControlServiceImpl {
params: thrift::CommitCommonBaseWithParams,
) -> Result<thrift::CommitLookupResponse, scs_errors::ServiceError> {
let (_repo, changeset, other_changeset) = self
.repo_changeset_pair(ctx, &commit, &params.other_commit_id)
.repo_changeset_pair(ctx.clone(), &commit, &params.other_commit_id)
.watched(ctx.logger())
.await?;
let lca = changeset
.common_base_with(other_changeset.id())
.watched(ctx.logger())
.await?;
let lca = changeset.common_base_with(other_changeset.id()).await?;
Ok(thrift::CommitLookupResponse {
exists: lca.is_some(),
ids: if let Some(lca) = lca {
Some(map_commit_identity(&lca, &params.identity_schemes).await?)
Some(
map_commit_identity(&lca, &params.identity_schemes)
.watched(ctx.logger())
.await?,
)
} else {
None
},
Expand Down Expand Up @@ -583,26 +592,40 @@ impl SourceControlServiceImpl {
) -> Result<thrift::CommitCompareResponse, scs_errors::ServiceError> {
let (base_changeset, other_changeset) = match &params.other_commit_id {
Some(id) => {
let (_repo, mut base_changeset, other_changeset) =
self.repo_changeset_pair(ctx.clone(), &commit, id).await?;
add_mutable_renames(&mut base_changeset, &params).await?;
let (_repo, mut base_changeset, other_changeset) = self
.repo_changeset_pair(ctx.clone(), &commit, id)
.watched(ctx.logger())
.await?;
add_mutable_renames(&mut base_changeset, &params)
.watched(ctx.logger())
.await?;
(base_changeset, Some(other_changeset))
}
None => {
let (repo, mut base_changeset) = self.repo_changeset(ctx.clone(), &commit).await?;
add_mutable_renames(&mut base_changeset, &params).await?;
let (repo, mut base_changeset) = self
.repo_changeset(ctx.clone(), &commit)
.watched(ctx.logger())
.await?;
add_mutable_renames(&mut base_changeset, &params)
.watched(ctx.logger())
.await?;
let other_changeset = self
.find_commit_compare_parent(&repo, &mut base_changeset, &params)
.watched(ctx.logger())
.await?;
(base_changeset, other_changeset)
}
};

// Log the generation difference to drill down on clients making
// expensive `commit_compare` requests
let base_generation = base_changeset.generation().await?.value();
let base_generation = base_changeset
.generation()
.watched(ctx.logger())
.await?
.value();
let other_generation = match other_changeset {
Some(ref cs) => cs.generation().await?.value(),
Some(ref cs) => cs.generation().watched(ctx.logger()).await?.value(),
// If there isn't another commit, let's use the same generation
// to have a difference of 0.
None => base_generation,
Expand Down Expand Up @@ -651,18 +674,21 @@ impl SourceControlServiceImpl {
paths,
diff_items,
)
.watched(ctx.logger())
.await?
}
None => {
base_changeset
.diff_root_unordered(paths, diff_items)
.watched(ctx.logger())
.await?
}
};
stream::iter(diff)
.map(CommitComparePath::from_path_diff)
.buffer_unordered(CONCURRENCY_LIMIT)
.try_collect::<Vec<_>>()
.watched(ctx.logger())
.await?
.into_iter()
.partition_map(|diff| match diff {
Expand Down Expand Up @@ -698,6 +724,7 @@ impl SourceControlServiceImpl {
ChangesetFileOrdering::Ordered { after },
Some(limit),
)
.watched(ctx.logger())
.await?
}
None => {
Expand All @@ -708,6 +735,7 @@ impl SourceControlServiceImpl {
ChangesetFileOrdering::Ordered { after },
Some(limit),
)
.watched(ctx.logger())
.await?
}
};
Expand All @@ -716,6 +744,7 @@ impl SourceControlServiceImpl {
.map(CommitComparePath::from_path_diff)
.collect::<FuturesOrdered<_>>()
.try_collect::<Vec<_>>()
.watched(ctx.logger())
.await?;
if diff_items.len() >= limit {
if let Some(item) = diff_items.last() {
Expand All @@ -731,9 +760,11 @@ impl SourceControlServiceImpl {

let other_commit_ids = match other_changeset {
None => None,
Some(other_changeset) => {
Some(map_commit_identity(&other_changeset, &params.identity_schemes).await?)
}
Some(other_changeset) => Some(
map_commit_identity(&other_changeset, &params.identity_schemes)
.watched(ctx.logger())
.await?,
),
};
Ok(thrift::CommitCompareResponse {
diff_files,
Expand Down

0 comments on commit e4780c2

Please sign in to comment.