-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Detect when the initial binary launch may be slow on macOS #15908
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.
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
base: master
Are you sure you want to change the base?
Changes from all commits
8ce3636
33bd6be
7afd727
90e4c21
c96e354
6bcb08c
56693aa
ca66bfd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -43,9 +43,10 @@ use crate::core::compiler::UserIntent; | |||||||||||||||
use crate::core::compiler::unit_dependencies::build_unit_dependencies; | ||||||||||||||||
use crate::core::compiler::unit_graph::{self, UnitDep, UnitGraph}; | ||||||||||||||||
use crate::core::compiler::{BuildConfig, BuildContext, BuildRunner, Compilation}; | ||||||||||||||||
use crate::core::compiler::{CompileKind, CompileTarget, RustcTargetData, Unit}; | ||||||||||||||||
use crate::core::compiler::{CompileKind, CompileMode, CompileTarget, RustcTargetData, Unit}; | ||||||||||||||||
use crate::core::compiler::{CrateType, TargetInfo, apply_env_config, standard_lib}; | ||||||||||||||||
use crate::core::compiler::{DefaultExecutor, Executor, UnitInterner}; | ||||||||||||||||
use crate::core::features::DetectAntivirus; | ||||||||||||||||
use crate::core::profiles::Profiles; | ||||||||||||||||
use crate::core::resolver::features::{self, CliFeatures, FeaturesFor}; | ||||||||||||||||
use crate::core::resolver::{HasDevUnits, Resolve}; | ||||||||||||||||
|
@@ -55,7 +56,7 @@ use crate::ops; | |||||||||||||||
use crate::ops::resolve::{SpecsAndResolvedFeatures, WorkspaceResolve}; | ||||||||||||||||
use crate::util::context::{GlobalContext, WarningHandling}; | ||||||||||||||||
use crate::util::interning::InternedString; | ||||||||||||||||
use crate::util::{CargoResult, StableHasher}; | ||||||||||||||||
use crate::util::{CargoResult, StableHasher, detect_antivirus}; | ||||||||||||||||
|
||||||||||||||||
mod compile_filter; | ||||||||||||||||
pub use compile_filter::{CompileFilter, FilterRule, LibRule}; | ||||||||||||||||
|
@@ -228,7 +229,11 @@ pub fn create_bcx<'a, 'gctx>( | |||||||||||||||
|
||||||||||||||||
// Perform some pre-flight validation. | ||||||||||||||||
match build_config.intent { | ||||||||||||||||
UserIntent::Test | UserIntent::Build | UserIntent::Check { .. } | UserIntent::Bench => { | ||||||||||||||||
UserIntent::Test | ||||||||||||||||
| UserIntent::Build | ||||||||||||||||
| UserIntent::Install | ||||||||||||||||
| UserIntent::Check { .. } | ||||||||||||||||
| UserIntent::Bench => { | ||||||||||||||||
if ws.gctx().get_env("RUST_FLAGS").is_ok() { | ||||||||||||||||
gctx.shell().warn( | ||||||||||||||||
"Cargo does not read `RUST_FLAGS` environment variable. Did you mean `RUSTFLAGS`?", | ||||||||||||||||
|
@@ -556,6 +561,40 @@ where `<compatible-ver>` is the latest version supporting rustc {rustc_version}" | |||||||||||||||
} | ||||||||||||||||
} | ||||||||||||||||
|
||||||||||||||||
if build_config.detect_antivirus != DetectAntivirus::Never { | ||||||||||||||||
// Count the number of test binaries and build scripts we'll need to | ||||||||||||||||
// run. This doesn't take into account the binary that will be run | ||||||||||||||||
// if `cargo run` was specified, and doesn't handle pre-2024 `rustdoc` | ||||||||||||||||
// tests, but that's fine, this is only a heuristic. | ||||||||||||||||
let num_binaries = unit_graph | ||||||||||||||||
.keys() | ||||||||||||||||
.filter(|unit| { | ||||||||||||||||
matches!( | ||||||||||||||||
unit.mode, | ||||||||||||||||
CompileMode::Test | CompileMode::Doctest | CompileMode::RunCustomBuild | ||||||||||||||||
) | ||||||||||||||||
}) | ||||||||||||||||
.count(); | ||||||||||||||||
|
||||||||||||||||
tracing::debug!("estimated {num_binaries} binaries that could be slowed down by antivirus"); | ||||||||||||||||
|
||||||||||||||||
// Heuristic: Only do the check if we have to run more than a specific | ||||||||||||||||
// number of binaries. This makes it so that small beginner projects | ||||||||||||||||
// don't hit this. | ||||||||||||||||
// | ||||||||||||||||
// We also don't want to do this check when installing, since there | ||||||||||||||||
// might be `cargo install` users who are not necessarily developers | ||||||||||||||||
// (and so the note will be irrelevant to them). | ||||||||||||||||
if (10 < num_binaries && build_config.intent != UserIntent::Install) | ||||||||||||||||
|| build_config.detect_antivirus == DetectAntivirus::Always | ||||||||||||||||
Comment on lines
+588
to
+589
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As an alternative, or additional thing, we could consider only doing the check once every X days, a bit similar to the automatic garbage collection. This has the problem that it might take a long time for the user to notice the message, and worse, it makes Cargo's output non-deterministic, so I'm inclined not to do it. |
||||||||||||||||
{ | ||||||||||||||||
if let Err(err) = detect_antivirus::detect_and_report(gctx) { | ||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One thing I wanted to note is that there is a performance cost to this. At least on my system, an error result took over 60ms, and a passing result took 14ms. We try to keep cargo's overhead relatively small (I like to keep the budget under 500ms total). 14ms is probably ok, but it's inching towards the territory where it is significant. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, you're right. On my machine (to have comparable numbers to those I've given elsewhere), I get:
This is of course unfortunate, and is more reason to not do the check unless the heuristic triggers (whatever the heuristic ends up being). |
||||||||||||||||
// Errors in this detection are not fatal. | ||||||||||||||||
tracing::error!("failed detecting whether binaries may be slow to run: {err}"); | ||||||||||||||||
} | ||||||||||||||||
Comment on lines
+591
to
+594
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am a bit unsure whether the end of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any particular reason for the concern? Note that we have another type of check in here, the rustc compatibility one which you put this right after so that works from a consolidating environment checks perspective. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's the right place, but I just wanted to call to attention that I'm not familiar with Cargo's internals, and that there might be a more suited place? But if you say it's correct, then it's correct. |
||||||||||||||||
} | ||||||||||||||||
} | ||||||||||||||||
|
||||||||||||||||
let bcx = BuildContext::new( | ||||||||||||||||
ws, | ||||||||||||||||
pkg_set, | ||||||||||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.