Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(turborepo): Feature flagged off file hashing #8229

Merged
merged 2 commits into from
May 28, 2024
Merged

chore(turborepo): Feature flagged off file hashing #8229

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cd1b488
Feature flagged off file hashing
NicholasLYang May 28, 2024
a5031dc
PR feedback
NicholasLYang May 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion crates/turborepo-lib/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,8 @@ default = ["rustls-tls", "daemon-package-discovery"]
native-tls = ["turborepo-api-client/native-tls", "turbo-updater/native-tls"]
rustls-tls = ["turborepo-api-client/rustls-tls", "turbo-updater/rustls-tls"]

go-daemon = []
daemon-package-discovery = []
daemon-file-hashing = []

# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html
[dev-dependencies]
Expand Down
126 changes: 67 additions & 59 deletions crates/turborepo-lib/src/task_hash.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,13 @@
use std::{
collections::{HashMap, HashSet},
sync::{Arc, Mutex},
time::Duration,
};

use rayon::prelude::*;
use serde::Serialize;
use thiserror::Error;
use tracing::{debug, Span};
use turbopath::{
AbsoluteSystemPath, AnchoredSystemPath, AnchoredSystemPathBuf, RelativeUnixPathBuf,
};
use turbopath::{AbsoluteSystemPath, AnchoredSystemPath, AnchoredSystemPathBuf};
use turborepo_cache::CacheHitMetadata;
use turborepo_env::{BySource, DetailedMap, EnvironmentVariableMap, ResolvedEnvMode};
use turborepo_repository::package_graph::{PackageInfo, PackageName};
Expand Down Expand Up @@ -83,7 +80,6 @@ impl PackageInputsHashes {
tracing::trace!(scm_manual=%scm.is_manual(), "scm running in {} mode", if scm.is_manual() { "manual" } else { "git" });

let span = Span::current();
let handle = tokio::runtime::Handle::current();
let (hashes, expanded_hashes): (HashMap<_, _>, HashMap<_, _>) = all_tasks
.filter_map(|task| {
let span = tracing::info_span!(parent: &span, "calculate_file_hash", ?task);
Expand All @@ -92,10 +88,6 @@ impl PackageInputsHashes {
return None;
};

let mut daemon = daemon
.as_ref() // Option::ref
.cloned();

let task_definition = match task_definitions
.get(task_id)
.ok_or_else(|| Error::MissingPipelineEntry(task_id.clone()))
Expand Down Expand Up @@ -126,58 +118,74 @@ impl PackageInputsHashes {
let scm_telemetry = package_task_event.child();
// Try hashing with the daemon, if we have a connection. If we don't, or if we
// timeout or get an error, fallback to local hashing
let hash_object = daemon
.as_mut()
.and_then(|daemon| {
let handle = handle.clone();
// We need an async block here because the timeout must be created with an
// active tokio context. Constructing it directly in
// the rayon thread doesn't provide one and will crash at runtime.
handle
.block_on(async {
tokio::time::timeout(
Duration::from_millis(100),
daemon.get_file_hashes(package_path, &task_definition.inputs),
)
.await
})
.map_err(|e| {
tracing::debug!(
"daemon file hashing timed out for {}",
package_path
);
e
})
.ok() // If we timed out, we don't need to error,
// just return None so we can move on to local
})
.and_then(|result| {
match result {
Ok(hashes_resp) => Some(
hashes_resp
.file_hashes
.into_iter()
.map(|(path, hash)| {
(
RelativeUnixPathBuf::new(path)
.expect("daemon returns relative unix paths"),
hash,
)
})
.collect::<HashMap<_, _>>(),
),
Err(e) => {
// Daemon could've failed for various reasons. We can still try
// local hashing.
tracing::debug!(
"daemon file hashing failed for {}: {}",
package_path,
#[cfg(feature = "daemon-file-hashing")]
let hash_object = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Super minor, but when possible I think if cfg!("daemon-file-hashing") {...} else {...} is better than conditional compilation in situations like this. Too often have we had code that doesn't compile when a feature isn't enabled.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do both syntaxes compile away the branch that isn't used? +1 for writing in a way that protects us from code that doesn't compile

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I'd be really surprised if LLVM can't figure out a const propagation and dead code elimination

let handle = tokio::runtime::Handle::current();
let mut daemon = daemon
.as_ref() // Option::ref
.cloned();

daemon
.as_mut()
.and_then(|daemon| {
let handle = handle.clone();
// We need an async block here because the timeout must be created with
// an active tokio context. Constructing it
// directly in the rayon thread doesn't
// provide one and will crash at runtime.
handle
.block_on(async {
tokio::time::timeout(
std::time::Duration::from_millis(100),
daemon
.get_file_hashes(package_path, &task_definition.inputs),
)
.await
})
.map_err(|e| {
tracing::debug!(
"daemon file hashing timed out for {}",
package_path
);
e
);
None
})
.ok() // If we timed out, we don't need to
// error,
// just return None so we can move on to
// local
})
.and_then(|result| {
match result {
Ok(hashes_resp) => Some(
hashes_resp
.file_hashes
.into_iter()
.map(|(path, hash)| {
(
turbopath::RelativeUnixPathBuf::new(path)
.expect("daemon returns relative unix paths"),
hash,
)
})
.collect::<HashMap<_, _>>(),
),
Err(e) => {
// Daemon could've failed for various reasons. We can still try
// local hashing.
tracing::debug!(
"daemon file hashing failed for {}: {}",
package_path,
e
);
None
}
}
}
});
})
};

#[cfg(not(feature = "daemon-file-hashing"))]
let hash_object = None;

let mut hash_object = match hash_object {
Some(hash_object) => hash_object,
None => {
Expand Down
1 change: 0 additions & 1 deletion crates/turborepo/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ license = "MPL-2.0"
default = ["rustls-tls", "turborepo-lib/daemon-package-discovery"]
native-tls = ["turborepo-lib/native-tls"]
rustls-tls = ["turborepo-lib/rustls-tls"]
go-daemon = ["turborepo-lib/go-daemon"]
pprof = ["turborepo-lib/pprof"]

# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html
Expand Down
Loading