Skip to content

Commit 9378cb7

Browse files
Ian Childsfacebook-github-bot
Ian Childs
authored andcommitted
Do local dep file cache lookup before a remote dep file cache lookup
Summary: I think we should always use local dep files if we have them, as they are faster to access that remote dep files. This changes the order of the lookups. I'm not sure if this needs a post in "Buck2 Dev", since it's not really an API change but it will affect some metrics. Tbh, I probably want to fix a bunch of metrics around this stuff anyway, so maybe worth a fuller post saying that I want to make this change along with some metrics changes. (E.g. right now I don't think we count identical dep file hits separately from actually dep file lookups in buck2 builds - though we do in buck2 actions). Reviewed By: blackm00n Differential Revision: D74064655 fbshipit-source-id: f6ad95f225c338c93f5b31ad3a70b57e90a06b75
1 parent 8567f53 commit 9378cb7

File tree

2 files changed

+25
-14
lines changed

2 files changed

+25
-14
lines changed

app/buck2_action_impl/src/actions/impls/run.rs

Lines changed: 13 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -677,6 +677,17 @@ impl RunAction {
677677
let result = match action_cache_result {
678678
ControlFlow::Break(_) => action_cache_result,
679679
ControlFlow::Continue(manager) => {
680+
// If we didn't find anything in the action cache, first do a local dep file cache lookup, and if that fails,
681+
// try to find a remote dep file cache hit.
682+
if should_fully_check_dep_file_cache {
683+
let lookup = dep_file_bundle
684+
.check_local_dep_file_cache(ctx, self.outputs.as_slice())
685+
.await?;
686+
if let Some((outputs, metadata)) = lookup {
687+
return Ok(ExecuteResult::LocalDepFileHit(outputs, metadata));
688+
}
689+
}
690+
680691
let remote_dep_file_result = ctx
681692
.remote_dep_file_cache(manager, &req, &prepared_action)
682693
.await;
@@ -697,21 +708,10 @@ impl RunAction {
697708
}
698709
};
699710

700-
// If the cache queries did not yield to a result, fallback to local dep file query (continuation), then execution.
711+
// If the cache queries did not yield to a result, then we need to execute the action.
701712
let mut result = match result {
702713
ControlFlow::Break(res) => res,
703-
ControlFlow::Continue(manager) => {
704-
if should_fully_check_dep_file_cache {
705-
let lookup = dep_file_bundle
706-
.check_local_dep_file_cache(ctx, self.outputs.as_slice())
707-
.await?;
708-
if let Some((outputs, metadata)) = lookup {
709-
return Ok(ExecuteResult::LocalDepFileHit(outputs, metadata));
710-
}
711-
}
712-
713-
ctx.exec_cmd(manager, &req, &prepared_action).await
714-
}
714+
ControlFlow::Continue(manager) => ctx.exec_cmd(manager, &req, &prepared_action).await,
715715
};
716716

717717
// If the action has a dep file, log the remote dep file key so we can look out for collisions

tests/core/build/test_dep_files.py

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -620,10 +620,21 @@ async def test_re_dep_file_query_change_tagged_unused_file(buck: Buck) -> None:
620620
# Check the MatchDepFiles events
621621
await check_match_dep_files(buck, expected_dep_file_match)
622622

623+
# # Change a file that is tracked by a dep file but shows up as unused, we get a local dep file cache hit
624+
# # as that is checked first.
625+
tagged_unused.write_text("CHANGE")
626+
result = await buck.build(*target_upload_enabled)
627+
output = result.get_build_report().output_for_target(target).read_text()
628+
assert output == "used1\nused2\nused3\n"
629+
630+
execution_kind = await _get_execution_kind(buck)
631+
assert execution_kind == ACTION_EXECUTION_KIND_LOCAL_DEP_FILE
632+
623633
# Change a file that is tracked by a dep file but shows up as unused, this will again result in one of
624634
# 1. A remote dep file cache hit and a subsequent dep file validation
625635
# 2. A remote dep file cache miss, fall back to local execution (local dep file cache is flushed)
626-
tagged_unused.write_text("CHANGE")
636+
await buck.debug("flush-dep-files")
637+
tagged_unused.write_text("CHANGE_AGAIN")
627638
result = await buck.build(*target_upload_enabled)
628639
output = result.get_build_report().output_for_target(target).read_text()
629640
assert output == "used1\nused2\nused3\n"

0 commit comments

Comments
 (0)