-
Notifications
You must be signed in to change notification settings - Fork 13.5k
Add profiler to bootstrap command #143525
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?
Conversation
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.
Thank you! I don't think that we'll need JSON output for now, I think that the main output should be some lightly formatted text that will be human interpretable.
@@ -679,6 +754,8 @@ impl<'a> DeferredCommand<'a> { | |||
&& let (Some(cache_key), Some(_)) = (&cache_key, output.status()) | |||
{ | |||
exec_ctx.command_cache.insert(cache_key.clone(), output.clone()); | |||
let duration = start_time.elapsed(); | |||
exec_ctx.profiler.record(cache_key.clone(), duration); |
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.
We should ideally record also the uncached commands. I guess that we could reuse the CacheKey logic to avoid introducing another approach for identifying the commands. So maybe let all commands return a cache key, and add a separate method for figuring out if caching is enabled or not? And maybe rename CacheKey
to CommandFingerprint
or something.
@@ -545,6 +607,7 @@ impl ExecutionContext { | |||
{ | |||
command.mark_as_executed(); | |||
self.verbose(|| println!("Cache hit: {command:?}")); | |||
self.profiler.record(cache_key.unwrap(), Duration::from_secs(0)); |
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.
We should represent this in a proper way, e.g. store a Vec
of execution traces, where each trace is either a cache hit, or an actual execution with a duration.
let mut stats = self.stats.lock().unwrap(); | ||
let entry = stats.entry(key).or_default(); | ||
entry.count += 1; | ||
entry.max_duration = std::cmp::max(entry.max_duration, duration); |
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.
Let's store all the durations, shouldn't be that expensive.
This PR adds command profiling to the bootstrap command. It tracks the total execution time and records cache hits for each command. It also provides the ability to export execution result to a JSON file. Integrating this with Chrome tracing could further enhance observability.
r? @Kobzol